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: #42829

PR-URL: #42910
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
santigimeno authored and targos committed Jul 12, 2022
1 parent 5c0a24d commit df50131
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/node_file-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,10 @@ 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()->is_stopping()) CHECK(finished_);
}

template <typename AliasedBufferT>
Expand Down
5 changes: 5 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ 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 +651,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
51 changes: 51 additions & 0 deletions test/parallel/test-worker-fshandles-error-on-termination.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

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

const MAX_ITERATIONS = 20;
const MAX_THREADS = 10;

// Do not use isMainThread so that this test itself can be run inside a Worker.
if (!process.env.HAS_STARTED_WORKER) {
process.env.HAS_STARTED_WORKER = 1;

function spinWorker(iter) {
const w = new Worker(__filename);
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 {
async function open_nok() {
await assert.rejects(
fs.open('this file does not exist'),
{
code: 'ENOENT',
syscall: 'open'
}
);
await scheduler.yield();
await open_nok();
}

// These async function calls never return as they are meant to continually
// open nonexistent files until the worker is terminated.
open_nok();
open_nok();

parentPort.postMessage('terminate');
}
46 changes: 46 additions & 0 deletions test/parallel/test-worker-fshandles-open-close-on-termination.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

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

const MAX_ITERATIONS = 20;
const MAX_THREADS = 10;

// Do not use isMainThread so that this test itself can be run inside a Worker.
if (!process.env.HAS_STARTED_WORKER) {
process.env.HAS_STARTED_WORKER = 1;

function spinWorker(iter) {
const w = new Worker(__filename);
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 {
async function open_close() {
const fh = await fs.open(__filename);
await fh.close();
await scheduler.yield();
await open_close();
}

// These async function calls never return as they are meant to continually
// open and close files until the worker is terminated.
open_close();
open_close();

parentPort.postMessage('terminate');
}

0 comments on commit df50131

Please sign in to comment.