diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 1a3f63acc64d32..f92f1838104456 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -4108,12 +4108,15 @@ an internal nodejs implementation rather than a public facing API, use `node:htt -Type: Documentation-only +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 diff --git a/src/env-inl.h b/src/env-inl.h index 2a2884ee31abab..c97249d3cab7ff 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -919,6 +919,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 diff --git a/src/env.h b/src/env.h index 84ff29e2e80c4e..2be330fdf0f0a1 100644 --- a/src/env.h +++ b/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; @@ -1103,6 +1106,7 @@ class Environment final : public MemoryRetainer { std::shared_ptr 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; diff --git a/src/node_dir.cc b/src/node_dir.cc index c9173d404c79a6..f9ad0326700a97 100644 --- a/src/node_dir.cc +++ b/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) { diff --git a/test/parallel/test-fs-dir-close-gc-deprecation.mjs b/test/parallel/test-fs-dir-close-gc-deprecation.mjs new file mode 100644 index 00000000000000..f0eb72c7696dfc --- /dev/null +++ b/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); + await Array.fromAsync(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()); + }); + })); +}