fs: runtime deprecate closing fs.Dir on garbage collection

Author: LiviaMedeiros

doc/api/deprecations.md

@@ -4058,6 +4058,45 @@ Type: Documentation-only
 
 The [`util.types.isNativeError`][] API is deprecated. Please use [`Error.isError`][] instead.
 
+### DEP0198: Closing fs.Dir on garbage collection
+
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/58850
+    description: Runtime deprecation.
+-->
+
+Type: Runtime
+
+Allowing a [`fs.Dir`][] 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.
+
+Please ensure that all `fs.Dir` objects are explicitly closed using
+`Dir.prototype.close()` or `using` keyword:
+
+```mjs
+import { opendir } from 'node:fs/promises';
+
+{
+  await using dir = await opendir('/async/disposable/directory');
+} // closed by dir[Symbol.asyncDispose]()
+
+{
+  using dir = await opendir('/sync/disposable/directory');
+} // closed by dir[Symbol.dispose]()
+
+{
+  let dir;
+  try {
+    dir = await opendir('/legacy/closeable/directory');
+  } finally {
+    await dir?.close();
+  }
+}
+```
+
 [DEP0142]: #dep0142-repl_builtinlibs
 [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
 [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
@@ -4115,6 +4154,7 @@ The [`util.types.isNativeError`][] API is deprecated. Please use [`Error.isError
 [`ecdh.setPublicKey()`]: crypto.md#ecdhsetpublickeypublickey-encoding
 [`emitter.listenerCount(eventName)`]: events.md#emitterlistenercounteventname-listener
 [`events.listenerCount(emitter, eventName)`]: events.md#eventslistenercountemitter-eventname
+[`fs.Dir`]: fs.md#class-fsdir
 [`fs.FileHandle`]: fs.md#class-filehandle
 [`fs.access()`]: fs.md#fsaccesspath-mode-callback
 [`fs.appendFile()`]: fs.md#fsappendfilepath-data-options-callback

src/env-inl.h

@@ -912,6 +912,14 @@ inline void Environment::RemoveHeapSnapshotNearHeapLimitCallback(
                                         heap_limit);
 }
 
+bool Environment::dir_gc_close_warning() const {
+  return emit_dir_gc_warning_;
+}
+
+void Environment::set_dir_gc_close_warning(bool on) {
+  emit_dir_gc_warning_ = on;
+}
+
 }  // namespace node
 
 // These two files depend on each other. Including base_object-inl.h after this

src/env.h

@@ -822,6 +822,9 @@ class Environment final : public MemoryRetainer {
   inline node_module* extra_linked_bindings_tail();
   inline const Mutex& extra_linked_bindings_mutex() const;
 
+  inline bool dir_gc_close_warning() const;
+  inline void set_dir_gc_close_warning(bool on);
+
   inline void set_source_maps_enabled(bool on);
   inline bool source_maps_enabled() const;
 
@@ -1101,6 +1104,7 @@ class Environment final : public MemoryRetainer {
   std::shared_ptr<KVStore> env_vars_;
   bool printed_error_ = false;
   bool trace_sync_io_ = false;
+  bool emit_dir_gc_warning_ = true;
   bool emit_env_nonstring_warning_ = true;
   bool emit_err_name_warning_ = true;
   bool source_maps_enabled_ = false;

src/node_dir.cc

@@ -164,13 +164,27 @@ inline void DirHandle::GCClose() {
   }
 
   // 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
+  // to notify that the directory handle was gc'd. We want to be noisy about
   // this because not explicitly closing the DirHandle is a bug.
 
-  env()->SetImmediate([](Environment* env) {
-    ProcessEmitWarning(env,
-                       "Closing directory handle on garbage collection");
-  }, CallbackFlags::kUnrefed);
+  env()->SetImmediate(
+      [](Environment* env) {
+        ProcessEmitWarning(env,
+                           "Closing directory handle on garbage collection");
+        if (env->dir_gc_close_warning()) {
+          env->set_dir_gc_close_warning(false);
+          USE(ProcessEmitDeprecationWarning(
+              env,
+              "Closing a Dir object on garbage collection is deprecated. "
+              "Please close Dir objects explicitly using "
+              "Dir.prototype.close(), "
+              "or by using explicit resource management ('using' keyword). "
+              "In the future, an error will be thrown if a directory is closed "
+              "during garbage collection.",
+              "DEP0198"));
+        }
+      },
+      CallbackFlags::kUnrefed);
 }
 
 void AfterClose(uv_fs_t* req) {

test/parallel/test-fs-dir-close-gc-deprecation.mjs

@@ -0,0 +1,34 @@
+// Flags: --expose-gc --no-warnings
+
+// Test that a runtime warning is emitted when a Dir 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.
+
+import { expectWarning, mustCall } from '../common/index.mjs';
+import { opendir } from 'node:fs/promises';
+import { setImmediate } from 'node:timers';
+
+const warning =
+  'Closing a Dir object on garbage collection is deprecated. ' +
+  'Please close Dir objects explicitly using Dir.prototype.close(), ' +
+  "or by using explicit resource management ('using' keyword). " +
+  'In the future, an error will be thrown if a directory is closed during garbage collection.';
+
+{
+  expectWarning({
+    Warning: [['Closing directory handle on garbage collection']],
+    DeprecationWarning: [[warning, 'DEP0198']],
+  });
+
+  await opendir(import.meta.dirname).then(mustCall(() => {
+    setImmediate(() => {
+      // The dir is out of scope 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(mustCall());
+    });
+  }));
+}