fs: runtime deprecate closing FileHandle on garbage collection#28396

jasnell · 2019-06-23T17:46:24Z

Closing the FileHandle on garbage collection is a bad practice.
Runtime deprecate and indicate that an error will be thrown in
the future.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

doc/api/deprecations.md

cjihrig

LGTM. The code example in the deprecation docs could probably just be dropped, but it's fine either way.

doc/api/deprecations.md

addaleax · 2019-06-24T21:00:46Z

Hm, no strong opinion here, but using a deprecation warning means it’s only shown once rather than for each fd, right? Is that a good thing or should the separate warning be kept (e.g. the way it’s done for unhandled rejections)?

doc/api/deprecations.md

jasnell · 2019-06-25T00:16:09Z

@addaleax... Sure, I could keep the other warning.

jasnell · 2019-06-25T03:03:24Z

@addaleax ... added back the other warning, fixed a couple linting issues, and updated the test. PTAL :-)

@Trott @lpinca @cjihrig @tniessen ... may I ask y'all to take another look as well?

doc/api/deprecations.md

jasnell · 2019-10-16T18:01:49Z

FYI... I'll be getting back to this very soon.

test/parallel/test-fs-promises-file-handle-close.js

doc/api/deprecations.md

fhinkel · 2019-11-01T00:42:51Z

ping @jasnell

jasnell · 2019-11-01T00:58:03Z

Yep, haven't forgotten. Will get to this next week

…

On Thu, Oct 31, 2019, 17:43 F. Hinkelmann ***@***.***> wrote: ping @jasnell <https://github.com/jasnell> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28396?email_source=notifications&email_token=AADLM6OCAR7TGMXG3APJTKDQRN3RLA5CNFSM4H2ZNPEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECZVGVQ#issuecomment-548623190>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADLM6KOFXFKD3HBG57OE43QRN3RLANCNFSM4H2ZNPEA> .

jasnell · 2020-02-03T17:01:43Z

Ping @addaleax @Trott @lpinca @cjihrig @tniessen ... please take another look. Updated this to avoid using process state

nodejs-github-bot · 2020-02-04T05:24:15Z

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

nodejs-github-bot · 2020-02-04T15:26:03Z

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

jasnell · 2020-02-04T15:39:09Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2259/ (link may not be immediately activated)

jasnell · 2020-02-05T02:27:08Z

CITGM results show no relevant failures.

Closing the FileHandle on garbage collection is a bad practice. Runtime deprecate and indicate that an error will be thrown in the future. PR-URL: #28396 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

jasnell · 2020-02-05T02:32:28Z

Landed in #28396

PR-URL: nodejs#31674 Refs: nodejs#28396 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Notable changes: * (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL (James M Snell) [#31166](#31166) * (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection (James M Snell) [#28396](#28396) * (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL (James M Snell) [#31164](#31164) * (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL (James M Snell) [#31167](#31167) * (SEMVER-MAJOR) os: move tmpDir() to EOL (James M Snell) [#31169](#31169) * (SEMVER-MAJOR) src: remove deprecated wasm type check (Clemens Backes) [#32116](#32116) * (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL (James M Snell) [#31165](#31165) * (SEMVER-MINOR) doc: deprecate process.mainModule (Antoine du HAMEL) [#32232](#32232) * (SEMVER-MINOR) doc: deprecate process.umask() with no arguments (Colin Ihrig) [#32499](#32499) * (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x (AshCripps) [#32454](#32454) * (SEMVER-MAJOR) win: block running on EOL Windows versions (João Reis) [#31954](#31954) * (SEMVER-MAJOR) deps: update V8 to 8.1.307.20 (Matheus Marchini) [#32116](#32116) PR-URL: #32181

- (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL (James M Snell) [#31166](#31166) - (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection (James M Snell) [#28396](#28396) - (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL (James M Snell) [#31164](#31164) - (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL (James M Snell) [#31167](#31167) - (SEMVER-MAJOR) os: move tmpDir() to EOL (James M Snell)[#31169](#31169) - (SEMVER-MAJOR) src: remove deprecated wasm type check (Clemens Backes) [#32116](#32116) - (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL (James M Snell) [#31165](#31165) - (SEMVER-MINOR) doc: deprecate process.mainModule (Antoine du HAMEL) [#32232](#32232) - (SEMVER-MINOR) doc: deprecate process.umask() with no arguments (Colin Ihrig) [#32499](#32499) - src: migrate to new V8 ArrayBuffer API (Thang Tran) [#30782](#30782) It is possible that this change will impact some native addons using `ArrayBuffer`. - (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x (AshCripps)[#32454](#32454) - (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7 (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8 (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) doc: remove SmartOS from official binaries (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) win: block running on EOL Windows versions (João Reis) [#31954](#31954) It is expected that there will be an ABI mismatch on ARM between the Node.js binary and native addons. - [#30786](#30786) - (SEMVER-MAJOR) deps: update V8 to 8.1.307.20 (Matheus Marchini) [#32116](#32116) - cli, report: move --report-on-fatalerror to stable (Colin Ihrig) [#32496](#32496) - deps: upgrade to libuv 1.37.0 (Colin Ihrig) [#32866](#32866) - fs: add fs/promises alias module (Gus Caplan) [#31553](#31553) - module: remove experimental modules warning (Guy Bedford) [#31974](#31974) PR-URL: #32181

Deprecations: - (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL (James M Snell) [#31166](#31166) - (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection (James M Snell) [#28396](#28396) - (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL (James M Snell) [#31164](#31164) - (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL (James M Snell) [#31167](#31167) - (SEMVER-MAJOR) os: move tmpDir() to EOL (James M Snell)[#31169](#31169) - (SEMVER-MAJOR) src: remove deprecated wasm type check (Clemens Backes) [#32116](#32116) - (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL (James M Snell) [#31165](#31165) - (SEMVER-MINOR) doc: deprecate process.mainModule (Antoine du HAMEL) [#32232](#32232) - (SEMVER-MINOR) doc: deprecate process.umask() with no arguments (Colin Ihrig) [#32499](#32499) ECMAScript Modules - Experimental Warning Removal: - module: remove experimental modules warning (Guy Bedford) [#31974](#31974) In Node.js 13 we removed the need to include the --experimental-modules flag, but when running EcmaScript Modules in Node.js, this would still result in a warning ExperimentalWarning: The ESM module loader is experimental. As of Node.js 14 there is no longer this warning when using ESM in Node.js. However, the ESM implementation in Node.js remains experimental. As per our stability index: “The feature is not subject to Semantic Versioning rules. Non-backward compatible changes or removal may occur in any future release.” Users should be cautious when using the feature in production environments. Please keep in mind that the implementation of ESM in Node.js differs from the developer experience you might be familiar with. Most transpilation workflows support features such as optional file extensions or JSON modules that the Node.js ESM implementation does not support. It is highly likely that modules from transpiled environments will require a certain degree of refactoring to work in Node.js. It is worth mentioning that many of our design decisions were made with two primary goals. Spec compliance and Web Compatibility. It is our belief that the current implementation offers a future proof model to authoring ESM modules that paves the path to Universal JavaScript. Please read more in our documentation. The ESM implementation in Node.js is still experimental but we do believe that we are getting very close to being able to call ESM in Node.js “stable”. Removing the warning is a huge step in that direction. Migrate to new V8 ArrayBuffer API: - src: migrate to new V8 ArrayBuffer API (Thang Tran) [#30782](#30782) It is possible that this change will impact some native addons using `ArrayBuffer`. Toolchain and Compiler Upgrades: - (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x (AshCripps)[#32454](#32454) - (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7 (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8 (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) doc: remove SmartOS from official binaries (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) win: block running on EOL Windows versions (João Reis) [#31954](#31954) It is expected that there will be an ABI mismatch on ARM between the Node.js binary and native addons. Native addons are only broken if they interact with `std::shared_ptr`. This is expected to be fixed in a later version of Node.js 14. - [#30786](#30786) Update to V8 8.1: - (SEMVER-MAJOR) deps: update V8 to 8.1.307.20 (Matheus Marchini) [#32116](#32116) Other Notable Changes: - cli, report: move --report-on-fatalerror to stable (Colin Ihrig) [#32496](#32496) - deps: upgrade to libuv 1.37.0 (Colin Ihrig) [#32866](#32866) - fs: add fs/promises alias module (Gus Caplan) [#31553](#31553) PR-URL: #32181

Deprecations: - (SEMVER-MAJOR) crypto: move pbkdf2 without digest to EOL (James M Snell) [#31166](#31166) - (SEMVER-MAJOR) fs: deprecate closing FileHandle on garbage collection (James M Snell) [#28396](#28396) - (SEMVER-MAJOR) http: move OutboundMessage.prototype.flush to EOL (James M Snell) [#31164](#31164) - (SEMVER-MAJOR) lib: move GLOBAL and root aliases to EOL (James M Snell) [#31167](#31167) - (SEMVER-MAJOR) os: move tmpDir() to EOL (James M Snell)[#31169](#31169) - (SEMVER-MAJOR) src: remove deprecated wasm type check (Clemens Backes) [#32116](#32116) - (SEMVER-MAJOR) stream: move \_writableState.buffer to EOL (James M Snell) [#31165](#31165) - (SEMVER-MINOR) doc: deprecate process.mainModule (Antoine du HAMEL) [#32232](#32232) - (SEMVER-MINOR) doc: deprecate process.umask() with no arguments (Colin Ihrig) [#32499](#32499) ECMAScript Modules - Experimental Warning Removal: - module: remove experimental modules warning (Guy Bedford) [#31974](#31974) In Node.js 13 we removed the need to include the --experimental-modules flag, but when running EcmaScript Modules in Node.js, this would still result in a warning ExperimentalWarning: The ESM module loader is experimental. As of Node.js 14 there is no longer this warning when using ESM in Node.js. However, the ESM implementation in Node.js remains experimental. As per our stability index: “The feature is not subject to Semantic Versioning rules. Non-backward compatible changes or removal may occur in any future release.” Users should be cautious when using the feature in production environments. Please keep in mind that the implementation of ESM in Node.js differs from the developer experience you might be familiar with. Most transpilation workflows support features such as optional file extensions or JSON modules that the Node.js ESM implementation does not support. It is highly likely that modules from transpiled environments will require a certain degree of refactoring to work in Node.js. It is worth mentioning that many of our design decisions were made with two primary goals. Spec compliance and Web Compatibility. It is our belief that the current implementation offers a future proof model to authoring ESM modules that paves the path to Universal JavaScript. Please read more in our documentation. The ESM implementation in Node.js is still experimental but we do believe that we are getting very close to being able to call ESM in Node.js “stable”. Removing the warning is a huge step in that direction. New V8 ArrayBuffer API: * **src**: migrate to new V8 ArrayBuffer API (Thang Tran) [#30782](#30782) Multiple ArrayBuffers pointing to the same base address are no longer allowed by V8. This may impact native addons. Toolchain and Compiler Upgrades: - (SEMVER-MAJOR) build: update macos deployment target to 10.13 for 14.x (AshCripps)[#32454](#32454) - (SEMVER-MAJOR) doc: update cross compiler machine for Linux armv7 (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) doc: update Centos/RHEL releases use devtoolset-8 (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) doc: remove SmartOS from official binaries (Richard Lau) [#32812](#32812) - (SEMVER-MAJOR) win: block running on EOL Windows versions (João Reis) [#31954](#31954) It is expected that there will be an ABI mismatch on ARM between the Node.js binary and native addons. Native addons are only broken if they interact with `std::shared_ptr`. This is expected to be fixed in a later version of Node.js 14. - [#30786](#30786) Update to V8 8.1: - (SEMVER-MAJOR) deps: update V8 to 8.1.307.20 (Matheus Marchini) [#32116](#32116) Other Notable Changes: - cli, report: move --report-on-fatalerror to stable (Colin Ihrig) [#32496](#32496) - deps: upgrade to libuv 1.37.0 (Colin Ihrig) [#32866](#32866) - fs: add fs/promises alias module (Gus Caplan) [#31553](#31553) PR-URL: #32181

Uko · 2020-07-01T07:11:29Z

Pardon my curiosity, but why "Closing the FileHandle on garbage collection is a bad practice?" I tried to google that up, but maybe smart people here can give me some pointers :)

tniessen · 2020-07-01T08:50:32Z

@Uko The garbage collector should really only free memory that is not used anymore. Relying on the GC for cleaning up resources (e.g., closing file descriptors) is generally considered bad practice, because

in many implementations, when and how the GC runs is not deterministic,
resource cleanup might be slow and might therefore cause long GC times, and
resource cleanup itself might produce errors, which cannot be handled by the GC.

jasnell added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. promises Issues and PRs related to ECMAScript promises. deprecations Issues and PRs related to deprecations. labels Jun 23, 2019

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 23, 2019

vsemozhetbyt reviewed Jun 23, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

cjihrig approved these changes Jun 24, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

tniessen approved these changes Jun 24, 2019
View reviewed changes

lpinca approved these changes Jun 24, 2019
View reviewed changes

Trott reviewed Jun 24, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

Trott approved these changes Jun 24, 2019
View reviewed changes

jasnell force-pushed the deprecation-close-on-gc branch from e2ffc7d to 95b4e3fCompare June 25, 2019 03:01

Trott reviewed Jul 15, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

Trott force-pushed the master branch from 1ecc406 to 49cf67eCompare September 17, 2019 16:51

tniessen approved these changes Oct 19, 2019
View reviewed changes

test/parallel/test-fs-promises-file-handle-close.js Outdated Show resolvedHide resolved
test/parallel/test-fs-promises-file-handle-close.js Outdated Show resolvedHide resolved

jasnell commented Oct 21, 2019
View reviewed changes

test/parallel/test-fs-promises-file-handle-close.js Outdated Show resolvedHide resolved

jasnell commented Oct 21, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

jasnell commented Oct 21, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

jasnell commented Oct 21, 2019
View reviewed changes

doc/api/deprecations.md Outdated Show resolvedHide resolved

tniessen approved these changes Oct 26, 2019
View reviewed changes

devnexen force-pushed the master branch from e8a4568 to 5289f80Compare December 26, 2019 19:46

jasnell force-pushed the deprecation-close-on-gc branch from 73bca77 to 9abe918Compare February 3, 2020 17:00

jasnell force-pushed the deprecation-close-on-gc branch from 9abe918 to c2560c0Compare February 3, 2020 17:28

fixup! [Squash] test fixup
7390858

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

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 4, 2020

jasnell closed this Feb 5, 2020

cjihrig mentioned this pull request Feb 7, 2020
doc,src,test: assign missing deprecation code #31674
Merged
4 tasks

BethGriggs mentioned this pull request Mar 10, 2020
2020-04-21, Version 14.0.0 (Current) #32181
Merged

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020

kpalumbokitu mentioned this pull request Mar 29, 2022
[Bug]: FileDescriptors not properly released on file rotate winstonjs/winston#2100
Closed

Uh oh!

fs: runtime deprecate closing FileHandle on garbage collection#28396

fs: runtime deprecate closing FileHandle on garbage collection #28396

Uh oh!

Conversation

jasnell commented Jun 23, 2019

Checklist

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax commented Jun 24, 2019

Uh oh!

Uh oh!

jasnell commented Jun 25, 2019

Uh oh!

jasnell commented Jun 25, 2019

Uh oh!

Uh oh!

jasnell commented Oct 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fhinkel commented Nov 1, 2019

Uh oh!

jasnell commented Nov 1, 2019 via email

Uh oh!

jasnell commented Feb 3, 2020

Uh oh!

nodejs-github-bot commented Feb 4, 2020

Uh oh!

nodejs-github-bot commented Feb 4, 2020

Uh oh!

jasnell commented Feb 4, 2020

Uh oh!

jasnell commented Feb 5, 2020

Uh oh!

jasnell commented Feb 5, 2020

Uh oh!

Uko commented Jul 1, 2020

Uh oh!

tniessen commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants