Writable stream is not immediatly closed after .destroy() call

[Veetaha]

• 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();
});

[addaleax]

@nodejs/streams

[Veetaha]

@addaleax, the link you provided doesn't work...

[mcollina]

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 change

node/lib/internal/fs/streams.js

Lines 368 to 372 in 0875837

	if (this.autoClose) {
	this.destroy();
	}
	callback();

to

  if (this.autoClose) {
    this.destroy(callback);
  } else {
    callback();
  }

Essentially we are not waiting to close the handle before emitting 'finish', and as a result pipeline thinks the stream is done.

[mcollina]

cc @ronag

[ronag]

@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]

@mcollina

Essentially you should not be caling .destroy() manually or wait for 'close'

Sorry, not perfect in English, did you mean we should neither call .destroy() nor wait for "close"?

The documentation says:

stream.pipeline() will call stream.destroy(err) on all streams except:

Readable streams which have emitted 'end' or 'close'.
Writable streams which have emitted 'finish' or 'close'.

I guess during the call to .pipeline(src, dest), dest doesn't emit neither "finish" nor "close"
So, please-please correct me if I am wrong. The following code is expected to work properly, right?

await pipeline(src, dest);
// Notice, no call to .destroy(), not waiting for "close" here, we go use the copied file right away
spawnSync("dest.exe", ["--version"]));

[mcollina]

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]

@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 after pipeline(src, dst) anyway?
I just want to understand, assuming the bug is fixed, what the correct code would look like for our current workaround?

await stream.pipeline(src, dest);
return new Promise(resolve => {
    dest.on("close", resolve); // resolve only when "close" is emited
    dest.destroy();
});

[ronag]

Maybe this is not the right place to put such questions, but should we manually call .destroy() on write-stream after pipeline(src, dst) anyway?

You would not need to call destroy.

I just want to understand, assuming the bug is fixed, what the correct code would look like for our current workaround?

await stream.pipeline(src, dest);

[Veetaha]

@ronag, will this fix be backported to nodejs 12.17.0 or 12.16.1 ?

[ronag]

@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.x