Correctly reset chunk during artifact upload on retry#458
Conversation
| strategy: | ||
| matrix: | ||
| runs-on: [ubuntu-latest, windows-latest, macOS-latest] | ||
| runs-on: [ubuntu-latest, windows-latest, macos-latest] |
There was a problem hiding this comment.
All of our public documentation has macos so I'm changing it just for consistency. A GitHub user actually brought this up in one of my other PRs so I'm looking out for this now.
https://github.com/actions/virtual-environments#available-environments
There was a problem hiding this comment.
These are case-insensitive and macOS is more correct than macos based on how Apple names it
| @@ -1,13 +1,21 @@ | |||
| name: artifact-unit-tests | |||
| on: push | |||
| on: | |||
There was a problem hiding this comment.
Also updating these to match our filters in unit-tests.yml
| passThrough.end(buffer) | ||
| uploadStream = passThrough | ||
| openUploadStream = () => { | ||
| const passThrough = new stream.PassThrough() |
There was a problem hiding this comment.
I realize this was the existing code, but why do we need a stream that is both readable and writable for upload?
There was a problem hiding this comment.
It's an implementation detail, we have an in-memory buffer and we have to convert it somehow to a readable stream. A passthrough stream is used to get the job done without any extra pipping or secondary streams. During the actual upload, it's treated only as a readable stream so it makes no difference.
I think this is where I originally got the idea from: https://stackoverflow.com/questions/16038705/how-to-wrap-a-buffer-as-a-stream2-readable-stream
I recall experimenting with a bunch of other techniques but they all ended up being considerably more complex.
There was a problem hiding this comment.
Could we document that in https://github.com/actions/toolkit/blob/master/packages/artifact/docs/implementation-details.md?
There was a problem hiding this comment.
* Correctly reset chunk during artifact upload on retry * Update workflow * Implementation details around the passthrough stream
3 participants
Fixes weird retry behavior reported here: actions/upload-artifact#71
If there is a connection error or timeout, the readstream should be successfully reset to the beginning so the subsequent retries should not give the same error.
actions/cacherecently fixed this issue in a PR so this is largely copying over the same fix: actions/cache#305