diff --git a/src/node_dir.cc b/src/node_dir.cc index eca5140b642..ca7fc340bcd 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -248,22 +248,14 @@ static void AfterDirRead(uv_fs_t* req) { uv_dir_t* dir = static_cast(req->ptr); - TryCatch try_catch(isolate); - Local js_array; - if (!DirentListToArray(env, - dir->dirents, - static_cast(req->result), - req_wrap->encoding()) - .ToLocal(&js_array)) { + ResolveOrReject(req_wrap.get(), [&]() { + MaybeLocal js_array = DirentListToArray( + env, dir->dirents, static_cast(req->result), req_wrap->encoding()); // Clear libuv resources *before* delivering results to JS land because - // that can schedule another operation on the same uv_dir_t. Ditto below. + // that can schedule another operation on the same uv_dir_t. after.Clear(); - CHECK(try_catch.CanContinue()); - return req_wrap->Reject(try_catch.Exception()); - } - - after.Clear(); - req_wrap->Resolve(js_array); + return js_array; + }); } diff --git a/src/node_file-inl.h b/src/node_file-inl.h index ebd97c1c8c5..5a32d002490 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -413,6 +413,29 @@ int SyncCallAndThrowOnError(Environment* env, return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...); } +// Delivers the value produced by `produce`, or the exception it threw, to +// `req_wrap`. The TryCatch is left before calling into JS: exceptions thrown +// by the callback, or by the tick queue drained afterwards, must not be caught +// by it. +template +void ResolveOrReject(FSReqBase* req_wrap, Fn&& produce) { + v8::Isolate* isolate = req_wrap->env()->isolate(); + v8::Local value; + v8::Local error; + { + v8::TryCatch try_catch(isolate); + if (!produce().ToLocal(&value)) { + CHECK(try_catch.CanContinue()); + error = try_catch.Exception(); + } + } + if (error.IsEmpty()) { + req_wrap->Resolve(value); + } else { + req_wrap->Reject(error); + } +} + } // namespace fs } // namespace node diff --git a/src/node_file.cc b/src/node_file.cc index b624e9da41b..8610a04509f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -900,16 +900,10 @@ void AfterMkdirp(uv_fs_t* req) { std::string first_path(req_wrap->continuation_data()->first_path()); if (first_path.empty()) return req_wrap->Resolve(Undefined(req_wrap->env()->isolate())); - Local path; - TryCatch try_catch(req_wrap->env()->isolate()); - if (!StringBytes::Encode(req_wrap->env()->isolate(), - first_path.c_str(), - req_wrap->encoding()) - .ToLocal(&path)) { - CHECK(try_catch.CanContinue()); - return req_wrap->Reject(try_catch.Exception()); - } - return req_wrap->Resolve(path); + ResolveOrReject(req_wrap, [&]() { + return StringBytes::Encode( + req_wrap->env()->isolate(), first_path.c_str(), req_wrap->encoding()); + }); } } @@ -918,19 +912,11 @@ void AfterStringPath(uv_fs_t* req) { FSReqAfterScope after(req_wrap, req); FS_ASYNC_TRACE_END1( req->fs_type, req_wrap, "result", static_cast(req->result)) - MaybeLocal link; - if (after.Proceed()) { - TryCatch try_catch(req_wrap->env()->isolate()); - link = StringBytes::Encode( - req_wrap->env()->isolate(), req->path, req_wrap->encoding()); - if (link.IsEmpty()) { - CHECK(try_catch.CanContinue()); - req_wrap->Reject(try_catch.Exception()); - } else { - Local val; - if (link.ToLocal(&val)) req_wrap->Resolve(val); - } + ResolveOrReject(req_wrap, [&]() { + return StringBytes::Encode( + req_wrap->env()->isolate(), req->path, req_wrap->encoding()); + }); } } @@ -939,20 +925,12 @@ void AfterStringPtr(uv_fs_t* req) { FSReqAfterScope after(req_wrap, req); FS_ASYNC_TRACE_END1( req->fs_type, req_wrap, "result", static_cast(req->result)) - MaybeLocal link; - if (after.Proceed()) { - TryCatch try_catch(req_wrap->env()->isolate()); - link = StringBytes::Encode(req_wrap->env()->isolate(), - static_cast(req->ptr), - req_wrap->encoding()); - if (link.IsEmpty()) { - CHECK(try_catch.CanContinue()); - req_wrap->Reject(try_catch.Exception()); - } else { - Local val; - if (link.ToLocal(&val)) req_wrap->Resolve(val); - } + ResolveOrReject(req_wrap, [&]() { + return StringBytes::Encode(req_wrap->env()->isolate(), + static_cast(req->ptr), + req_wrap->encoding()); + }); } } @@ -2700,14 +2678,9 @@ class ReadDirRecursiveRequest { walk_.error_path().c_str())); } - Local value; - TryCatch try_catch(isolate); - if (!MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_) - .ToLocal(&value)) { - CHECK(try_catch.CanContinue()); - return req_wrap->Reject(try_catch.Exception()); - } - req_wrap->Resolve(value); + ResolveOrReject(req_wrap.get(), [&]() { + return MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_); + }); } private: diff --git a/test/parallel/test-fs-callback-throw.js b/test/parallel/test-fs-callback-throw.js new file mode 100644 index 00000000000..b35f33ccfe4 --- /dev/null +++ b/test/parallel/test-fs-callback-throw.js @@ -0,0 +1,62 @@ +'use strict'; +// Refs: https://github.com/nodejs/node/issues/65667 +// Exceptions thrown after an fs operation completes must reach +// 'uncaughtException' instead of being swallowed. +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); + +tmpdir.refresh(); +const dir = tmpdir.path; +const file = path.join(dir, 'file'); +const link = path.join(dir, 'link'); +fs.writeFileSync(file, ''); + +// The callback throws. +const callbackCases = { + 'mkdtemp': (cb) => fs.mkdtemp(path.join(dir, 'x-'), cb), + 'realpath.native': (cb) => fs.realpath.native(dir, cb), + 'mkdir recursive': (cb) => fs.mkdir(path.join(dir, 'a', 'b'), { recursive: true }, cb), + 'readdir recursive': (cb) => fs.readdir(dir, { recursive: true }, cb), +}; +if (common.canCreateSymLink()) { + fs.symlinkSync(file, link); + callbackCases.readlink = (cb) => fs.readlink(link, cb); +} + +// A nextTick callback scheduled after the promise settles throws. +const promiseCases = { + 'promises.mkdtemp': () => fs.promises.mkdtemp(path.join(dir, 'p-')), + 'dir.read': async () => { + const d = await fs.promises.opendir(dir); + await d.read(); + process.nextTick(() => d.closeSync()); + }, +}; + +const cases = [ + ...Object.entries(callbackCases).map(([name, run]) => [name, () => { + run(common.mustSucceed(() => { throw new Error(name); })); + }]), + ...Object.entries(promiseCases).map(([name, run]) => [name, async () => { + await run(); + process.nextTick(() => { throw new Error(name); }); + }]), +]; + +let current; +process.on('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, current); + next(); +}, cases.length)); + +function next() { + const entry = cases.shift(); + if (entry === undefined) return; + current = entry[0]; + entry[1](); +} + +next();