Welcome to mirror list, hosted at ThFree Co, Russian Federation.

github.com/nodejs/node.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--doc/api/deprecations.md33
-rw-r--r--src/env-inl.h8
-rw-r--r--src/env.h4
-rw-r--r--src/node_file.cc11
-rw-r--r--test/parallel/test-fs-filehandle.js9
-rw-r--r--test/parallel/test-fs-promises-file-handle-close.js39
6 files changed, 102 insertions, 2 deletions
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index f6fec7d1ba0..609b906e51c 100644
--- 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
index 2002df9abaf..3c7b83795d7 100644
--- 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
index 3b636dafd2c..83f2a0b54ed 100644
--- 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
index 0547b2d35de..6436de5a67e 100644
--- 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
index 30f42a60f04..f56a8cc6ab5 100644
--- 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
new file mode 100644
index 00000000000..ee1ab50200b
--- /dev/null
+++ 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);