Skip to content

Commit

Permalink
src: change how fd counter is done
Browse files Browse the repository at this point in the history
Because of a recent change in Node.js it is necessary to move where
open/close fd's are tracked. Also update the test to check additional
cases, and remove the check for GC close cleanup since it's deprecated.
  • Loading branch information
trevnorris committed Nov 27, 2023
1 parent 1826278 commit 8b04b74
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 43 deletions.
6 changes: 0 additions & 6 deletions src/node_file-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -352,12 +352,6 @@ int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
ctx_obj->Set(context,
env->syscall_string(),
OneByteString(isolate, syscall)).Check();
} else {
if (strncmp(syscall, "open", 4) == 0) {
env->envinst_->inc_fs_handles_opened();
} else if (strncmp(syscall, "close", 5) == 0) {
env->envinst_->inc_fs_handles_closed();
}
}
return err;
}
Expand Down
3 changes: 3 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,7 @@ void Close(const FunctionCallbackInfo<Value>& args) {
FS_SYNC_TRACE_BEGIN(close);
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd);
FS_SYNC_TRACE_END(close);
env->envinst_->inc_fs_handles_closed();
}
}

Expand Down Expand Up @@ -2038,6 +2039,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
FS_SYNC_TRACE_END(open);
if (is_uv_error(result)) return;
env->AddUnmanagedFd(result);
env->envinst_->inc_fs_handles_opened();
args.GetReturnValue().Set(result);
}
}
Expand Down Expand Up @@ -2079,6 +2081,7 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
}
FileHandle* fd = FileHandle::New(binding_data, result);
if (fd == nullptr) return;
env->envinst_->inc_fs_handles_opened();
args.GetReturnValue().Set(fd->object());
}
}
Expand Down
75 changes: 38 additions & 37 deletions test/parallel/test-nsolid-file-handle-count.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ const fs = require('fs');
const baseOpened = nsolid.metrics().fsHandlesOpenedCount;
const baseClosed = nsolid.metrics().fsHandlesClosedCount;

let oCntr = 0;
let cCntr = 0;

function getOpened() {
return nsolid.metrics().fsHandlesOpenedCount - baseOpened;
}
Expand All @@ -23,54 +26,52 @@ try {
// It's meant to throw and the exception is to be ignored.
}

assert.strictEqual(getOpened(), 0);
assert.strictEqual(getClosed(), 0);
assert.strictEqual(getOpened(), oCntr);
assert.strictEqual(getClosed(), cCntr);

const fd = fs.openSync(__filename);
assert.strictEqual(getOpened(), 1);
assert.strictEqual(getClosed(), 0);
assert.strictEqual(getOpened(), ++oCntr);
assert.strictEqual(getClosed(), cCntr);

fs.closeSync(fd);
assert.strictEqual(getOpened(), 1);
assert.strictEqual(getClosed(), 1);
assert.strictEqual(getOpened(), oCntr);
assert.strictEqual(getClosed(), ++cCntr);

fs.readFileSync(__filename);
assert.strictEqual(getOpened(), ++oCntr);
assert.strictEqual(getClosed(), ++cCntr);

fs.open(__filename, common.mustCall((err, fd) => {
assert.ok(!err);
assert.strictEqual(getOpened(), 2);
assert.strictEqual(getClosed(), 1);
fs.readFile(__filename, () => {
assert.strictEqual(getOpened(), ++oCntr);
assert.strictEqual(getClosed(), ++cCntr);

fs.close(fd, common.mustCall((err) => {
fs.open(__filename, common.mustCall((err, fd) => {
assert.ok(!err);
assert.strictEqual(getOpened(), 2);
assert.strictEqual(getClosed(), 2);

checkPromise().then(common.mustCall(() => {
openFileHandle();
setTimeout(() => {
// The FileHandle should be out-of-scope and no longer accessed now.
global.gc();
setImmediate(() => {
assert.strictEqual(getOpened(), 4);
assert.strictEqual(getClosed(), 4);
});
}, 100);
})).catch(common.mustNotCall());
assert.strictEqual(getOpened(), ++oCntr);
assert.strictEqual(getClosed(), cCntr);

fs.close(fd, common.mustCall((err) => {
assert.ok(!err);
assert.strictEqual(getOpened(), oCntr);
assert.strictEqual(getClosed(), ++cCntr);

checkPromise()
.then(common.mustCall(closePromiseFd))
.catch(common.mustNotCall());
}));
}));
}));
});

async function checkPromise() {
let fh = await fs.promises.open(__filename);

Check failure on line 66 in test/parallel/test-nsolid-file-handle-count.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

'fh' is never reassigned. Use 'const' instead
assert.strictEqual(getOpened(), 3);
assert.strictEqual(getClosed(), 2);
fh = await fh.close();
assert.strictEqual(fh, undefined);
assert.strictEqual(getOpened(), 3);
assert.strictEqual(getClosed(), 3);
assert.strictEqual(getOpened(), ++oCntr);
assert.strictEqual(getClosed(), cCntr);
return fh;
}

async function openFileHandle() {
const fh = await fs.promises.open(__filename);
console.log(fh);
assert.strictEqual(getOpened(), 4);
assert.strictEqual(getClosed(), 3);
async function closePromiseFd(fh) {
fh = await fh.close();
assert.strictEqual(fh, undefined);
assert.strictEqual(getOpened(), oCntr);
assert.strictEqual(getClosed(), ++cCntr);
}

0 comments on commit 8b04b74

Please sign in to comment.