fs: runtime deprecate closing fs.Dir on garbage collection

Author: LiviaMedeiros

doc/api/deprecations.md

@@ -4104,6 +4104,45 @@ The `node:_http_agent`, `node:_http_client`, `node:_http_common`, `node:_http_in
 `node:_http_outgoing` and `node:_http_server` modules are deprecated as they should be considered
 an internal nodejs implementation rather than a public facing API, use `node:http` instead.
 
+### DEP0200: 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

src/env-inl.h

@@ -918,6 +918,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

@@ -820,6 +820,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;
 
@@ -1102,6 +1105,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.",
+              "DEP0200"));
+        }
+      },
+      CallbackFlags::kUnrefed);
 }
 
 void AfterClose(uv_fs_t* req) {

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

@@ -0,0 +1,42 @@
+// 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 { rejects } from 'node:assert';
+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.';
+
+// Test that closing Dir automatically using iterator doesn't emit warning
+{
+  const dir = await opendir(import.meta.dirname);
+  for await (const _ of dir) {}
+  await rejects(dir.close(), { code: 'ERR_DIR_CLOSED' });
+}
+
+{
+  expectWarning({
+    Warning: [['Closing directory handle on garbage collection']],
+    DeprecationWarning: [[warning, 'DEP0200']],
+  });
+
+  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());
+    });
+  }));
+}