Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 3d89e6b

Browse filesBrowse files
joyeecheungmarco-ippolito
joyeecheung
authored and
marco-ippolito
committed
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: #56122 Backport-PR-URL: #56927 Fixes: #56115 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Refs: #52697
1 parent e01dd4b commit 3d89e6b
Copy full SHA for 3d89e6b

File tree

Expand file treeCollapse file tree

6 files changed

+59
-0
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+59
-0
lines changed
Open diff view settings
Collapse file

‎src/module_wrap.cc‎

Copy file name to clipboardExpand all lines: src/module_wrap.cc
+16Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,22 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
609609
CHECK(result->IsPromise());
610610
Local<Promise> promise = result.As<Promise>();
611611
if (promise->State() == Promise::PromiseState::kRejected) {
612+
// The rejected promise is created by V8, so we don't get a chance to mark
613+
// it as resolved before the rejection happens from evaluation. But we can
614+
// tell the promise rejection callback to treat it as a promise rejected
615+
// before handler was added which would remove it from the unhandled
616+
// rejection handling, since we are converting it into an error and throw
617+
// from here directly.
618+
Local<Value> type = v8::Integer::New(
619+
isolate,
620+
static_cast<int32_t>(
621+
v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject));
622+
Local<Value> args[] = {type, promise, Undefined(isolate)};
623+
if (env->promise_reject_callback()
624+
->Call(context, Undefined(isolate), arraysize(args), args)
625+
.IsEmpty()) {
626+
return;
627+
}
612628
Local<Value> exception = promise->Result();
613629
Local<v8::Message> message =
614630
v8::Exception::CreateMessage(isolate, exception);
Collapse file

‎test/es-module/test-require-module-error-catching.js‎

Copy file name to clipboard
+21Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// This tests synchronous errors in ESM from require(esm) can be caught.
2+
3+
'use strict';
4+
5+
require('../common');
6+
const assert = require('assert');
7+
8+
// Runtime errors from throw should be caught.
9+
assert.throws(() => {
10+
require('../fixtures/es-modules/runtime-error-esm.js');
11+
}, {
12+
message: 'hello'
13+
});
14+
15+
// References errors should be caught too.
16+
assert.throws(() => {
17+
require('../fixtures/es-modules/reference-error-esm.js');
18+
}, {
19+
name: 'ReferenceError',
20+
message: 'exports is not defined'
21+
});
Collapse file

‎test/es-module/test-require-module-synchronous-rejection-handling.js‎

Copy file name to clipboard
+13Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// This synchronous rejections from require(esm) still go to the unhandled rejection
2+
// handler.
3+
4+
'use strict';
5+
6+
const common = require('../common');
7+
const assert = require('assert');
8+
9+
process.on('unhandledRejection', common.mustCall((reason, promise) => {
10+
assert.strictEqual(reason, 'reject!');
11+
}));
12+
13+
require('../fixtures/es-modules/synchronous-rejection-esm.js');
Collapse file

‎test/fixtures/es-modules/reference-error-esm.js‎

Copy file name to clipboard
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// This module is invalid in both ESM and CJS, because
2+
// 'exports' are not defined in ESM, while require cannot be
3+
// redeclared in CJS.
4+
Object.defineProperty(exports, "__esModule", { value: true });
5+
const require = () => {};
Collapse file

‎test/fixtures/es-modules/runtime-error-esm.js‎

Copy file name to clipboard
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
import 'node:fs'; // Forces it to be recognized as ESM.
2+
throw new Error('hello');
Collapse file

‎test/fixtures/es-modules/synchronous-rejection-esm.js‎

Copy file name to clipboard
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
import 'node:fs'; // Forces it to be recognized as ESM.
2+
Promise.reject('reject!');

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.