writableStream.destroy() closes the underlying file even when stream is created with {autoClose: false, emitClose: false}

[notorca]

Version

v18.17.1, v20.5.1

Platform

Linux 6.4.10-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Aug 11 12:20:29 UTC 2023 x86_64 GNU/Linux

Subsystem

fs, stream

What steps will reproduce the bug?

Minimal program to reproduce.

Create 2 writeStreams from a FileHandle with {autoClose: false, emitClose: false} options. Write some data to the first stream and destroy it. Try to write some data to the second stream.

import { open } from 'node:fs/promises'
import { pipeline } from 'node:stream/promises'

async function main() {
 const destFile = await open('test.tmp', 'w')
 destFile.truncate(1024)
 const stream0 = destFile.createWriteStream({start: 0, autoClose: false, emitClose: false})
 const stream1 = destFile.createWriteStream({start: 512, autoClose: false, emitClose: false})
 stream0.on('error', (err) => { console.log('stream0 error', err) })
 await pipeline("Hello world from stream0", stream0)
 stream0.destroy(new Error('destroyed'))
 try {
 await pipeline("Hello world from stream1", stream1) // emits ERR_STREAM_WRITE_AFTER_END
 } finally {
 // destFile.fd is -1 here
 destFile.close()
 }
}

await main()

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

Data is written, FileHandle is not closed up until close() call because emitClose: true is specified on stream creation.

What do you see instead?

ERR_STREAM_WRITE_AFTER_END is emmited when trying to write to the steream1. destFile.fd is set to -1.

Additional information

No response

[bnoordhuis]

Possible duplicate of #45721? Can you check with strace -y if the file descriptor is actually closed?

[benjamingr]

You're destroying the stream this is expected behavior, it's not closed on its own you're explicitly destroying the stream...

[notorca]

@benjamingr According to the docs:

By default, the stream will emit a 'close' event after it has been destroyed. Set the emitClose option to false to change this behavior.

[benjamingr]

@notorca right, but the issue you're seeing isn't the "close" event being emitted it's that you're trying to reuse the file handle after destroying the streama

[benjamingr]

In fact for compatibility reasons for most of Node's history emitClose was set to false on fs streams, it was changed by ronag here: #31408

[notorca]

@benjamingr It's still not clear for me why destroying a stream closes the underlying FileHandle. I suppose it should be controlled by emitClose, FileHandle and streams related as 1:N.

[benjamingr]

As soon as you create a write stream from a file handle, you're tying that file to the stream and if you close one the other is closed - so the relationship is not in fact 1:N.

You misunderstand emitClose it has nothing to do with actually closing stuff - it's only about whether or not the close event should be emitted.

We have discussed having a "unref this stream so destroying it is a no-op" a bunch for various use cases (the one Ben linked to and a few others, see context in #48007 cc @rluvaton ) but we haven't been come up with an implementation that doesn't leak.

[notorca]

@benjamingr As I can see emitClose is actually related to closing stuff. Here is a sample code showing that file handles and streams are actually 1:N (it's the same as in the original bug posted, but with close instead of destroy):

import { open } from 'node:fs/promises'
import { pipeline } from 'node:stream/promises'

async function main() {
 const destFile = await open('test1.tmp', 'w')
 destFile.truncate(1024)
 const stream0 = destFile.createWriteStream({start: 0, autoClose: false, emitClose: false})
 const stream1 = destFile.createWriteStream({start: 512, autoClose: false, emitClose: false})
 stream0.on('error', (err) => { console.log('stream0 error', err) })
 await pipeline("Hello world from stream0", stream0)
 stream0.close()
 try {
 await pipeline("Hello world from stream1", stream1)
 } finally {
 destFile.close()
 }
}

await main()

[benjamingr]

If "Hello world from stream1" exists in the file that's probably a bug we should fix (assuming it's not timing)

[notorca]

@bnoordhuis Seems related to #45721. According to strace file descriptor is leaked in this case. So destroying the stream just sets fd to -1 in FileHandle without closing it.

[notorca]

@benjamingr "Hello world from stream1" exists in the file. If you want to enforce 1:1 semantics (IMHO that's unreasonable restriction) it would be better to throw an error on attempt to create a second stream from the same FileHandle.

[benjamingr]

@ronag am I wrong? Do we support 1:N fs.WriteStreams to FileHandles intentionally? If we do why does destroy act differently from end/close? (and why isn't .close on fs streams deprecated? Is there an actual reason to use it and not .end?)

[ronag]

I agree the current behavior with FileHandle is weird and I tried to fix it in #47484

IMO

• .close should be deprecated.
• FileHandle should expose ref/unref
• fs streams should unref FileHandle not close it.

[benjamingr]

All that sounds good to me, @rluvaton anything of those you want to work on?