fs: move FileHandle close on GC to EOL

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.

PR-URL: https://github.com/nodejs/node/pull/58536
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

doc/api/deprecations.md

@@ -2865,16 +2865,18 @@ To maintain existing behavior `response.finished` should be replaced with
 
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/58536
+    description: End-of-Life.
   - version: v14.0.0
     pr-url: https://github.com/nodejs/node/pull/28396
     description: Runtime deprecation.
 -->
 
-Type: Runtime
+Type: End-of-Life
 
-Allowing a [`fs.FileHandle`][] object to be closed on garbage collection is
-deprecated. In the future, doing so might result in a thrown error that will
-terminate the process.
+Allowing a [`fs.FileHandle`][] object to be closed on garbage collection used
+to be allowed, but now throws an error.
 
 Please ensure that all `fs.FileHandle` objects are explicitly closed using
 `FileHandle.prototype.close()` when the `fs.FileHandle` is no longer needed:

doc/api/fs.md

@@ -540,7 +540,7 @@ const {
 
 While the `ReadableStream` will read the file to completion, it will not
 close the `FileHandle` automatically. User code must still call the
-`fileHandle.close()` method.
+`fileHandle.close()` method unless the `autoClose` option is set to `true`.
 
 #### `filehandle.readFile(options)`
 

src/env-inl.h

@@ -683,14 +683,6 @@ inline bool Environment::no_browser_globals() const {
 #endif
 }
 
-bool Environment::filehandle_close_warning() const {
-  return emit_filehandle_warning_;
-}
-
-void Environment::set_filehandle_close_warning(bool on) {
-  emit_filehandle_warning_ = on;
-}
-
 void Environment::set_source_maps_enabled(bool on) {
   source_maps_enabled_ = on;
 }

src/env.h

@@ -822,9 +822,6 @@ class Environment final : public MemoryRetainer {
   inline node_module* extra_linked_bindings_tail();
   inline const Mutex& extra_linked_bindings_mutex() const;
 
-  inline bool filehandle_close_warning() const;
-  inline void set_filehandle_close_warning(bool on);
-
   inline void set_source_maps_enabled(bool on);
   inline bool source_maps_enabled() const;
 
@@ -1106,7 +1103,6 @@ class Environment final : public MemoryRetainer {
   bool trace_sync_io_ = false;
   bool emit_env_nonstring_warning_ = true;
   bool emit_err_name_warning_ = true;
-  bool emit_filehandle_warning_ = true;
   bool source_maps_enabled_ = false;
 
   size_t async_callback_scope_depth_ = 0;

src/node_file.cc

@@ -333,12 +333,10 @@ BaseObjectPtr<BaseObject> FileHandle::TransferData::Deserialize(
   return BaseObjectPtr<BaseObject> { FileHandle::New(bd, fd) };
 }
 
-// Close the file descriptor if it hasn't already been closed. A process
-// warning will be emitted using a SetImmediate to avoid calling back to
-// JS during GC. If closing the fd fails at this point, a fatal exception
-// will crash the process immediately.
+// Throw an exception if the file handle has not yet been closed.
 inline void FileHandle::Close() {
   if (closed_ || closing_) return;
+
   uv_fs_t req;
   CHECK_NE(fd_, -1);
   FS_SYNC_TRACE_BEGIN(close);
@@ -352,42 +350,38 @@ inline void FileHandle::Close() {
 
   AfterClose();
 
-  if (ret < 0) {
-    // Do not unref this
-    env()->SetImmediate([detail](Environment* env) {
+  // Even though we closed the file descriptor, we still throw an error
+  // if the FileHandle object was not closed before garbage collection.
+  // Because this method is called during garbage collection, we will defer
+  // throwing the error until the next immediate queue tick so as not
+  // to interfere with the gc process.
+  //
+  // This exception will end up being fatal for the process because
+  // it is being thrown from within the SetImmediate handler and
+  // there is no JS stack to bubble it to. In other words, tearing
+  // down the process is the only reasonable thing we can do here.
+  env()->SetImmediate([detail](Environment* env) {
+    HandleScope handle_scope(env->isolate());
+
+    // If there was an error while trying to close the file descriptor,
+    // we will throw that instead.
+    if (detail.ret < 0) {
       char msg[70];
-      snprintf(msg, arraysize(msg),
-              "Closing file descriptor %d on garbage collection failed",
-              detail.fd);
-      // This exception will end up being fatal for the process because
-      // it is being thrown from within the SetImmediate handler and
-      // there is no JS stack to bubble it to. In other words, tearing
-      // down the process is the only reasonable thing we can do here.
+      snprintf(msg,
+               arraysize(msg),
+               "Closing file descriptor %d on garbage collection failed",
+               detail.fd);
       HandleScope handle_scope(env->isolate());
       env->ThrowUVException(detail.ret, "close", msg);
-    });
-    return;
-  }
-
-  // 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()->SetImmediate([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);
-      USE(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.",
-          "DEP0137"));
+      return;
     }
-  }, CallbackFlags::kUnrefed);
+
+    THROW_ERR_INVALID_STATE(
+        env,
+        "A FileHandle object was closed during garbage collection. "
+        "This used to be allowed with a deprecation warning but is now "
+        "considered an error. Please close FileHandle objects explicitly.");
+  });
 }
 
 void FileHandle::CloseReq::Resolve() {

test/parallel/test-fs-filehandle.js

@@ -8,33 +8,21 @@ const { internalBinding } = require('internal/test/binding');
 const fs = internalBinding('fs');
 const { stringToFlags } = require('internal/fs/utils');
 
-// Verifies that the FileHandle object is garbage collected and that a
-// warning is emitted if it is not closed.
+// Verifies that the FileHandle object is garbage collected and that an
+// error is thrown if it is not closed.
+process.on('uncaughtException', common.mustCall((err) => {
+  assert.strictEqual(err.code, 'ERR_INVALID_STATE');
+  assert.match(err.message, /^A FileHandle object was closed during/);
+}));
+
 
-let fdnum;
 {
   const ctx = {};
-  fdnum = fs.openFileHandle(path.toNamespacedPath(__filename),
-                            stringToFlags('r'), 0o666, undefined, ctx).fd;
+  fs.openFileHandle(path.toNamespacedPath(__filename),
+                    stringToFlags('r'), 0o666, undefined, ctx);
   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, 'DEP0137']]
-});
-
 globalThis.gc();
 
 setTimeout(() => {}, 10);

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

@@ -1,41 +0,0 @@
-// 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, 'DEP0137']]
-  });
-
-  return fh;
-}
-
-doOpen().then(common.mustCall((fd) => {
-  assert.strictEqual(typeof fd, 'object');
-})).then(common.mustCall(() => {
-  setImmediate(() => {
-    // The FileHandle should be out-of-scope and no longer accessed now.
-    globalThis.gc();
-
-    // Wait an extra event loop turn, as the warning is emitted from the
-    // native layer in an unref()'ed setImmediate() callback.
-    setImmediate(common.mustCall());
-  });
-}));

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

@@ -22,7 +22,7 @@ tmpdir.refresh();
 
 async function validateReadFile() {
   const filePath = path.resolve(tmpDir, 'tmp-read-file.txt');
-  const fileHandle = await open(filePath, 'w+');
+  await using fileHandle = await open(filePath, 'w+');
   const buffer = Buffer.from('Hello world'.repeat(100), 'utf8');
 
   const fd = fs.openSync(filePath, 'w+');
@@ -31,8 +31,6 @@ async function validateReadFile() {
 
   const readFileData = await fileHandle.readFile();
   assert.deepStrictEqual(buffer, readFileData);
-
-  await fileHandle.close();
 }
 
 async function validateReadFileProc() {
@@ -46,48 +44,36 @@ async function validateReadFileProc() {
   if (!common.isLinux)
     return;
 
-  const fileHandle = await open('/proc/sys/kernel/hostname', 'r');
-  try {
-    const hostname = await fileHandle.readFile();
-    assert.ok(hostname.length > 0);
-  } finally {
-    await fileHandle.close();
-  }
+  await using fileHandle = await open('/proc/sys/kernel/hostname', 'r');
+  const hostname = await fileHandle.readFile();
+  assert.ok(hostname.length > 0);
 }
 
 async function doReadAndCancel() {
   // Signal aborted from the start
   {
     const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt');
-    const fileHandle = await open(filePathForHandle, 'w+');
-    try {
-      const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
-      fs.writeFileSync(filePathForHandle, buffer);
-      const signal = AbortSignal.abort();
-      await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
-        name: 'AbortError'
-      });
-    } finally {
-      await fileHandle.close();
-    }
+    await using fileHandle = await open(filePathForHandle, 'w+');
+    const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
+    fs.writeFileSync(filePathForHandle, buffer);
+    const signal = AbortSignal.abort();
+    await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
+      name: 'AbortError'
+    });
   }
 
   // Signal aborted on first tick
   {
     const filePathForHandle = path.resolve(tmpDir, 'dogs-running1.txt');
-    const fileHandle = await open(filePathForHandle, 'w+');
-    try {
-      const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
-      fs.writeFileSync(filePathForHandle, buffer);
-      const controller = new AbortController();
-      const { signal } = controller;
-      process.nextTick(() => controller.abort());
-      await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
-        name: 'AbortError'
-      }, 'tick-0');
-    } finally {
-      await fileHandle.close();
-    }
+    await using fileHandle = await open(filePathForHandle, 'w+');
+    const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
+    fs.writeFileSync(filePathForHandle, buffer);
+    const controller = new AbortController();
+    const { signal } = controller;
+    process.nextTick(() => controller.abort());
+    await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
+      name: 'AbortError'
+    }, 'tick-0');
   }
 
   // Signal aborted right before buffer read
@@ -96,18 +82,14 @@ async function doReadAndCancel() {
     const buffer = Buffer.from('Dogs running'.repeat(1000), 'utf8');
     fs.writeFileSync(newFile, buffer);
 
-    const fileHandle = await open(newFile, 'r');
-    try {
-      const controller = new AbortController();
-      const { signal } = controller;
-      tick(1, () => controller.abort());
-      await assert.rejects(fileHandle.readFile(
-        common.mustNotMutateObjectDeep({ signal, encoding: 'utf8' })), {
-        name: 'AbortError'
-      }, 'tick-1');
-    } finally {
-      await fileHandle.close();
-    }
+    await using fileHandle = await open(newFile, 'r');
+    const controller = new AbortController();
+    const { signal } = controller;
+    tick(1, () => controller.abort());
+    await assert.rejects(fileHandle.readFile(
+      common.mustNotMutateObjectDeep({ signal, encoding: 'utf8' })), {
+      name: 'AbortError'
+    }, 'tick-1');
   }
 
   // Validate file size is within range for reading
@@ -123,13 +105,12 @@ async function doReadAndCancel() {
       await writeFile(newFile, Buffer.from('0'));
       await truncate(newFile, kIoMaxLength + 1);
 
-      const fileHandle = await open(newFile, 'r');
+      await using fileHandle = await open(newFile, 'r');
 
       await assert.rejects(fileHandle.readFile(), {
         name: 'RangeError',
         code: 'ERR_FS_FILE_TOO_LARGE'
       });
-      await fileHandle.close();
     }
   }
 }