Skip to content

Commit f5a1a9e

Browse files
anonriganonrig
committed
fs: add fast api for fs.closeSync only
1 parent 7134a23 commit f5a1a9e

File tree

7 files changed

+118
-20
lines changed

7 files changed

+118
-20
lines changed

‎lib/fs.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@ function close(fd, callback = defaultCloseCallback) {
515515
* @returns {void}
516516
*/
517517
function closeSync(fd) {
518-
binding.close(fd);
518+
binding.closeSync(fd);
519519
}
520520

521521
/**

‎src/env.cc

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1841,12 +1841,29 @@ void Environment::AddUnmanagedFd(int fd) {
18411841
}
18421842
}
18431843

1844-
void Environment::RemoveUnmanagedFd(int fd) {
1844+
void Environment::RemoveUnmanagedFd(int fd,
1845+
ProcessEmitScheduleType schedule_type) {
18451846
if (!tracks_unmanaged_fds()) return;
18461847
size_t removed_count = unmanaged_fds_.erase(fd);
18471848
if (removed_count == 0) {
1848-
ProcessEmitWarning(
1849-
this, "File descriptor %d closed but not opened in unmanaged mode", fd);
1849+
switch (schedule_type) {
1850+
case kSync:
1851+
ProcessEmitWarning(
1852+
this,
1853+
"File descriptor %d closed but not opened in unmanaged mode",
1854+
fd);
1855+
break;
1856+
case kSetImmediate:
1857+
SetImmediateThreadsafe([&](Environment* env) {
1858+
ProcessEmitWarning(
1859+
env,
1860+
"File descriptor %d closed but not opened in unmanaged mode",
1861+
fd);
1862+
});
1863+
break;
1864+
default:
1865+
UNREACHABLE();
1866+
}
18501867
}
18511868
}
18521869

‎src/env.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1026,8 +1026,13 @@ class Environment : public MemoryRetainer {
10261026
uv_buf_t allocate_managed_buffer(const size_t suggested_size);
10271027
std::unique_ptr<v8::BackingStore> release_managed_buffer(const uv_buf_t& buf);
10281028

1029+
enum ProcessEmitScheduleType {
1030+
kSync,
1031+
kSetImmediate,
1032+
};
1033+
10291034
void AddUnmanagedFd(int fd);
1030-
void RemoveUnmanagedFd(int fd);
1035+
void RemoveUnmanagedFd(int fd, ProcessEmitScheduleType schedule_type = kSync);
10311036

10321037
template <typename T>
10331038
void ForEachRealm(T&& iterator) const;

‎src/node_external_reference.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,9 @@ using CFunctionWithInt64Fallback = void (*)(v8::Local<v8::Value>,
5555
const int64_t,
5656
v8::FastApiCallbackOptions&);
5757
using CFunctionWithBool = void (*)(v8::Local<v8::Value>, bool);
58+
using CFunctionWithUint32AndOptions = void (*)(v8::Local<v8::Object>,
59+
const uint32_t,
60+
v8::FastApiCallbackOptions&);
5861

5962
// This class manages the external references from the V8 heap
6063
// to the C++ addresses in Node.js.
@@ -79,6 +82,7 @@ class ExternalReferenceRegistry {
7982
V(CFunctionWithDoubleReturnDouble) \
8083
V(CFunctionWithInt64Fallback) \
8184
V(CFunctionWithBool) \
85+
V(CFunctionWithUint32AndOptions) \
8286
V(const v8::CFunctionInfo*) \
8387
V(v8::FunctionCallback) \
8488
V(v8::AccessorNameGetterCallback) \

‎src/node_file.cc

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ namespace fs {
6060

6161
using v8::Array;
6262
using v8::BigInt;
63+
using v8::CFunction;
6364
using v8::Context;
6465
using v8::EscapableHandleScope;
6566
using v8::FastApiCallbackOptions;
@@ -971,32 +972,67 @@ void Access(const FunctionCallbackInfo<Value>& args) {
971972
}
972973
}
973974

974-
void Close(const FunctionCallbackInfo<Value>& args) {
975+
static void Close(const FunctionCallbackInfo<Value>& args) {
975976
Environment* env = Environment::GetCurrent(args);
976977

977-
const int argc = args.Length();
978-
CHECK_GE(argc, 1);
978+
CHECK_EQ(args.Length(), 2); // fd, req
979979

980980
int fd;
981981
if (!GetValidatedFd(env, args[0]).To(&fd)) {
982982
return;
983983
}
984984
env->RemoveUnmanagedFd(fd);
985985

986-
if (argc > 1) { // close(fd, req)
987-
FSReqBase* req_wrap_async = GetReqWrap(args, 1);
988-
CHECK_NOT_NULL(req_wrap_async);
989-
FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async)
990-
AsyncCall(env, req_wrap_async, args, "close", UTF8, AfterNoArgs,
991-
uv_fs_close, fd);
992-
} else { // close(fd)
993-
FSReqWrapSync req_wrap_sync("close");
994-
FS_SYNC_TRACE_BEGIN(close);
995-
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd);
996-
FS_SYNC_TRACE_END(close);
986+
FSReqBase* req_wrap_async = GetReqWrap(args, 1);
987+
CHECK_NOT_NULL(req_wrap_async);
988+
FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async)
989+
AsyncCall(
990+
env, req_wrap_async, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd);
991+
}
992+
993+
// Separate implementations are required to provide fast API for closeSync.
994+
// If both close and closeSync are implemented using the same function, and
995+
// if a fast API implementation is added for closeSync, close(fd, req) will
996+
// also trigger the fast API implementation and cause an incident.
997+
// Ref: https://github.com/nodejs/node/issues/53902
998+
static void CloseSync(const FunctionCallbackInfo<Value>& args) {
999+
Environment* env = Environment::GetCurrent(args);
1000+
CHECK_EQ(args.Length(), 1);
1001+
1002+
int fd;
1003+
if (!GetValidatedFd(env, args[0]).To(&fd)) {
1004+
return;
1005+
}
1006+
env->RemoveUnmanagedFd(fd);
1007+
FSReqWrapSync req_wrap_sync("close");
1008+
FS_SYNC_TRACE_BEGIN(close);
1009+
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd);
1010+
FS_SYNC_TRACE_END(close);
1011+
}
1012+
1013+
static void FastCloseSync(Local<Object> recv,
1014+
const uint32_t fd,
1015+
// NOLINTNEXTLINE(runtime/references) This is V8 api.
1016+
v8::FastApiCallbackOptions& options) {
1017+
Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked());
1018+
1019+
uv_fs_t req;
1020+
FS_SYNC_TRACE_BEGIN(close);
1021+
int err = uv_fs_close(nullptr, &req, fd, nullptr);
1022+
FS_SYNC_TRACE_END(close);
1023+
1024+
if (is_uv_error(err)) {
1025+
options.fallback = true;
1026+
} else {
1027+
// Note: Only remove unmanaged fds if the close was successful.
1028+
// RemoveUnmanagedFd() can call ProcessEmitWarning() which calls back into
1029+
// JS process.emitWarning() and violates the fast API protocol.
1030+
env->RemoveUnmanagedFd(fd, Environment::kSetImmediate);
9971031
}
9981032
}
9991033

1034+
CFunction fast_close_sync_(CFunction::Make(FastCloseSync));
1035+
10001036
static void ExistsSync(const FunctionCallbackInfo<Value>& args) {
10011037
Environment* env = Environment::GetCurrent(args);
10021038
Isolate* isolate = env->isolate();
@@ -3547,6 +3583,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
35473583
GetFormatOfExtensionlessFile);
35483584
SetMethod(isolate, target, "access", Access);
35493585
SetMethod(isolate, target, "close", Close);
3586+
SetFastMethod(isolate, target, "closeSync", CloseSync, &fast_close_sync_);
35503587
SetMethod(isolate, target, "existsSync", ExistsSync);
35513588
SetMethod(isolate, target, "open", Open);
35523589
SetMethod(isolate, target, "openFileHandle", OpenFileHandle);
@@ -3674,6 +3711,9 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
36743711

36753712
registry->Register(GetFormatOfExtensionlessFile);
36763713
registry->Register(Close);
3714+
registry->Register(CloseSync);
3715+
registry->Register(FastCloseSync);
3716+
registry->Register(fast_close_sync_.GetTypeInfo());
36773717
registry->Register(ExistsSync);
36783718
registry->Register(Open);
36793719
registry->Register(OpenFileHandle);

‎test/parallel/test-fs-close-fast-api.js

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fs = require('fs');
5+
6+
// A large number is selected to ensure that the fast path is called.
7+
// Ref: https://github.com/nodejs/node/issues/53902
8+
for (let i = 0; i < 100_000; i++) {
9+
try {
10+
fs.closeSync(100);
11+
} catch (e) {
12+
// Ignore all EBADF errors.
13+
// EBADF stands for "Bad file descriptor".
14+
if (e.code !== 'EBADF') {
15+
throw e;
16+
}
17+
}
18+
}
19+
20+
// This test is to ensure that fs.close() does not crash under pressure.
21+
for (let i = 0; i < 100_000; i++) {
22+
fs.close(100, common.mustCall());
23+
}
24+
25+
// This test is to ensure that fs.readFile() does not crash.
26+
// readFile() calls fs.close() internally if the input is not a file descriptor.
27+
// Large number is selected to ensure that the fast path is called.
28+
// Ref: https://github.com/nodejs/node/issues/53902
29+
for (let i = 0; i < 100_000; i++) {
30+
fs.readFile(__filename, common.mustCall());
31+
}

‎typings/internalBinding/fs.d.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ declare namespace InternalFSBinding {
7070
function chown(path: string, uid: number, gid: number): void;
7171

7272
function close(fd: number, req: FSReqCallback): void;
73-
function close(fd: number): void;
73+
function closeSync(fd: number): void;
7474

7575
function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: FSReqCallback): void;
7676
function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: undefined, ctx: FSSyncContext): void;
@@ -258,6 +258,7 @@ export interface FsBinding {
258258
chmod: typeof InternalFSBinding.chmod;
259259
chown: typeof InternalFSBinding.chown;
260260
close: typeof InternalFSBinding.close;
261+
closeSync: typeof InternalFSBinding.closeSync;
261262
copyFile: typeof InternalFSBinding.copyFile;
262263
cpSyncCheckPaths: typeof InternalFSBinding.cpSyncCheckPaths;
263264
fchmod: typeof InternalFSBinding.fchmod;

0 commit comments

Comments
 (0)