Repository navigation
Writable stream is not immediatly closed after .destroy() call #31776
Description
- Version: 12.6.0
- Platform:
64-bit (Windows 10 Education version 1809),Linux lenovo520 4.15.0-70-generic #79-Ubuntu SMP Tue Nov 12 10:36:11 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux- Subsystem:
fs,stream
What steps will reproduce the bug?
Run this code with node
const fs = require("fs");
const { spawnSync } = require("child_process");
const pipeline = require("util").promisify(require("stream").pipeline);
async function main() {
const src = fs.createReadStream("src.exe");
const dest = fs.createWriteStream("dest.exe", { mode: 0o755 });
await pipeline(src, dest);
dest.destroy();
console.dir(spawnSync("dest.exe", ["--version"]));
}
main().finally(() => console.log("DONE"));How often does it reproduce? Is there a required condition?
It doesn't reproduce on all platforms stably. On my laptop it does always reproduce on Windows 10. I am not sure what is the required condition for this...
What is the expected behavior?
As per documentation of .destroy():
This is a destructive and immediate way to destroy a stream
I expect write-stream to release the file handle during the call to .destroy(), so that the next call to spawnSync of the copied file does execute the binary successfully.
What do you see instead?
Windows 10 x64
D:\junk\catch>node index.js
{
error: Error: spawnSync dest.exe EBUSY
at Object.spawnSync (internal/child_process.js:1041:20)
at spawnSync (child_process.js:609:24)
at main (D:\junk\catch\index.js:12:17)
at processTicksAndRejections (internal/process/task_queues.js:85:5) {
errno: 'EBUSY',
code: 'EBUSY',
syscall: 'spawnSync dest.exe',
path: 'dest.exe',
spawnargs: [ '--version' ]
},
status: null,
signal: null,
output: null,
pid: 0,
stdout: null,
stderr: null
}
DONE
D:\junk\catch>
Ubuntu 18.04.3
~/my/junk/stream $ node index.js
{
error: Error: spawnSync ./dest.exe EACCES
at Object.spawnSync (internal/child_process.js:1045:20)
at spawnSync (child_process.js:597:24)
at main (/home/veetaha/my/junk/stream/index.js:12:17)
at processTicksAndRejections (internal/process/task_queues.js:97:5) {
errno: -13,
code: 'EACCES',
syscall: 'spawnSync ./dest.exe',
path: './dest.exe',
spawnargs: [ '--version' ]
},
status: null,
signal: null,
output: null,
pid: 6261,
stdout: null,
stderr: null
}
DONE
~/my/junk/stream $
Additional information
The original problem was when downloading a binary executable from GitHub releases for rust-analyzer. You can see the initial discussion on the bug here.
The workaround for this that we use now is to wait for "close" event on writable-stream after the call to .destroy(), i.e.
await stream.pipeline(src, dest);
return new Promise(resolve => {
dest.on("close", resolve);
dest.destroy();
});
Activity
addaleax commentedon Feb 13, 2020
@nodejs/streams
Veetaha commentedon Feb 13, 2020
@addaleax, the link you provided doesn't work...
mcollina commentedon Feb 14, 2020
I think this is a bug in
fs, not on pipeline or streams. Essentially you should not be caling.destroy()manually or wait for'close'. I think we should changenode/lib/internal/fs/streams.js
Lines 368 to 372 in 0875837
to
Essentially we are not waiting to close the handle before emitting
'finish', and as a resultpipelinethinks the stream is done.mcollina commentedon Feb 14, 2020
cc @ronag
ronag commentedon Feb 14, 2020
@mcollina Yea, I think you are right.
This is probably fixed through #29656. I can add an additional test.
Would you consider this a bug worth back-porting? i.e. should I prepare a PR narrowed to this specific issue?
Veetaha commentedon Feb 14, 2020
@mcollina
Sorry, not perfect in English, did you mean we should neither call
.destroy()norwait for "close"?The documentation says:
I guess during the call to
.pipeline(src, dest),destdoesn't emit neither"finish"nor"close"So, please-please correct me if I am wrong. The following code is expected to work properly, right?
mcollina commentedon Feb 14, 2020
I’m saying that you have found a bug in node core ;). The stream is emitting ‘finish’ before the handle is destroyed, and it shouldn’t.
Veetaha commentedon Feb 14, 2020
@mcollina, sure, I am glad that we spotted a bug!
Though, I am just a bit concerned about our use-case.
Maybe this is not the right place to put such questions, but should we manually call
.destroy()on write-stream afterpipeline(src, dst)anyway?I just want to understand, assuming the bug is fixed, what the correct code would look like for our current workaround?
ronag commentedon Feb 14, 2020
You would not need to call destroy.
Veetaha commentedon Feb 16, 2020
@ronag, will this fix be backported to
nodejs 12.17.0or12.16.1?ronag commentedon Feb 16, 2020
@Veetaha I think it might be backported to a 12.x release. I'm not too familiar with the backport process yet. @targos might have better input. I've labeled the PR
lts-watch-v12.xfs: fix WriteStream autoClose order
fs: fix WriteStream autoClose order