fs: add autoClose option to FileHandle readableWebStream #58548

jasnell · 2025-06-01T15:50:09Z

Separated out from #58536

By default, the readableWebStream method of FileHandle returns a ReadableStream that, when finished, does not close the underlying FileHandle. This can lead to issues if the stream is consumed without having a reference to the FileHandle to close after use. This commit adds an autoClose option to the readableWebStream method, which, when set to true, will automatically close the FileHandle when the stream is finished or canceled.

The test modified in this commit demonstrates one of the cases where this is necessary in that the stream is consumed by separate code than the FileHandle which was being left to close the underlying fd when it is garbage collected, which is a deprecated behavior.

LiviaMedeiros

Thanks! Being able to return new Response(filehandle.readableWebStream()); without leaking the handle is great, I hope we can make this enabled by default in next major releases.

LGTM with a few nits.

doc/api/fs.md

lib/internal/fs/promises.js

BridgeAR

The change is LGTM. Should we test that the stream is indeed closed? I could not see that in the test where the option is now used.

lib/internal/fs/promises.js

jasnell · 2025-06-02T18:22:45Z

Should we test that the stream is indeed closed? I could not see that in the test where the option is now used.

I'm not entirely sure how to test that yet in this case but will look at it. The reason this is needed for this particular test is that I tried to make the "FileHandle closed on GC" warning an error and this test hit that. With the autoClose option that works and the corresponding test passes. Given that I think we will likely have sufficient test coverage. Either way, I think we can likely add a test separately if we are concerned that the streams close path may not be followed correctly.

LiviaMedeiros · 2025-06-02T18:53:28Z

I'm not entirely sure how to test that yet in this case but will look at it.

I think something like this would be sufficient

import { open } from 'node:fs/promises';
import { rejects } from 'node:assert';

{
  const fh = await open(new URL(import.meta.url));

  // TODO: remove autoClose option when it becomes default
  const readableStream = fh.readableWebStream({ autoClose: true });

  // Consume the stream
  await new Response(readableStream).text();

  await rejects(fh.read(), { code: 'EBADF' });
}

{
  await using fh = await open(new URL(import.meta.url)); // or the usual let-try-finally-close if we want this backportable...

  const readableStream = fh.readableWebStream({ autoClose: false });

  // Consume the stream
  await new Response(readableStream).text();

 // Filehandle must be still open
  await fh.read();
}

By default, the `readableWebStream` method of `FileHandle` returns a ReadableStream that, when finished, does not close the underlying FileHandle. This can lead to issues if the stream is consumed without having a reference to the FileHandle to close after use. This commit adds an `autoClose` option to the `readableWebStream` method, which, when set to `true`, will automatically close the FileHandle when the stream is finished or canceled. The test modified in this commit demonstrates one of the cases where this is necessary in that the stream is consumed by separate code than the FileHandle which was being left to close the underlying fd when it is garbage collected, which is a deprecated behavior.

nodejs-github-bot · 2025-06-02T19:42:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/67260/

By default, the `readableWebStream` method of `FileHandle` returns a ReadableStream that, when finished, does not close the underlying FileHandle. This can lead to issues if the stream is consumed without having a reference to the FileHandle to close after use. This commit adds an `autoClose` option to the `readableWebStream` method, which, when set to `true`, will automatically close the FileHandle when the stream is finished or canceled. The test modified in this commit demonstrates one of the cases where this is necessary in that the stream is consumed by separate code than the FileHandle which was being left to close the underlying fd when it is garbage collected, which is a deprecated behavior. PR-URL: #58548 Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jasnell · 2025-06-05T01:27:10Z

Landed in 3f81645

By default, the `readableWebStream` method of `FileHandle` returns a ReadableStream that, when finished, does not close the underlying FileHandle. This can lead to issues if the stream is consumed without having a reference to the FileHandle to close after use. This commit adds an `autoClose` option to the `readableWebStream` method, which, when set to `true`, will automatically close the FileHandle when the stream is finished or canceled. The test modified in this commit demonstrates one of the cases where this is necessary in that the stream is consumed by separate code than the FileHandle which was being left to close the underlying fd when it is garbage collected, which is a deprecated behavior. PR-URL: #58548 Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Notable changes: doc: * deprecate utilisNativeError in favor of ErrorisError (Miguel Marcondes Filho) #58262 * deprecate passing an empty string to `options.shell` (Antoine du Hamel) #58564 * deprecate HTTP/2 priority signaling (Matteo Collina) #58313 * (SEMVER-MINOR) graduate Symbol.dispose/asyncDispose from experimental (James M Snell) #58467 esm: * (SEMVER-MINOR) implement import.meta.main (Joe) #57804 fs: * (SEMVER-MINOR) add autoClose option to FileHandle readableWebStream (James M Snell) #58548 http: * deprecate instantiating classes without new (Yagiz Nizipli) #58518 http2: * (SEMVER-MINOR) add diagnostics channel 'http2.server.stream.finish' (Darshan Sen) #58560 lib: * (SEMVER-MINOR) graduate error codes that have been around for years (James M Snell) #58541 perf_hooks: * (SEMVER-MINOR) make event loop delay histogram disposable (James M Snell) #58384 src: * (SEMVER-MINOR) support namespace options in configuration file (Pietro Marchini) #58073 test: * (SEMVER-MINOR) add disposable histogram test (James M Snell) #58384 * (SEMVER-MINOR) add test for async disposable worker thread (James M Snell) #58385 util: * (SEMVER-MINOR) add 'none' style to styleText (James M Snell) #58437 worker: * (SEMVER-MINOR) make Worker async disposable (James M Snell) #58385 PR-URL: #58635

Notable changes: doc: * deprecate `util.isNativeError` in favor of `Error.isError` (Miguel Marcondes Filho) #58262 * deprecate passing an empty string to `options.shell` (Antoine du Hamel) #58564 * deprecate HTTP/2 priority signaling (Matteo Collina) #58313 * (SEMVER-MINOR) graduate `Symbol.dispose`/`asyncDispose` from experimental (James M Snell) #58467 esm: * (SEMVER-MINOR) implement import.meta.main (Joe) #57804 fs: * (SEMVER-MINOR) add `autoClose` option to `FileHandle` `readableWebStream` (James M Snell) #58548 http: * deprecate instantiating classes without new (Yagiz Nizipli) #58518 http2: * (SEMVER-MINOR) add diagnostics channel 'http2.server.stream.finish' (Darshan Sen) #58560 * (SEMVER-MAJOR) remove support for priority signaling (Matteo Collina) #58293 lib: * (SEMVER-MINOR) graduate error codes that have been around for years (James M Snell) #58541 perf_hooks: * (SEMVER-MINOR) make event loop delay histogram disposable (James M Snell) #58384 src: * (SEMVER-MINOR) support namespace options in configuration file (Pietro Marchini) #58073 test: * (SEMVER-MINOR) add disposable histogram test (James M Snell) #58384 * (SEMVER-MINOR) add test for async disposable worker thread (James M Snell) #58385 util: * (SEMVER-MINOR) add 'none' style to styleText (James M Snell) #58437 worker: * (SEMVER-MINOR) make Worker async disposable (James M Snell) #58385 PR-URL: #58635

Notable changes: doc: * add Filip Skokan to TSC (Rafael Gonzaga) #58499 * deprecate `util.isNativeError` in favor of `Error.isError` (Miguel Marcondes Filho) #58262 * deprecate passing an empty string to `options.shell` (Antoine du Hamel) #58564 * deprecate HTTP/2 priority signaling (Matteo Collina) #58313 * (SEMVER-MINOR) graduate `Symbol.dispose`/`asyncDispose` from experimental (James M Snell) #58467 esm: * (SEMVER-MINOR) implement import.meta.main (Joe) #57804 fs: * (SEMVER-MINOR) add `autoClose` option to `FileHandle` `readableWebStream` (James M Snell) #58548 http: * deprecate instantiating classes without new (Yagiz Nizipli) #58518 http2: * (SEMVER-MINOR) add diagnostics channel 'http2.server.stream.finish' (Darshan Sen) #58560 * (SEMVER-MAJOR) remove support for priority signaling (Matteo Collina) #58293 lib: * (SEMVER-MINOR) graduate error codes that have been around for years (James M Snell) #58541 perf_hooks: * (SEMVER-MINOR) make event loop delay histogram disposable (James M Snell) #58384 src: * (SEMVER-MINOR) support namespace options in configuration file (Pietro Marchini) #58073 permission: * implicit allow-fs-read to app entrypoint (Rafael Gonzaga) #58579 test: * (SEMVER-MINOR) add disposable histogram test (James M Snell) #58384 * (SEMVER-MINOR) add test for async disposable worker thread (James M Snell) #58385 util: * (SEMVER-MINOR) add 'none' style to styleText (James M Snell) #58437 worker: * (SEMVER-MINOR) make Worker async disposable (James M Snell) #58385 PR-URL: #58635

jasnell requested review from mcollina, anonrig and LiviaMedeiros June 1, 2025 15:50

jasnell added fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 1, 2025

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 1, 2025

This comment was marked as outdated.

Sign in to view

LiviaMedeiros approved these changes Jun 1, 2025

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

doc/api/fs.md Outdated Show resolved Hide resolved

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

benjamingr approved these changes Jun 1, 2025

View reviewed changes

BridgeAR reviewed Jun 2, 2025

View reviewed changes

jasnell commented Jun 2, 2025

View reviewed changes

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

LiviaMedeiros approved these changes Jun 2, 2025

View reviewed changes

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 2, 2025

jasnell force-pushed the jasnell/filehandle-readablewebstream-autoclose branch from a90acc4 to e49cce2 Compare June 2, 2025 18:26

jasnell requested a review from LiviaMedeiros June 2, 2025 18:26

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 2, 2025

This comment was marked as resolved.

Sign in to view

LiviaMedeiros approved these changes Jun 2, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

LiviaMedeiros removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Jun 2, 2025

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 2, 2025

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the jasnell/filehandle-readablewebstream-autoclose branch from e49cce2 to 777d231 Compare June 2, 2025 19:36

jasnell added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 5, 2025

jasnell closed this Jun 5, 2025

github-actions bot mentioned this pull request Jun 8, 2025

2025-06-09, Version 24.2.0 (Current) #58635

Merged

May	JUN	Jul
	10
2024	2025	2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fs: add autoClose option to FileHandle readableWebStream #58548

fs: add autoClose option to FileHandle readableWebStream #58548

Uh oh!

jasnell commented Jun 1, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

LiviaMedeiros left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Uh oh!

Uh oh!

jasnell commented Jun 2, 2025 •

edited

Loading

Uh oh!

This comment was marked as resolved.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as outdated.

LiviaMedeiros commented Jun 2, 2025 •

edited

Loading

Uh oh!

nodejs-github-bot commented Jun 2, 2025

Uh oh!

jasnell commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

fs: add autoClose option to FileHandle readableWebStream #58548

fs: add autoClose option to FileHandle readableWebStream #58548

Uh oh!

Conversation

jasnell commented Jun 1, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

LiviaMedeiros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasnell commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as outdated.

LiviaMedeiros commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 2, 2025

Uh oh!

jasnell commented Jun 5, 2025

Uh oh!

Uh oh!

jasnell commented Jun 2, 2025 •

edited

Loading

LiviaMedeiros commented Jun 2, 2025 •

edited

Loading