fs: deprecate closing FileHandle on garbage collection

jasnell · jasnell · commit 2d8febceef35 · 2020-02-04T18:29:06.000-08:00
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 <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git 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`.
 
+<a id="DEP00XX"></a>
+### DEP00XX: Closing fs.FileHandle on garbage collection
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/28396
+    description: Runtime deprecation.
+-->
+
+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
@@ -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
@@ -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<double> destroy_async_id_list_;
 
diff --git 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
@@ -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
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -888,6 +888,14 @@ inline bool Environment::owns_inspector() const {`
`888`	`888`	`return flags_ & kOwnsInspector;`
`889`	`889`	`}`
`890`	`890`
	`891`	`+bool Environment::filehandle_close_warning() const {`
	`892`	`+ return emit_filehandle_warning_;`
	`893`	`+}`
	`894`	`+`
	`895`	`+void Environment::set_filehandle_close_warning(bool on) {`
	`896`	`+ emit_filehandle_warning_ = on;`
	`897`	`+}`
	`898`	`+`
`891`	`899`	`inline uint64_t Environment::thread_id() const {`
`892`	`900`	`return thread_id_;`
`893`	`901`	`}`