vscode: fix binary is not functional on windows

[Veetaha]

This is my first approach to fix this error, need to double-check this on windows still...
Fixes #3087
Fixes #3090

[matklad]

Seems reasonable!

[Veetaha]

Switched to stream.pipeline(), this should prevent us from making this kind of errors at all
@matklad

[Veetaha]

Wait, rechecked on windows, and this damn pipeline api doesn't actually work ...

[Veetaha]

God, I love Node's API. Will revert to the first impl, since it does work...

[Veetaha]

Now we are ready to merge

[marcogroppo]

Just trying to understand: what happens if there is a write error? Is there the possibility that the promise will stay in the pending status forever?

[Veetaha]

This would be unfortunate, though yes, I thought about it and handling double reject would bring some amount of "who-knows-what-it-is-doing" code. For this reason I tried to switch to stream.pipeline() API, but it doesn't resolve when the write-stream is closed either. I'll try to come up with better error handling here, but in a separate PR, let's leave this one as one particular fix an nothing more

[matklad]

bors r+

[bors]

Build succeeded

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

[Veetaha]

@marcogroppo , I've just checked ECMAScript specs. And found that we do can call reject() more than 1 time, so yes, we can omit the code that ensures that reject is not called repeatedly. just on("close", resolve).on("error", reject) will work. I'll create a PR for that today.
Thank you for the involvement!

[yoshuawuyts]

It's recommended to use stream.pipeline in all cases over stream.pipe. Formerly this functionality was only available in the pump and pumpify packages, but was added two years ago to fix the problems around pipe (in particular: error handling, and correctly detecting close events -- as witnessed here). Also when used with util.promisify it allows simplifying Promise construction for the pipeline:

const pipeline = util.promisify(stream.pipeline);

let bs = response.body;
  .on("data", (chunk: Buffer) => {
    readBytes += chunk.length;
    onProgress(readBytes, totalBytes);
  });
await pipeline(bs, fs.createWriteStream(destFilePath);

nb: I used to be on the node streams wg up until a few years ago 😅

[Veetaha]

@yoshuawuyts , thank you for the suggestion, I did look into stream.pipeline(), but then reverted back, since it doesn't resolve the promise when the write-stream is closed. You can see my comments about trying to incorporate it in this PR, but still it didn't fix the error, but my initial approach with .on("close") did, anyway... Maybe this is a bug in NodeJS?

[Veetaha]

Though, no, I cannot reproduce it again with stream.pipeline()... I need to recheck it on my home laptop where I did saw it was not working. Or maybe I just was overwhelmed by the bug and somehow was running the wrong code? I am not sure... Thank you for the pointer anyway, I'll double-recheck that stream.pipeline() does resolve the promise when the write-stream is closed today!

[Veetaha]

Though, no, here is a gist of the reproduction.
When I run it, I get RESOLVED printed before CLOSED, though there is no EBUSY error on my current PC...

[yoshuawuyts]

Okay, so this turned out to be a fun one.

In the example you're listening for the "close" event, which you would rightfully assume is called "at the end of the pipeline". However that's not what it does: it's emitted when the underlying file descriptor is closed. Which because Node.js doesn't have destructors it's emitted when the program is closed: which means it's last.

However if you listen for the "finish" event instead, it will trigger when you expect it to: namely when the writing to the writer has finished. The reason this doesn't also close the file descriptor is because it's possible to reuse the same write stream in multiple pipelines and the descriptor should be kept open.

If you want to close the file descriptor after the pipeline has finished, call .destroy() on the stream. The docs don't mention this, but it's the standard way of doing it and I checked the source to double check.

Either way: this is definitely harder than it should be, but at least with pipeline it's harder to get wrong.

Example

I believe the downloadFile function should be defined like this to correctly wait for it to finish, and then close the fd:

async function downloadFile(url, destFilePath) {
    let source = await fetch(url).body;
    let sink = fs.createWriteStream(destFilePath, { mode: 0o755 });
    await pipeline(source, sink);
    sink.destroy()
}

[Veetaha]

We cannot promisify neither .destroy(), nor .end(), they are synchronous and don't accept any callbacks. One thing we can do is:

const destFileStream = fs.createWriteStream(path, { mode });
await stream.pipeline(response.body, destFileStream);
return new Promise(resolve => {
    destFileStream.on("close", resolve);
    stream.destroy();
});

This looks quite hacky...

[Veetaha]

Hmm, wait, stream.end() does accept an optional callback...

[Veetaha]

No stream.end() accepts a callback that is just attached to 'finish' event...

I suppose we can use the snippet I wrote higher for now...

[Veetaha]

I've switched to stream.pipeline() with .on("close") in #3116. Let's leave it like that for now...
@matklad ?

[Veetaha]

FYI: I have created an issue on nodejs repo regarding stream.destroy() not releasing the file handle immediatly.
nodejs/node#31776