From 300299e6077082122406041b2bc08676eb640341 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Jun 2019 08:35:04 -0500 Subject: [PATCH 1/4] fs: deprecate closing FileHandle on garbage collection Closing the FileHandle on garbage collection is a bad practice. Runtime deprecate and indicate that an error will be thrown in the future. --- doc/api/deprecations.md | 34 +++++++++++++++- src/env-inl.h | 8 ++++ src/env.h | 4 ++ src/node_file.cc | 11 ++++++ .../test-fs-promises-file-handle-close.js | 39 +++++++++++++++++++ 5 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-fs-promises-file-handle-close.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index f6fec7d1ba0f61..ca2ce391cf7d6a 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2538,8 +2538,6 @@ an officially supported API. changes: - version: v13.0.0 pr-url: https://github.com/nodejs/node/pull/29061 - description: Runtime deprecation. ---> Type: Runtime @@ -2569,6 +2567,37 @@ accordingly instead to avoid the ambigiuty. To maintain existing behaviour `response.finished` should be replaced with `response.writableEnded`. + +### DEP00XX: Closing fs.FileHandle on garbage collection + + +Type: Runtime + +Allowing a [`fs.FileHandle`][] object to be closed on garbage collection is +deprecated. In the future, doing so may result in a thrown error that will +terminate the process. + +Please ensure that all `fs.FileHandle` objects are explicitly closed using +`FileHandle.prototype.close()` when the `fs.FileHandle` is no longer needed: + +```js +const fsPromises = require('fs').promises; +async function openAndClose() { + let filehandle; + try { + filehandle = await fsPromises.open('thefile.txt', 'r'); + } finally { + if (filehandle !== undefined) + await filehandle.close(); + } +} +``` + [`--pending-deprecation`]: cli.html#cli_pending_deprecation [`--throw-deprecation`]: cli.html#cli_throw_deprecation [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size @@ -2606,6 +2635,7 @@ To maintain existing behaviour `response.finished` should be replaced with [`domain`]: domain.html [`ecdh.setPublicKey()`]: crypto.html#crypto_ecdh_setpublickey_publickey_encoding [`emitter.listenerCount(eventName)`]: events.html#events_emitter_listenercount_eventname +[`fs.FileHandle`]: fs.html#fs_class_filehandle [`fs.access()`]: fs.html#fs_fs_access_path_mode_callback [`fs.createReadStream()`]: fs.html#fs_fs_createreadstream_path_options [`fs.createWriteStream()`]: fs.html#fs_fs_createwritestream_path_options diff --git a/src/env-inl.h b/src/env-inl.h index 2002df9abaf1a4..3c7b83795d723a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -888,6 +888,14 @@ inline bool Environment::owns_inspector() const { return flags_ & kOwnsInspector; } +bool Environment::filehandle_close_warning() const { + return emit_filehandle_warning_; +} + +void Environment::set_filehandle_close_warning(bool on) { + emit_filehandle_warning_ = on; +} + inline uint64_t Environment::thread_id() const { return thread_id_; } diff --git a/src/env.h b/src/env.h index 1f8442d478b585..b0bf30c65e17c9 100644 --- a/src/env.h +++ b/src/env.h @@ -1072,6 +1072,9 @@ class Environment : public MemoryRetainer { inline node_module* extra_linked_bindings_head(); inline const Mutex& extra_linked_bindings_mutex() const; + inline bool filehandle_close_warning() const; + inline void set_filehandle_close_warning(bool on); + inline void ThrowError(const char* errmsg); inline void ThrowTypeError(const char* errmsg); inline void ThrowRangeError(const char* errmsg); @@ -1287,6 +1290,7 @@ class Environment : public MemoryRetainer { bool trace_sync_io_ = false; bool emit_env_nonstring_warning_ = true; bool emit_err_name_warning_ = true; + bool emit_filehandle_warning_ = true; size_t async_callback_scope_depth_ = 0; std::vector destroy_async_id_list_; diff --git a/src/node_file.cc b/src/node_file.cc index c35f8aa839ef75..04c0cd65f6f326 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -205,10 +205,21 @@ inline void FileHandle::Close() { // If the close was successful, we still want to emit a process warning // to notify that the file descriptor was gc'd. We want to be noisy about // this because not explicitly closing the FileHandle is a bug. + env()->SetUnrefImmediate([detail](Environment* env) { ProcessEmitWarning(env, "Closing file descriptor %d on garbage collection", detail.fd); + if (env->filehandle_close_warning()) { + env->set_filehandle_close_warning(false); + ProcessEmitDeprecationWarning( + env, + "Closing a FileHandle object on garbage collection is deprecated. " + "Please close FileHandle objects explicitly using " + "FileHandle.prototype.close(). In the future, an error will be " + "thrown if a file descriptor is closed during garbage collection.", + "DEP00XX").IsNothing(); + } }); } diff --git a/test/parallel/test-fs-promises-file-handle-close.js b/test/parallel/test-fs-promises-file-handle-close.js new file mode 100644 index 00000000000000..ef0d73a1622fe3 --- /dev/null +++ b/test/parallel/test-fs-promises-file-handle-close.js @@ -0,0 +1,39 @@ +// Flags: --expose-gc --no-warnings +'use strict'; + +// Test that a runtime warning is emitted when a FileHandle object +// is allowed to close on garbage collection. In the future, this +// test should verify that closing on garbage collection throws a +// process fatal exception. + +const common = require('../common'); +const assert = require('assert'); +const { promises: fs } = require('fs'); + +const warning = + 'Closing a FileHandle object on garbage collection is deprecated. ' + + 'Please close FileHandle objects explicitly using ' + + 'FileHandle.prototype.close(). In the future, an error will be ' + + 'thrown if a file descriptor is closed during garbage collection.'; + +async function doOpen() { + const fh = await fs.open(__filename); + + common.expectWarning({ + Warning: [[`Closing file descriptor ${fh.fd} on garbage collection`]], + DeprecationWarning: [[warning, 'DEP00XX']] + }); + + return fh; +} + +// Perform the file open assignment within a block. +// When the block scope exits, the file handle will +// be eligible for garbage collection. +{ + doOpen().then(common.mustCall((fd) => { + assert.strictEqual(typeof fd, 'number'); + })); +} + +setTimeout(() => global.gc(), 10); From 7e367ae687938b9e8743db6606103d70866bce83 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 3 Feb 2020 20:52:00 -0800 Subject: [PATCH 2/4] fixup! fs: deprecate closing FileHandle on garbage collection --- doc/api/deprecations.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index ca2ce391cf7d6a..609b906e51c0e6 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2538,6 +2538,7 @@ an officially supported API. changes: - version: v13.0.0 pr-url: https://github.com/nodejs/node/pull/29061 +--> Type: Runtime From 3156c50969a399febdea286cf998f5d70d0ec431 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 3 Feb 2020 21:22:18 -0800 Subject: [PATCH 3/4] [Squash] test fixup --- test/parallel/test-fs-filehandle.js | 11 +++++++++-- test/parallel/test-fs-promises-file-handle-close.js | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-fs-filehandle.js b/test/parallel/test-fs-filehandle.js index 30f42a60f044b3..7161a51b863ffc 100644 --- a/test/parallel/test-fs-filehandle.js +++ b/test/parallel/test-fs-filehandle.js @@ -19,13 +19,20 @@ let fdnum; assert.strictEqual(ctx.errno, undefined); } +const deprecationWarning = + 'Closing a FileHandle object on garbage collection is deprecated. ' + + 'Please close FileHandle objects explicitly using ' + + 'FileHandle.prototype.close(). In the future, an error will be ' + + 'thrown if a file descriptor is closed during garbage collection.'; + common.expectWarning({ 'internal/test/binding': [ 'These APIs are for internal testing only. Do not use them.' ], - 'Warning': [ + Warning: [ `Closing file descriptor ${fdnum} on garbage collection` - ] + ], + DeprecationWarning: [[deprecationWarning, 'DEP00XX']] }); global.gc(); diff --git a/test/parallel/test-fs-promises-file-handle-close.js b/test/parallel/test-fs-promises-file-handle-close.js index ef0d73a1622fe3..ee1ab50200b1dd 100644 --- a/test/parallel/test-fs-promises-file-handle-close.js +++ b/test/parallel/test-fs-promises-file-handle-close.js @@ -32,7 +32,7 @@ async function doOpen() { // be eligible for garbage collection. { doOpen().then(common.mustCall((fd) => { - assert.strictEqual(typeof fd, 'number'); + assert.strictEqual(typeof fd, 'object'); })); } From 739085831c234bbfd003635ba25a3c8873ee7921 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 3 Feb 2020 21:31:30 -0800 Subject: [PATCH 4/4] fixup! [Squash] test fixup --- test/parallel/test-fs-filehandle.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-fs-filehandle.js b/test/parallel/test-fs-filehandle.js index 7161a51b863ffc..f56a8cc6ab5cb4 100644 --- a/test/parallel/test-fs-filehandle.js +++ b/test/parallel/test-fs-filehandle.js @@ -29,10 +29,10 @@ common.expectWarning({ 'internal/test/binding': [ 'These APIs are for internal testing only. Do not use them.' ], - Warning: [ + 'Warning': [ `Closing file descriptor ${fdnum} on garbage collection` ], - DeprecationWarning: [[deprecationWarning, 'DEP00XX']] + 'DeprecationWarning': [[deprecationWarning, 'DEP00XX']] }); global.gc();