From 51623c430ec5d0eee5ddcc3b15da4c557d5440b4 Mon Sep 17 00:00:00 2001 From: Leonardo Ivo Julca Date: Tue, 20 Dec 2022 00:25:00 -0500 Subject: [PATCH 1/3] test: ensure ReadableStream is not leaked by FileHandle --- test/fixtures/stream-readable-leak.js | 47 +++++++++++++++++++ test/parallel/test-stream-readable-fh-hold.js | 21 +++++++++ 2 files changed, 68 insertions(+) create mode 100644 test/fixtures/stream-readable-leak.js create mode 100644 test/parallel/test-stream-readable-fh-hold.js diff --git a/test/fixtures/stream-readable-leak.js b/test/fixtures/stream-readable-leak.js new file mode 100644 index 00000000000000..5f303106365aa6 --- /dev/null +++ b/test/fixtures/stream-readable-leak.js @@ -0,0 +1,47 @@ +'use strict'; + +const { mustCall, mustNotCall } = require('../common'); +const { open } = require('node:fs/promises'); +const { setImmediate } = require('node:timers/promises'); + +function getHugeVector() { + return new Array(1e5).fill(0).map((e, i) => i); +} + +const registry = new FinalizationRegistry(() => { + process.exitCode = 0; + process.send(`success`); +}); + +async function run() { + process.exitCode = 1; + + // Long-lived file handle. Ensure we keep a reference to it. + const fh = await open(__filename) + fh.on('close', mustNotCall()) + + const stream = fh.createReadStream({ autoClose: false }) + // Keeping the file handle open shouldn't prevent the stream from being GCed. + registry.register(stream, `[GC] Collected readable ${fh.fd}`); + + for await (const chunk of stream.iterator({ destroyOnReturn: false })) { + break + } + + // Keep a reference to the file handle + return { fh }; +} + +async function forceGC() { + gc(); + const hugeMatrix = []; + for (let i = 0; i < 0x40; i++) { + hugeMatrix.push(getHugeVector()); + gc(); + await setImmediate(); + } +} + +run().then(async function (result) { + await forceGC(); +}); diff --git a/test/parallel/test-stream-readable-fh-hold.js b/test/parallel/test-stream-readable-fh-hold.js new file mode 100644 index 00000000000000..87fbc897408fa4 --- /dev/null +++ b/test/parallel/test-stream-readable-fh-hold.js @@ -0,0 +1,21 @@ +'use strict'; + +const { mustCall, mustNotCall } = require('../common'); +const { strictEqual, notStrictEqual } = require('assert'); +const fixtures = require('../common/fixtures'); +const { fork } = require('child_process'); + +{ + const cp = fork(fixtures.path('stream-readable-leak.js'), { + execArgv: ['--expose-gc', '--max-old-space-size=16', '--max-semi-space-size=2'], + silent: true, + }); + cp.on('exit', mustCall((code, killSignal) => { + notStrictEqual(code, 1); + strictEqual(killSignal, null); + })); + cp.on('error', mustNotCall()); + cp.on('message', mustCall(message => { + strictEqual(message, `success`); + })); +} From c61247c9d3d113ab2636e2d1a45d2b8a4226e3ae Mon Sep 17 00:00:00 2001 From: Leonardo Ivo Julca Date: Tue, 20 Dec 2022 00:26:09 -0500 Subject: [PATCH 2/3] fs: allow FileHandle to outlive ReadableStream Refs: https://github.com/nodejs/node/issues/45721 --- lib/internal/fs/streams.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index c5bc567a8409bf..856e8cd7600baf 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -119,6 +119,13 @@ function close(stream, err, cb) { } } +function closeWeakRef(ref) { + const stream = ref.deref(); + if (stream) { + stream.close(); + } +} + function importFd(stream, options) { if (typeof options.fd === 'number') { // When fd is a raw descriptor, we must keep our fingers crossed @@ -137,7 +144,7 @@ function importFd(stream, options) { stream[kHandle] = options.fd; stream[kFs] = FileHandleOperations(stream[kHandle]); stream[kHandle][kRef](); - options.fd.on('close', FunctionPrototypeBind(stream.close, stream)); + stream[kHandle].on('close', FunctionPrototypeBind(closeWeakRef, null, new WeakRef(stream))); return options.fd.fd; } From f358742af6853d38f75820c54a58eba9f24d383e Mon Sep 17 00:00:00 2001 From: Leonardo Ivo Julca Date: Tue, 20 Dec 2022 00:55:44 -0500 Subject: [PATCH 3/3] style fixup --- test/fixtures/stream-readable-leak.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/fixtures/stream-readable-leak.js b/test/fixtures/stream-readable-leak.js index 5f303106365aa6..fc2f7f9c958242 100644 --- a/test/fixtures/stream-readable-leak.js +++ b/test/fixtures/stream-readable-leak.js @@ -17,15 +17,15 @@ async function run() { process.exitCode = 1; // Long-lived file handle. Ensure we keep a reference to it. - const fh = await open(__filename) - fh.on('close', mustNotCall()) + const fh = await open(__filename); + fh.on('close', mustNotCall()); - const stream = fh.createReadStream({ autoClose: false }) + const stream = fh.createReadStream({ autoClose: false }); // Keeping the file handle open shouldn't prevent the stream from being GCed. registry.register(stream, `[GC] Collected readable ${fh.fd}`); for await (const chunk of stream.iterator({ destroyOnReturn: false })) { - break + break; } // Keep a reference to the file handle