FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: move FileHandle close on GC to EOL · nodejs/node@2bb7667 · GitHub

/ node Public

Commit 2bb7667

Browse files
jasnell
committed
fs: move FileHandle close on GC to EOL
Automatically closing a FileHandle on garbage collection has been deprecated for some time now. We've said that it will eventually be removed and become and error. PR-URL: #58536 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
1 parent 100c6da commit 2bb7667

8 files changed

Lines changed: 75 additions & 163 deletions

File tree

‎doc/api/deprecations.md‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2865,16 +2865,18 @@ To maintain existing behavior `response.finished` should be replaced with
28652865

28662866
<!-- YAML
28672867
changes:
2868+
- version: REPLACEME
2869+
pr-url: https://github.com/nodejs/node/pull/58536
2870+
description: End-of-Life.
28682871
- version: v14.0.0
28692872
pr-url: https://github.com/nodejs/node/pull/28396
28702873
description: Runtime deprecation.
28712874
-->
28722875

2873-
Type: Runtime
2876+
Type: End-of-Life
28742877

2875-
Allowing a [`fs.FileHandle`][] object to be closed on garbage collection is
2876-
deprecated. In the future, doing so might result in a thrown error that will
2877-
terminate the process.
2878+
Allowing a [`fs.FileHandle`][] object to be closed on garbage collection used
2879+
to be allowed, but now throws an error.
28782880

28792881
Please ensure that all `fs.FileHandle` objects are explicitly closed using
28802882
`FileHandle.prototype.close()` when the `fs.FileHandle` is no longer needed:

‎doc/api/fs.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,7 @@ const {
540540
541541
While the `ReadableStream` will read the file to completion, it will not
542542
close the `FileHandle` automatically. User code must still call the
543-
`fileHandle.close()` method.
543+
`fileHandle.close()` method unless the `autoClose` option is set to `true`.
544544
545545
#### `filehandle.readFile(options)`
546546

‎src/env-inl.h‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -683,14 +683,6 @@ inline bool Environment::no_browser_globals() const {
683683
#endif
684684
}
685685

686-
bool Environment::filehandle_close_warning() const {
687-
return emit_filehandle_warning_;
688-
}
689-
690-
void Environment::set_filehandle_close_warning(bool on) {
691-
emit_filehandle_warning_ = on;
692-
}
693-
694686
void Environment::set_source_maps_enabled(bool on) {
695687
source_maps_enabled_ = on;
696688
}

‎src/env.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -822,9 +822,6 @@ class Environment final : public MemoryRetainer {
822822
inline node_module* extra_linked_bindings_tail();
823823
inline const Mutex& extra_linked_bindings_mutex() const;
824824

825-
inline bool filehandle_close_warning() const;
826-
inline void set_filehandle_close_warning(bool on);
827-
828825
inline void set_source_maps_enabled(bool on);
829826
inline bool source_maps_enabled() const;
830827

@@ -1106,7 +1103,6 @@ class Environment final : public MemoryRetainer {
11061103
bool trace_sync_io_ = false;
11071104
bool emit_env_nonstring_warning_ = true;
11081105
bool emit_err_name_warning_ = true;
1109-
bool emit_filehandle_warning_ = true;
11101106
bool source_maps_enabled_ = false;
11111107

11121108
size_t async_callback_scope_depth_ = 0;

‎src/node_file.cc‎

Lines changed: 30 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -333,12 +333,10 @@ BaseObjectPtr<BaseObject> FileHandle::TransferData::Deserialize(
333333
return BaseObjectPtr<BaseObject> { FileHandle::New(bd, fd) };
334334
}
335335

336-
// Close the file descriptor if it hasn't already been closed. A process
337-
// warning will be emitted using a SetImmediate to avoid calling back to
338-
// JS during GC. If closing the fd fails at this point, a fatal exception
339-
// will crash the process immediately.
336+
// Throw an exception if the file handle has not yet been closed.
340337
inline void FileHandle::Close() {
341338
if (closed_ || closing_) return;
339+
342340
uv_fs_t req;
343341
CHECK_NE(fd_, -1);
344342
FS_SYNC_TRACE_BEGIN(close);
@@ -352,42 +350,38 @@ inline void FileHandle::Close() {
352350

353351
AfterClose();
354352

355-
if (ret < 0) {
356-
// Do not unref this
357-
env()->SetImmediate([detail](Environment* env) {
353+
// Even though we closed the file descriptor, we still throw an error
354+
// if the FileHandle object was not closed before garbage collection.
355+
// Because this method is called during garbage collection, we will defer
356+
// throwing the error until the next immediate queue tick so as not
357+
// to interfere with the gc process.
358+
//
359+
// This exception will end up being fatal for the process because
360+
// it is being thrown from within the SetImmediate handler and
361+
// there is no JS stack to bubble it to. In other words, tearing
362+
// down the process is the only reasonable thing we can do here.
363+
env()->SetImmediate([detail](Environment* env) {
364+
HandleScope handle_scope(env->isolate());
365+
366+
// If there was an error while trying to close the file descriptor,
367+
// we will throw that instead.
368+
if (detail.ret < 0) {
358369
char msg[70];
359-
snprintf(msg, arraysize(msg),
360-
"Closing file descriptor %d on garbage collection failed",
361-
detail.fd);
362-
// This exception will end up being fatal for the process because
363-
// it is being thrown from within the SetImmediate handler and
364-
// there is no JS stack to bubble it to. In other words, tearing
365-
// down the process is the only reasonable thing we can do here.
370+
snprintf(msg,
371+
arraysize(msg),
372+
"Closing file descriptor %d on garbage collection failed",
373+
detail.fd);
366374
HandleScope handle_scope(env->isolate());
367375
env->ThrowUVException(detail.ret, "close", msg);
368-
});
369-
return;
370-
}
371-
372-
// If the close was successful, we still want to emit a process warning
373-
// to notify that the file descriptor was gc'd. We want to be noisy about
374-
// this because not explicitly closing the FileHandle is a bug.
376+
return;
377+
}
375378

376-
env()->SetImmediate([detail](Environment* env) {
377-
ProcessEmitWarning(env,
378-
"Closing file descriptor %d on garbage collection",
379-
detail.fd);
380-
if (env->filehandle_close_warning()) {
381-
env->set_filehandle_close_warning(false);
382-
USE(ProcessEmitDeprecationWarning(
383-
env,
384-
"Closing a FileHandle object on garbage collection is deprecated. "
385-
"Please close FileHandle objects explicitly using "
386-
"FileHandle.prototype.close(). In the future, an error will be "
387-
"thrown if a file descriptor is closed during garbage collection.",
388-
"DEP0137"));
389-
}
390-
}, CallbackFlags::kUnrefed);
379+
THROW_ERR_INVALID_STATE(
380+
env,
381+
"A FileHandle object was closed during garbage collection. "
382+
"This used to be allowed with a deprecation warning but is now "
383+
"considered an error. Please close FileHandle objects explicitly.");
384+
});
391385
}
392386

393387
void FileHandle::CloseReq::Resolve() {

‎test/parallel/test-fs-filehandle.js‎

Lines changed: 9 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,33 +8,21 @@ const { internalBinding } = require('internal/test/binding');
88
const fs = internalBinding('fs');
99
const { stringToFlags } = require('internal/fs/utils');
1010

11-
// Verifies that the FileHandle object is garbage collected and that a
12-
// warning is emitted if it is not closed.
11+
// Verifies that the FileHandle object is garbage collected and that an
12+
// error is thrown if it is not closed.
13+
process.on('uncaughtException', common.mustCall((err) => {
14+
assert.strictEqual(err.code, 'ERR_INVALID_STATE');
15+
assert.match(err.message, /^A FileHandle object was closed during/);
16+
}));
17+
1318

14-
let fdnum;
1519
{
1620
const ctx = {};
17-
fdnum = fs.openFileHandle(path.toNamespacedPath(__filename),
18-
stringToFlags('r'), 0o666, undefined, ctx).fd;
21+
fs.openFileHandle(path.toNamespacedPath(__filename),
22+
stringToFlags('r'), 0o666, undefined, ctx);
1923
assert.strictEqual(ctx.errno, undefined);
2024
}
2125

22-
const deprecationWarning =
23-
'Closing a FileHandle object on garbage collection is deprecated. ' +
24-
'Please close FileHandle objects explicitly using ' +
25-
'FileHandle.prototype.close(). In the future, an error will be ' +
26-
'thrown if a file descriptor is closed during garbage collection.';
27-
28-
common.expectWarning({
29-
'internal/test/binding': [
30-
'These APIs are for internal testing only. Do not use them.',
31-
],
32-
'Warning': [
33-
`Closing file descriptor ${fdnum} on garbage collection`,
34-
],
35-
'DeprecationWarning': [[deprecationWarning, 'DEP0137']]
36-
});
37-
3826
globalThis.gc();
3927

4028
setTimeout(() => {}, 10);

‎test/parallel/test-fs-promises-file-handle-close.js‎

Lines changed: 0 additions & 41 deletions
This file was deleted.

‎test/parallel/test-fs-promises-file-handle-readFile.js‎

Lines changed: 29 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ tmpdir.refresh();
2222

2323
async function validateReadFile() {
2424
const filePath = path.resolve(tmpDir, 'tmp-read-file.txt');
25-
const fileHandle = await open(filePath, 'w+');
25+
await using fileHandle = await open(filePath, 'w+');
2626
const buffer = Buffer.from('Hello world'.repeat(100), 'utf8');
2727

2828
const fd = fs.openSync(filePath, 'w+');
@@ -31,8 +31,6 @@ async function validateReadFile() {
3131

3232
const readFileData = await fileHandle.readFile();
3333
assert.deepStrictEqual(buffer, readFileData);
34-
35-
await fileHandle.close();
3634
}
3735

3836
async function validateReadFileProc() {
@@ -46,48 +44,36 @@ async function validateReadFileProc() {
4644
if (!common.isLinux)
4745
return;
4846

49-
const fileHandle = await open('/proc/sys/kernel/hostname', 'r');
50-
try {
51-
const hostname = await fileHandle.readFile();
52-
assert.ok(hostname.length > 0);
53-
} finally {
54-
await fileHandle.close();
55-
}
47+
await using fileHandle = await open('/proc/sys/kernel/hostname', 'r');
48+
const hostname = await fileHandle.readFile();
49+
assert.ok(hostname.length > 0);
5650
}
5751

5852
async function doReadAndCancel() {
5953
// Signal aborted from the start
6054
{
6155
const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt');
62-
const fileHandle = await open(filePathForHandle, 'w+');
63-
try {
64-
const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
65-
fs.writeFileSync(filePathForHandle, buffer);
66-
const signal = AbortSignal.abort();
67-
await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
68-
name: 'AbortError'
69-
});
70-
} finally {
71-
await fileHandle.close();
72-
}
56+
await using fileHandle = await open(filePathForHandle, 'w+');
57+
const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
58+
fs.writeFileSync(filePathForHandle, buffer);
59+
const signal = AbortSignal.abort();
60+
await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
61+
name: 'AbortError'
62+
});
7363
}
7464

7565
// Signal aborted on first tick
7666
{
7767
const filePathForHandle = path.resolve(tmpDir, 'dogs-running1.txt');
78-
const fileHandle = await open(filePathForHandle, 'w+');
79-
try {
80-
const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
81-
fs.writeFileSync(filePathForHandle, buffer);
82-
const controller = new AbortController();
83-
const { signal } = controller;
84-
process.nextTick(() => controller.abort());
85-
await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
86-
name: 'AbortError'
87-
}, 'tick-0');
88-
} finally {
89-
await fileHandle.close();
90-
}
68+
await using fileHandle = await open(filePathForHandle, 'w+');
69+
const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
70+
fs.writeFileSync(filePathForHandle, buffer);
71+
const controller = new AbortController();
72+
const { signal } = controller;
73+
process.nextTick(() => controller.abort());
74+
await assert.rejects(readFile(fileHandle, common.mustNotMutateObjectDeep({ signal })), {
75+
name: 'AbortError'
76+
}, 'tick-0');
9177
}
9278

9379
// Signal aborted right before buffer read
@@ -96,18 +82,14 @@ async function doReadAndCancel() {
9682
const buffer = Buffer.from('Dogs running'.repeat(1000), 'utf8');
9783
fs.writeFileSync(newFile, buffer);
9884

99-
const fileHandle = await open(newFile, 'r');
100-
try {
101-
const controller = new AbortController();
102-
const { signal } = controller;
103-
tick(1, () => controller.abort());
104-
await assert.rejects(fileHandle.readFile(
105-
common.mustNotMutateObjectDeep({ signal, encoding: 'utf8' })), {
106-
name: 'AbortError'
107-
}, 'tick-1');
108-
} finally {
109-
await fileHandle.close();
110-
}
85+
await using fileHandle = await open(newFile, 'r');
86+
const controller = new AbortController();
87+
const { signal } = controller;
88+
tick(1, () => controller.abort());
89+
await assert.rejects(fileHandle.readFile(
90+
common.mustNotMutateObjectDeep({ signal, encoding: 'utf8' })), {
91+
name: 'AbortError'
92+
}, 'tick-1');
11193
}
11294

11395
// Validate file size is within range for reading
@@ -123,13 +105,12 @@ async function doReadAndCancel() {
123105
await writeFile(newFile, Buffer.from('0'));
124106
await truncate(newFile, kIoMaxLength + 1);
125107

126-
const fileHandle = await open(newFile, 'r');
108+
await using fileHandle = await open(newFile, 'r');
127109

128110
await assert.rejects(fileHandle.readFile(), {
129111
name: 'RangeError',
130112
code: 'ERR_FS_FILE_TOO_LARGE'
131113
});
132-
await fileHandle.close();
133114
}
134115
}
135116
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL