diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 3266d994363e49..affbeb02b6a575 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2865,16 +2865,18 @@ To maintain existing behavior `response.finished` should be replaced with -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: diff --git a/doc/api/fs.md b/doc/api/fs.md index ac936bbad1263e..74b73ee6c69e88 100644 --- a/doc/api/fs.md +++ b/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)` diff --git a/src/env-inl.h b/src/env-inl.h index f4c42efefeca05..28909778adf52e 100644 --- a/src/env-inl.h +++ b/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; } diff --git a/src/env.h b/src/env.h index 10e579bee9c059..db87ec30161ba7 100644 --- a/src/env.h +++ b/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; diff --git a/src/node_file.cc b/src/node_file.cc index 7ad748a80cc7ce..abf15a58bf99b7 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -333,12 +333,10 @@ BaseObjectPtr FileHandle::TransferData::Deserialize( return BaseObjectPtr { 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. + return; + } - 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")); - } - }, 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() { diff --git a/test/parallel/test-fs-filehandle.js b/test/parallel/test-fs-filehandle.js index bcb62da9e4c2cc..6813ad1c28a1d0 100644 --- a/test/parallel/test-fs-filehandle.js +++ b/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); diff --git a/test/parallel/test-fs-promises-file-handle-close.js b/test/parallel/test-fs-promises-file-handle-close.js deleted file mode 100644 index 288bc31ea0ada5..00000000000000 --- a/test/parallel/test-fs-promises-file-handle-close.js +++ /dev/null @@ -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()); - }); -})); diff --git a/test/parallel/test-fs-promises-file-handle-readFile.js b/test/parallel/test-fs-promises-file-handle-readFile.js index 9c93b46a05db2f..997433bf8434dd 100644 --- a/test/parallel/test-fs-promises-file-handle-readFile.js +++ b/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(); } } }