Skip to content

Commit

Permalink
fs: don't end fs promises on Isolate termination
Browse files Browse the repository at this point in the history
This is specially prevalent in the case of having in-progress FileHandle
operations in a worker thread while the Worker is exiting.

Fixes: nodejs#42829
  • Loading branch information
santigimeno committed May 4, 2022
1 parent c3aa86d commit 3e58881
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 2 deletions.
7 changes: 5 additions & 2 deletions src/node_file-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,11 @@ FSReqPromise<AliasedBufferT>::New(BindingData* binding_data,

template <typename AliasedBufferT>
FSReqPromise<AliasedBufferT>::~FSReqPromise() {
// Validate that the promise was explicitly resolved or rejected.
CHECK(finished_);
// Validate that the promise was explicitly resolved or rejected but only if
// the Isolate is not terminating because in this case the promise might have
// not finished.
if (env()->can_call_into_js())
CHECK(finished_);
}

template <typename AliasedBufferT>
Expand Down
6 changes: 6 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,8 @@ MaybeLocal<Promise> FileHandle::ClosePromise() {
std::unique_ptr<CloseReq> close(CloseReq::from_req(req));
CHECK_NOT_NULL(close);
close->file_handle()->AfterClose();
if (!close->env()->can_call_into_js())
return;
Isolate* isolate = close->env()->isolate();
if (req->result < 0) {
HandleScope handle_scope(isolate);
Expand Down Expand Up @@ -650,6 +652,10 @@ void FSReqAfterScope::Reject(uv_fs_t* req) {
}

bool FSReqAfterScope::Proceed() {
if (!wrap_->env()->can_call_into_js()) {
return false;
}

if (req_->result < 0) {
Reject(req_);
return false;
Expand Down
55 changes: 55 additions & 0 deletions test/parallel/test-worker-fshandles-closed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const {
setImmediate,
} = require('timers/promises');
const { isMainThread, parentPort, Worker } = require('worker_threads');

const MAX_ITERATIONS = 20;
const MAX_THREADS = 10;

if (isMainThread) {
function spinWorker(iter) {
const w = new Worker(__filename, { workerData: iter });
w.on('message', common.mustCall((msg) => {
assert.strictEqual(msg, 'terminate');
w.terminate();
}));

w.on('exit', common.mustCall(() => {
if (iter < MAX_ITERATIONS)
spinWorker(++iter);
}));
}

for (let i = 0; i < MAX_THREADS; i++) {
spinWorker(0);
}
} else {
const { promises } = require('fs');
async function open_close_ok() {
const fh = await promises.open(__filename);
await fh.close();
await setImmediate();
await open_close_ok();
}

async function open_close_nok() {
await assert.rejects(
promises.open('this file does not exist'),
{
code: 'ENOENT',
syscall: 'open'
}
);
await setImmediate();
await open_close_nok();
}

open_close_ok();
open_close_nok();

parentPort.postMessage('terminate');
}

0 comments on commit 3e58881

Please sign in to comment.