From b6005b3faccee90b200a613e1db4fb79ed800af4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Dec 2024 17:12:50 +0100 Subject: [PATCH] module: mark evaluation rejection in require(esm) as handled Previously the implemention of require(esm) only converted the rejected promise from module evaluation into an error, but the rejected promise was still treated as a pending unhandled rejection by the promise rejection callback, because the promise is created by V8 internals and we don't get a chance to mark it as handled, so the rejection incorrectly marked as unhandled would still go through unhandled rejection handling (if no global listener is set, the default handling would print a warning and make the Node.js instance exit with 1). This patch fixes it by calling into the JS promise rejection callback to mark the evalaution rejection handled so that it doesn't go through unhandled rejection handling. PR-URL: https://github.com/nodejs/node/pull/56122 Fixes: https://github.com/nodejs/node/issues/56115 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow --- src/module_wrap.cc | 16 ++++++++++++++ .../test-require-module-error-catching.js | 21 +++++++++++++++++++ ...e-module-synchronous-rejection-handling.js | 13 ++++++++++++ .../es-modules/reference-error-esm.js | 5 +++++ test/fixtures/es-modules/runtime-error-esm.js | 2 ++ .../es-modules/synchronous-rejection-esm.js | 2 ++ 6 files changed, 59 insertions(+) create mode 100644 test/es-module/test-require-module-error-catching.js create mode 100644 test/es-module/test-require-module-synchronous-rejection-handling.js create mode 100644 test/fixtures/es-modules/reference-error-esm.js create mode 100644 test/fixtures/es-modules/runtime-error-esm.js create mode 100644 test/fixtures/es-modules/synchronous-rejection-esm.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 6f8c9d10955b88..8be6dbd1d0262c 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -663,6 +663,22 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { CHECK(result->IsPromise()); Local promise = result.As(); if (promise->State() == Promise::PromiseState::kRejected) { + // The rejected promise is created by V8, so we don't get a chance to mark + // it as resolved before the rejection happens from evaluation. But we can + // tell the promise rejection callback to treat it as a promise rejected + // before handler was added which would remove it from the unhandled + // rejection handling, since we are converting it into an error and throw + // from here directly. + Local type = v8::Integer::New( + isolate, + static_cast( + v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject)); + Local args[] = {type, promise, Undefined(isolate)}; + if (env->promise_reject_callback() + ->Call(context, Undefined(isolate), arraysize(args), args) + .IsEmpty()) { + return; + } Local exception = promise->Result(); Local message = v8::Exception::CreateMessage(isolate, exception); diff --git a/test/es-module/test-require-module-error-catching.js b/test/es-module/test-require-module-error-catching.js new file mode 100644 index 00000000000000..c314513d9bbf04 --- /dev/null +++ b/test/es-module/test-require-module-error-catching.js @@ -0,0 +1,21 @@ +// This tests synchronous errors in ESM from require(esm) can be caught. + +'use strict'; + +require('../common'); +const assert = require('assert'); + +// Runtime errors from throw should be caught. +assert.throws(() => { + require('../fixtures/es-modules/runtime-error-esm.js'); +}, { + message: 'hello' +}); + +// References errors should be caught too. +assert.throws(() => { + require('../fixtures/es-modules/reference-error-esm.js'); +}, { + name: 'ReferenceError', + message: 'exports is not defined' +}); diff --git a/test/es-module/test-require-module-synchronous-rejection-handling.js b/test/es-module/test-require-module-synchronous-rejection-handling.js new file mode 100644 index 00000000000000..76a25e742bb900 --- /dev/null +++ b/test/es-module/test-require-module-synchronous-rejection-handling.js @@ -0,0 +1,13 @@ +// This synchronous rejections from require(esm) still go to the unhandled rejection +// handler. + +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +process.on('unhandledRejection', common.mustCall((reason, promise) => { + assert.strictEqual(reason, 'reject!'); +})); + +require('../fixtures/es-modules/synchronous-rejection-esm.js'); diff --git a/test/fixtures/es-modules/reference-error-esm.js b/test/fixtures/es-modules/reference-error-esm.js new file mode 100644 index 00000000000000..baf773c78970ad --- /dev/null +++ b/test/fixtures/es-modules/reference-error-esm.js @@ -0,0 +1,5 @@ +// This module is invalid in both ESM and CJS, because +// 'exports' are not defined in ESM, while require cannot be +// redeclared in CJS. +Object.defineProperty(exports, "__esModule", { value: true }); +const require = () => {}; diff --git a/test/fixtures/es-modules/runtime-error-esm.js b/test/fixtures/es-modules/runtime-error-esm.js new file mode 100644 index 00000000000000..1df3cc469adbfc --- /dev/null +++ b/test/fixtures/es-modules/runtime-error-esm.js @@ -0,0 +1,2 @@ +import 'node:fs'; // Forces it to be recognized as ESM. +throw new Error('hello'); diff --git a/test/fixtures/es-modules/synchronous-rejection-esm.js b/test/fixtures/es-modules/synchronous-rejection-esm.js new file mode 100644 index 00000000000000..34d066037e2140 --- /dev/null +++ b/test/fixtures/es-modules/synchronous-rejection-esm.js @@ -0,0 +1,2 @@ +import 'node:fs'; // Forces it to be recognized as ESM. +Promise.reject('reject!');