Hotfix skipping the first chunks of the artifacts

[Veetaha]

Quick hotfix.
fixes: #4986, #4987

The stream starts being consumed once we put a handler for data event.
When extracting stream.pipeline() under withTempFile in #4963 I didn't move it into the scope too, which due to preliminary awaiting for async operations with the file system allowed for the first chunks of the file to be skipped

[Veetaha]

@matklad sorry for that, for some reason this doesn't error out on my laptop, we should publish a hotfix for this

[lnicola]

That code is so weird. Does attaching the data handler start the download? Why does this change fix the problem?

I would have thought that either data and pipeline don't interact, or that there'd be a race between them.

[Veetaha]

@lnicola

The first chunks of the file are received asynchronously after the current call stack is empty.
Before the hotfix we had:

stream.on("data", chunk => { reportProgress(chunk.length) });
await withTempFile(async x => await stream.pipeline(x));
// Inside of `withTempFile() we await for async operations with the file system,
// whilst we have already put the first handler for `data` event, this means
// that the stream starts being consumed and while we are waiting for async file system operations
// the handler with `reportProgress()` starts consuming the first chunks of the stream
// after the async operations for creating the temp file are done `stream.pipeline()` internally puts
// its own `data` handler but this happens only after the part of the stream has already been consumed.

After the hotfix:

await withTempFile(x => {
    // we have put our own handler, the stream will be consumed starting from the next spin of the event loop
    stream.on("data", chunk => { reportProgress(chunk.length) }); 
    // Now stream.pipeline() has put its own handler (now there are 2 handlers total)
    // But no bytes have been consumed, since handlers are called only on the next spin of the event loop
    await stream.pipeline(x);
})

[lnicola]

The first chunks of the file are received asynchronously after the current call stack is empty.

we have put our own handler, the stream will be consumed starting from the next spin of the event loop

Ohh..

[Veetaha]

In pseudocode:

- registrer on("data", chunk => reportProgress(chunk.length))
- await withTempFile() -> await mkdirTemp() // empties the current async callstack
- reportPorgess() handler consumes the first chunk
- mkdirTemp() is finished, await is resumed
- await stream.pipeline() // registers own on("data") handler, empties the current async callstack
- reportProgress() and internal stream.pipeline's handler consume the **second** chunk

[lnicola]

I got it. I was missing the "stream starts on the next event loop after the first handler is attached" part.

[matklad]

bors r+

Thanks for looking into this @Veetaha ! Seems like a hell of a bug 😱 :(

I'll try to make a patch release...

[Veetaha]

Yup, this hotfix is sufficient as a fix, but the core problem was in mixing temp dir logic with the download logic, I'll submit a PR to decouple them to further prevent such mistakes

[bors]

Build succeeded:

• TypeScript (windows-latest)
• Rust (macos-latest)
• TypeScript (ubuntu-latest)
• Rust (ubuntu-latest)
• TypeScript (macos-latest)
• Rust (windows-latest)

[matklad]

Release is published!