node icon indicating copy to clipboard operation
node copied to clipboard

Closing filehandle after being used in a stream exits node without error message

Open mshopf opened this issue 1 year ago • 3 comments

Version

20.3.0

Platform

Linux zuse1.efi.fh-nuernberg.de 5.4.17-2136.319.1.4.el8uek.x86_64 #2 SMP Mon Jun 5 13:39:13 PDT 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

streams

What steps will reproduce the bug?

I use a stream for writing into a filehandle (of a regular file). After closing the stream (not the filehandle!) I write to other positions in the file. When I now close the file, node simply exists with return code 0. Following code is NOT executed.

const fs_p     = require('node:fs').promises;
const fs       = require('node:fs');
const stream_p = require('node:stream').promises;

async function test() {
    // Create writeable filehandle
    var fh = await fs_p.open ('/tmp/ttt.txt', 'w', 0o644);

    // Create stream, use stream, end stream, wait for finish
    var st = fh.createWriteStream ({autoClose: false});
    st.on ('error', (e) => {throw e});
    // No need to test return value with this little data
    st.write ('this is a filehandle / stream test');
    st.end ();
    await stream_p.finished (st);

    // Continue using filehandle, write something a beginning of file
    await fh.write ("TEST", 0);

    // Close filehandle to flush data and reuse system ressources
    console.log ('pre-close');

    // It is supposed to work like that.
    // filehandle.close() returns a Promise that is fullfilled with undefined after closing.
    // However, node simply exits (return code 0). Following code is not executed
    await fh.close ();

    // Working alternative. But only fixes system ressources, not node ressources.
    // And it blocks.
    //   fs.closeSync (fh.fd);

    // Continue
    console.log ('post-close');
    // should throw error because fd is closed
    await fh.write ("FH", 5);
}
test();

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

Reproduces always. Tried on v16, v18, v20.

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

Output:

pre-close
post-close
node:internal/process/promises:[...]
[Error: EBADF: bad file descriptor, write] { [...]

Content of /tmp/ttt.txt:

TEST is a filehandle / stream test

Works correctly with fs.closeSync (fh.fd); enabled instead of await fh.close(); Also works as expected, if the whole stream related block is commented out (contents of ttt.txt will differ).

What do you see instead?

Output:

pre-close

Content of /tmp/ttt.txt:

TEST is a filehandle / stream test

Additional information

No response

mshopf avatar Jun 15 '23 10:06 mshopf

The issue is we "reffing" the file handle here: https://github.com/nodejs/node/blob/9f192530f200b545aba7c34399d015aab8eb1804/lib/internal/fs/streams.js#L139

And we are never "unreffing" it until stream.close is called: https://github.com/nodejs/node/blob/9f192530f200b545aba7c34399d015aab8eb1804/lib/internal/fs/streams.js#L89

In your case, I think you expect stream.end() to unref file handle. If we can do that in a non-breaking way, that'd be great I think. PR's welcome :)

aduh95 avatar Jun 15 '23 16:06 aduh95

Sounds reasonable. Unfortunately, I'm not fluent in the node base classes codebase... And the potential side effects of such a change. If I'd do a patch, it would probably be a breaking one ;-)

mshopf avatar Jun 16 '23 11:06 mshopf

As far as I read the code, end() basically finishes a stream. So should the unref() happen when we receive the "finish" event? Probably not before, because data could still to be written...

mshopf avatar Jun 16 '23 11:06 mshopf

Is there a workaround for this? I'm trying to do a similar pattern here so that I can ensure filehandle.sync() is called before close(), but as seen here, node20 is just exiting 0 silently after the attempt to close.

With this added in node21 (https://github.com/nodejs/node/issues/49886), I see a path for safe fs stream IO, but I don't really understand how reliable FS stream writes are possible in node20 with this unresolved bug.

colemickens avatar Oct 24 '23 10:10 colemickens