Partially consuming fs.ReadStream closes file handle despite autoClose: false

[wolfgang42]

Version

v19.2.0, v14.21.1

Platform

Linux wolf-x1c6 5.15.0-53-generic #59-Ubuntu SMP Mon Oct 17 18:53:30 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

import fsPromises from 'node:fs/promises'

const fh = await fsPromises.open('example.txt')
fh.on('close', () => {throw new Error('handle closed!')})

const stream = fh.createReadStream({
	autoClose: false,
})

for await (const chunk of stream) {
	break
}

// Give the event loop a breather: this seems to be where the close happens
await new Promise(resolve => setTimeout(resolve, 10))

console.log('fd', fh.fd) // = -1; stream closed

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

Always

What is the expected behavior?

FileHandle.createReadStream({autoClose: false}) should result in the file handle not being automatically closed when the stream is destroyed.

What do you see instead?

Using fs.ReadStream[Symbol.asyncIterator] seems to unconditionally close the file handle (after a small delay; I guess at the end of the event loop or something? but haven't dug in; just noted the setTimeout above is necessary to reproduce) despite {autoClose: false} being passed to FileHandle.createReadStream().

This seems to be the result of the underlying readable.destroy() also closing the file handle; but it means that, as far as I can tell, there's no way to only partially consume an fs.ReadStream created from a file handle without either closing the handle or leaking the stream.

Additional information

No response

[Slayer95]

break; has active semantics in a streaming context. As you have correctly pointed out, it calls .destroy().

I suggest you try stream composition and/or iterator-helpers, such as ReadableStream#take().

"use strict";

const fsPromises = require('node:fs/promises');

async function run() {
	const fh = await fsPromises.open('example.txt')
	fh.on('close', () => {throw new Error('handle closed!')})

	const stream = fh.createReadStream({
		autoClose: false,
	})

	for await (const chunk of stream.take(Infinity)) {
		break
	}

	// Give the event loop a breather: this seems to be where the close happens
	await new Promise(resolve => setTimeout(resolve, 10))

	console.log('fd', fh.fd) // PASSES
}

run();

[wolfgang42]

Yes, I understand that break causes a stream to be destroyed; that part is well documented in readable[Symbol.asyncIterator]() (and makes sense; it's reasonable not to have to do manual cleanup on the stream after interrupting iteration.)

The thing I find surprising is that fs.ReadStream.destroy() closes the underlying file handle when I explicitly request that the file descriptor should not be closed.

I was merely using the loop as an example of the context in which I discovered this problem. I am aware that there are other ways of interacting with streams. However, as I said in my original post:

as far as I can tell, there's no way to only partially consume an fs.ReadStream created from a file handle without either closing the handle or leaking the stream.

I don't think your example, as given, solves this problem: it doesn’t close the handle, but it still seems to leak the stream. (The easiest way to see this is to wrap lines 7 through 16 in a loop: this results in “MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [FileHandle].”.)

[Slayer95]

The docs also state that destroying the stream closes internal resources, which may only be understood as releasing the fd, and that's in fact all it does. According to my understanding, autoClose is only meant to avoid closing the fd when it ends and when it errors, but it wouldn't apply to stream destruction, either explicit or implicit (through break).

I am afraid I don't follow what you mean by "leaking the stream". As far as I can tell, looping over line 7 more than 10 times is enough to reproduce the warning. Could you please clarify what you mean? Maybe we can get to a simpler test case which gets more attention on the issue you face.

[Slayer95]

Okay, I think this is what you are looking for.

"use strict";

const fsPromises = require('node:fs/promises');
const {setTimeout} = require('node:timers/promises');

async function run() {
	const fh = await fsPromises.open('example.txt')
	fh.on('close', () => {throw new Error('handle closed!')})
	for (let i = 0; i < 10; i++) {
		const stream = fh.createReadStream({
			autoClose: false,
		})
		// Long-lived file handle. Streams needn't be kept around til it closes.
		fh.off('close', fh.rawListeners('close').at(-1));

		for await (const chunk of stream.iterator({destroyOnReturn: false})) {
			break
		}
	}

	await setTimeout(10);

	console.log('fd', fh.fd) // PASSES
}

run();

However, removing close listeners looks to me like an ugly hack. Surely some API is missing to handle this case. What do you think?

[wolfgang42]

I don’t pretend to fully understand Node’s internals here, but your code block does seem like a plausible workaround, though I agree it looks like a hack. Also worth noting that, for reasons I'm not entirely clear on, it seems to work fine even without {destroyOnReturn: false}, which makes me think there's something I’m not understanding about how this works. (Calling stream.destroy() explicitly does make things crash in the way I’d expect.)

I'v also commented on #45917 with some additional notes.