node icon indicating copy to clipboard operation
node copied to clipboard

fs: move FileHandle close on GC to EOL

Open jasnell opened this issue 6 months ago • 6 comments

Automatically closing a FileHandle on garbage collection has been deprecated for some time now. We've said that it will eventually be removed and become and error.

~~This PR also adds a new autoClose option to the FileHandle readableWebStream() API in order to deal with a possible bug in which the readable stream that wraps the FileHandle is passed out of a context where the FileHandle is accessible to be closed.~~ This was extracted and landed in https://github.com/nodejs/node/pull/58548

jasnell avatar May 31 '25 19:05 jasnell

Codecov Report

Attention: Patch coverage is 40.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.10%. Comparing base (ce546e4) to head (37282ce). Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 40.00% 4 Missing and 2 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58536      +/-   ##
==========================================
- Coverage   90.13%   90.10%   -0.04%     
==========================================
  Files         640      640              
  Lines      188294   188282      -12     
  Branches    36930    36924       -6     
==========================================
- Hits       169721   169651      -70     
- Misses      11290    11353      +63     
+ Partials     7283     7278       -5     
Files with missing lines Coverage Δ
src/env-inl.h 94.20% <ø> (-0.07%) :arrow_down:
src/env.h 98.14% <ø> (ø)
src/node_file.cc 75.79% <40.00%> (-0.07%) :arrow_down:

... and 33 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar May 31 '25 20:05 codecov[bot]

This PR also adds a new autoClose option to the FileHandle readableWebStream() API in order to deal with a possible bug in which the readable stream that wraps the FileHandle is passed out of a context where the FileHandle is accessible to be closed.

Unless there already is a trivial userspace solution to this or autoClose defaults to true, I think we should land the new option first and wait some extra time before making this EOL (assuming greenish enough CITGM).

LiviaMedeiros avatar May 31 '25 20:05 LiviaMedeiros

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

nodejs-github-bot avatar Jun 01 '25 01:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 14 '25 17:06 nodejs-github-bot

~~CI: https://ci.nodejs.org/job/node-test-pull-request/67445/~~ Duplicate... there was already a CI run going CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3612/

nodejs-github-bot avatar Jun 14 '25 19:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 15 '25 18:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 20 '25 17:06 nodejs-github-bot

There are relevant failures in CI ...

jasnell avatar Jun 20 '25 19:06 jasnell

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

nodejs-github-bot avatar Jun 20 '25 20:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 20 '25 20:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 20 '25 20:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 22 '25 04:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 24 '25 03:06 nodejs-github-bot

Landed in 2bb7667475e9

jasnell avatar Jun 25 '25 16:06 jasnell