From 2d8febceef35bdd52624028bdee2e0d58830ae7f Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Jun 2019 08:35:04 -0500 Subject: [PATCH] fs: deprecate closing FileHandle on garbage collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/28396 Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen Reviewed-By: Luigi Pinca Reviewed-By: Rich Trott --- doc/api/deprecations.md | 33 +++++++++++++++- src/env-inl.h | 8 ++++ src/env.h | 4 ++ src/node_file.cc | 11 ++++++ test/parallel/test-fs-filehandle.js | 9 ++++- .../test-fs-promises-file-handle-close.js | 39 +++++++++++++++++++ 6 files changed, 102 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..609b906e51c0e6 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2538,7 +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 +2568,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 +2636,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 3b636dafd2c7d7..83f2a0b54ed2ba 100644 --- a/src/env.h +++ b/src/env.h @@ -1073,6 +1073,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); @@ -1288,6 +1291,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 0547b2d35debf3..6436de5a67ee16 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-filehandle.js b/test/parallel/test-fs-filehandle.js index 30f42a60f044b3..f56a8cc6ab5cb4 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': [ `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 new file mode 100644 index 00000000000000..ee1ab50200b1dd --- /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, 'object'); + })); +} + +setTimeout(() => global.gc(), 10);