Skip to content

Commit fe93fc3

Browse files
committed
fs: do not close FileHandle on stream destroy with autoClose: false
When a ReadStream or WriteStream is created from a FileHandle, the stream takes a reference on the handle. Destroying the stream (for example by breaking out of a `for await` loop) dropped that reference and then unconditionally called `filehandle.close()`, which also dropped the caller's reference and closed the handle, even when the stream was created with `autoClose: false`. Only release the stream's own reference when `autoClose` is false, so that the FileHandle stays usable and remains owned by the caller, as documented. Also remove the stream's 'close' listener from the FileHandle when the stream closes, so that creating many streams from a long-lived FileHandle does not leak listeners and streams. Fixes: nodejs#45721 Refs: nodejs#45909 Signed-off-by: Sahinur <[email protected]>
1 parent 2192f7f commit fe93fc3

3 files changed

Lines changed: 113 additions & 10 deletions

File tree

‎doc/api/fs.md‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -304,10 +304,10 @@ setTimeout(() => {
304304
```
305305
306306
If `autoClose` is false, then the file descriptor won't be closed, even if
307-
there's an error. It is the application's responsibility to close it and make
308-
sure there's no file descriptor leak. If `autoClose` is set to true (default
309-
behavior), on `'error'` or `'end'` the file descriptor will be closed
310-
automatically.
307+
there's an error or the stream is destroyed. It is the application's
308+
responsibility to close it and make sure there's no file descriptor leak. If
309+
`autoClose` is set to true (default behavior), on `'error'` or `'end'` the file
310+
descriptor will be closed automatically.
311311
312312
An example to read the last 10 bytes of a file which is 100 bytes long:
313313
@@ -352,9 +352,9 @@ the default `r`. The `encoding` can be any one of those accepted by {Buffer}.
352352
353353
If `autoClose` is set to true (default behavior) on `'error'` or `'finish'`
354354
the file descriptor will be closed automatically. If `autoClose` is false,
355-
then the file descriptor won't be closed, even if there's an error.
356-
It is the application's responsibility to close it and make sure there's no
357-
file descriptor leak.
355+
then the file descriptor won't be closed, even if there's an error or the
356+
stream is destroyed. It is the application's responsibility to close it and
357+
make sure there's no file descriptor leak.
358358
359359
By default, the stream will emit a `'close'` event after it has been
360360
destroyed. Set the `emitClose` option to `false` to change this behavior.

‎lib/internal/fs/streams.js‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,13 +90,23 @@ function _construct(callback) {
9090
}
9191

9292
// This generates an fs operations structure for a FileHandle
93-
const FileHandleOperations = (handle) => {
93+
const FileHandleOperations = (stream, handle, onHandleClose) => {
9494
return {
9595
open: (path, flags, mode, cb) => {
9696
throw new ERR_METHOD_NOT_IMPLEMENTED('open()');
9797
},
9898
close: (fd, cb) => {
99+
// The stream no longer needs to react to the FileHandle closing, and
100+
// must not be kept alive by a long-lived FileHandle.
101+
handle.off('close', onHandleClose);
102+
// Release the reference taken in importFd().
99103
handle[kUnref]();
104+
if (!stream.autoClose) {
105+
// With autoClose: false the FileHandle is owned by the caller, so
106+
// destroying the stream must not close it.
107+
process.nextTick(cb);
108+
return;
109+
}
100110
PromisePrototypeThen(handle.close(),
101111
() => cb(), cb);
102112
},
@@ -155,10 +165,11 @@ function importFd(stream, options) {
155165
// FileHandle is not supported with custom fs operations
156166
throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs');
157167
}
168+
const onHandleClose = FunctionPrototypeBind(stream.close, stream);
158169
stream[kHandle] = options.fd;
159-
stream[kFs] = FileHandleOperations(stream[kHandle]);
170+
stream[kFs] = FileHandleOperations(stream, stream[kHandle], onHandleClose);
160171
stream[kHandle][kRef]();
161-
options.fd.on('close', FunctionPrototypeBind(stream.close, stream));
172+
options.fd.on('close', onHandleClose);
162173
return options.fd.fd;
163174
}
164175

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
'use strict';
2+
3+
// Regression test for https://github.com/nodejs/node/issues/45721
4+
// Destroying a stream created from a FileHandle with `autoClose: false` must
5+
// not close the FileHandle, and must not leave a 'close' listener behind on
6+
// the FileHandle.
7+
8+
const common = require('../common');
9+
const assert = require('assert');
10+
const fs = require('fs');
11+
const tmpdir = require('../common/tmpdir');
12+
13+
tmpdir.refresh();
14+
const file = tmpdir.resolve('stream-file-handle-autoclose-false.txt');
15+
const input = 'x'.repeat(1024);
16+
fs.writeFileSync(file, input);
17+
18+
(async () => {
19+
// Partially consuming a ReadStream with `break` destroys it.
20+
const handle = await fs.promises.open(file, 'r');
21+
handle.on('close', common.mustCall());
22+
const listeners = handle.listenerCount('close');
23+
24+
// Run more iterations than the default maxListeners (10) to make sure
25+
// no listener leaks onto the long-lived FileHandle.
26+
process.on('warning', common.mustNotCall());
27+
for (let i = 0; i < 15; i++) {
28+
const stream = handle.createReadStream({
29+
autoClose: false,
30+
start: 0,
31+
highWaterMark: 16,
32+
});
33+
for await (const chunk of stream) {
34+
assert.strictEqual(chunk.toString(), input.slice(0, 16));
35+
break;
36+
}
37+
assert.strictEqual(stream.destroyed, true);
38+
// The stream is only closed once any pending read has completed.
39+
if (!stream.closed) {
40+
await new Promise((resolve) => stream.once('close', resolve));
41+
}
42+
assert.notStrictEqual(handle.fd, -1);
43+
assert.strictEqual(handle.listenerCount('close'), listeners);
44+
}
45+
46+
// Explicitly destroying the stream.
47+
const stream = handle.createReadStream({ autoClose: false, start: 0 });
48+
stream.destroy();
49+
await new Promise((resolve) => stream.on('close', resolve));
50+
assert.notStrictEqual(handle.fd, -1);
51+
52+
// The FileHandle is still usable.
53+
const { bytesRead } = await handle.read(Buffer.alloc(4), 0, 4, 0);
54+
assert.strictEqual(bytesRead, 4);
55+
await handle.close();
56+
assert.strictEqual(handle.fd, -1);
57+
})().then(common.mustCall());
58+
59+
(async () => {
60+
// Same for WriteStream.
61+
const handle = await fs.promises.open(tmpdir.resolve('ws.txt'), 'w');
62+
handle.on('close', common.mustCall());
63+
const stream = handle.createWriteStream({ autoClose: false });
64+
stream.write('hello');
65+
stream.destroy();
66+
await new Promise((resolve) => stream.on('close', resolve));
67+
assert.notStrictEqual(handle.fd, -1);
68+
assert.strictEqual(handle.listenerCount('close'), 1);
69+
await handle.write('world');
70+
await handle.close();
71+
})().then(common.mustCall());
72+
73+
(async () => {
74+
// Closing the FileHandle while a stream with `autoClose: false` holds a
75+
// reference to it still closes the stream and the FileHandle.
76+
const handle = await fs.promises.open(file, 'r');
77+
const stream = handle.createReadStream({ autoClose: false });
78+
stream.on('close', common.mustCall());
79+
await handle.close();
80+
assert.strictEqual(handle.fd, -1);
81+
})().then(common.mustCall());
82+
83+
(async () => {
84+
// `autoClose: true` (the default) still closes the FileHandle on destroy.
85+
const handle = await fs.promises.open(file, 'r');
86+
handle.on('close', common.mustCall());
87+
const stream = handle.createReadStream();
88+
stream.destroy();
89+
await new Promise((resolve) => stream.on('close', resolve));
90+
assert.strictEqual(handle.fd, -1);
91+
assert.strictEqual(handle.listenerCount('close'), 1);
92+
})().then(common.mustCall());

0 commit comments

Comments
 (0)