Skip to content

Commit f256361

Browse files
authored
fix(ext/node): fire uncaughtExceptionMonitor with correct origin for sync top-level throws (#34048)
## Summary `process.on('uncaughtExceptionMonitor', handler)` was firing with `origin === 'unhandledRejection'` instead of `'uncaughtException'` for synchronous top-level throws in CommonJS entry modules. Root cause: in Deno, a synchronous top-level throw in a CJS entry surfaces as a module-evaluation rejection (the entry CJS module is loaded through an ESM wrapper generated by `node_resolver::analyze`). Without intervention, the rejection is routed through Deno's unhandled-rejection path, so the monitor/uncaughtException listeners see the wrong origin. ## Fix - `ext/node/polyfills/01_require.js`: catch the sync entry-module throw in `Module._load` and invoke `process._fatalException` up-front so the monitor and `uncaughtException` listeners see `origin === 'uncaughtException'`, matching Node.js semantics. The error is marked in a WeakSet so the unhandled-rejection fallback in `process.ts` won't re-fire when the rejection arrives. - `ext/node/polyfills/process.ts`: - Track `uncaughtExceptionMonitor` listener count so the monitor still fires when no `uncaughtException`/`unhandledRejection` listener is registered. - Only `preventDefault()` on the unhandled-rejection event when a registered handler actually consumed the error (so unhandled cases still terminate). ## Tests New spec test at `tests/specs/node/uncaught_exception_monitor/`: - `entry.cjs`: verifies monitor fires with `origin === 'uncaughtException'` for a sync top-level throw. - `rejection.cjs`: verifies monitor fires with `origin === 'unhandledRejection'` for an actual unhandled promise rejection. ## Test plan - [x] `cargo test -p deno --test integration_tests specs::node::uncaught_exception_monitor` - [x] Verified `node:test` and other existing process-event tests still pass. Closes denoland/orchid#54 This replaces #34040 (that PR had unrelated commits merged in from main; this branch contains only the two fix commits).
1 parent e6c548d commit f256361

7 files changed

Lines changed: 187 additions & 29 deletions

File tree

ext/node/polyfills/01_require.js

Lines changed: 54 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1663,32 +1663,65 @@ Module._load = function (request, parent, isMain) {
16631663

16641664
let threw = true;
16651665
try {
1666-
module.load(filename);
1667-
threw = false;
1668-
} finally {
1669-
if (threw) {
1670-
delete Module._cache[filename];
1671-
if (parent !== undefined) {
1672-
delete relativeResolveCache[relResolveCacheIdentifier];
1673-
const children = parent?.children;
1674-
if (ArrayIsArray(children)) {
1675-
const index = ArrayPrototypeIndexOf(children, module);
1676-
if (index !== -1) {
1677-
ArrayPrototypeSplice(children, index, 1);
1666+
try {
1667+
module.load(filename);
1668+
threw = false;
1669+
} finally {
1670+
if (threw) {
1671+
delete Module._cache[filename];
1672+
if (parent !== undefined) {
1673+
delete relativeResolveCache[relResolveCacheIdentifier];
1674+
const children = parent?.children;
1675+
if (ArrayIsArray(children)) {
1676+
const index = ArrayPrototypeIndexOf(children, module);
1677+
if (index !== -1) {
1678+
ArrayPrototypeSplice(children, index, 1);
1679+
}
16781680
}
16791681
}
1682+
} else if (
1683+
module.exports &&
1684+
// Skip Proxy module.exports so the cleanup pass after a circular
1685+
// require doesn't invoke user-visible getPrototypeOf traps. Matches
1686+
// Node's lib/internal/modules/cjs/loader.js behavior.
1687+
!core.isProxy(module.exports) &&
1688+
ObjectGetPrototypeOf(module.exports) ===
1689+
CircularRequirePrototypeWarningProxy
1690+
) {
1691+
ObjectSetPrototypeOf(module.exports, ObjectPrototype);
16801692
}
1681-
} else if (
1682-
module.exports &&
1683-
// Skip Proxy module.exports so the cleanup pass after a circular
1684-
// require doesn't invoke user-visible getPrototypeOf traps. Matches
1685-
// Node's lib/internal/modules/cjs/loader.js behavior.
1686-
!core.isProxy(module.exports) &&
1687-
ObjectGetPrototypeOf(module.exports) ===
1688-
CircularRequirePrototypeWarningProxy
1693+
}
1694+
} catch (err) {
1695+
// For a top-level CommonJS throw in the entry module, fire
1696+
// 'uncaughtExceptionMonitor' and 'uncaughtException' synchronously with
1697+
// origin === 'uncaughtException', matching Node.js semantics.
1698+
//
1699+
// Without this, the throw bubbles up to the ESM wrapper that loads the
1700+
// main CJS module (generated by node_resolver::analyze), becomes a
1701+
// module evaluation rejection, and is routed through Deno's unhandled-
1702+
// rejection path -- which emits with origin === 'unhandledRejection'.
1703+
if (
1704+
isMain &&
1705+
parent === null &&
1706+
typeof process !== "undefined" &&
1707+
typeof process._fatalException === "function"
16891708
) {
1690-
ObjectSetPrototypeOf(module.exports, ObjectPrototype);
1709+
if (process._fatalException(err)) {
1710+
// Handled by a registered 'uncaughtException' listener or an
1711+
// 'uncaughtException' capture callback. Treat the load as complete
1712+
// so the ESM wrapper module evaluation succeeds and the runtime can
1713+
// continue running pending callbacks.
1714+
return module.exports;
1715+
}
1716+
// Not handled. Mark the error so the unhandled-rejection fallback in
1717+
// process.ts skips a redundant second emit of monitor/uncaughtException
1718+
// when the re-thrown error surfaces as a module-evaluation rejection.
1719+
if (err !== null && typeof err === "object") {
1720+
const set = internals._dispatchedFatalErrors;
1721+
if (set !== undefined) set.add(err);
1722+
}
16911723
}
1724+
throw err;
16921725
}
16931726

16941727
return module.exports;

ext/node/polyfills/process.ts

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -539,16 +539,27 @@ export { getegid, geteuid, getgid, getuid, setegid, seteuid, setgid, setuid };
539539

540540
const ALLOWED_FLAGS = buildAllowedFlags();
541541

542+
// Tracks error values for which the synchronous Module._load entry-module
543+
// path in 01_require.js has already invoked process._fatalException. When
544+
// the same error is later re-thrown and surfaces as a module-evaluation
545+
// rejection (via the ESM wrapper that loads the main CJS module), the
546+
// unhandled-rejection fallback below uses this set to skip emitting
547+
// 'uncaughtExceptionMonitor' / 'uncaughtException' a second time.
542548
// deno-lint-ignore no-explicit-any
543-
function uncaughtExceptionHandler(err: any, origin: string) {
549+
const _dispatchedFatalErrors = new WeakSet<any>();
550+
internals._dispatchedFatalErrors = _dispatchedFatalErrors;
551+
552+
// deno-lint-ignore no-explicit-any
553+
function uncaughtExceptionHandler(err: any, origin: string): boolean {
544554
// The origin parameter can be 'unhandledRejection' or 'uncaughtException'
545555
// depending on how the uncaught exception was created. In Node.js,
546556
// exceptions thrown from the top level of a CommonJS module are reported as
547557
// 'uncaughtException', while exceptions thrown from the top level of an ESM
548558
// module are reported as 'unhandledRejection'. Deno does not have a true
549-
// CommonJS implementation, so all exceptions thrown from the top level are
550-
// reported as 'uncaughtException'.
551-
process._fatalException(err, origin === "unhandledRejection");
559+
// CommonJS implementation; sync throws in the entry CJS module are
560+
// dispatched up-front via Module._load (see ext/node/polyfills/01_require.js)
561+
// so this path only fires for real unhandled promise rejections.
562+
return process._fatalException(err, origin === "unhandledRejection");
552563
}
553564

554565
export let execPath: string = "";
@@ -1297,6 +1308,7 @@ export const removeAllListeners = process.removeAllListeners;
12971308
let unhandledRejectionListenerCount = 0;
12981309
let rejectionHandledListenerCount = 0;
12991310
let uncaughtExceptionListenerCount = 0;
1311+
let uncaughtExceptionMonitorListenerCount = 0;
13001312
let beforeExitListenerCount = 0;
13011313
let exitListenerCount = 0;
13021314

@@ -1311,6 +1323,9 @@ process.on("newListener", (event: string) => {
13111323
case "uncaughtException":
13121324
uncaughtExceptionListenerCount++;
13131325
break;
1326+
case "uncaughtExceptionMonitor":
1327+
uncaughtExceptionMonitorListenerCount++;
1328+
break;
13141329
case "beforeExit":
13151330
beforeExitListenerCount++;
13161331
break;
@@ -1334,6 +1349,9 @@ process.on("removeListener", (event: string) => {
13341349
case "uncaughtException":
13351350
uncaughtExceptionListenerCount--;
13361351
break;
1352+
case "uncaughtExceptionMonitor":
1353+
uncaughtExceptionMonitorListenerCount--;
1354+
break;
13371355
case "beforeExit":
13381356
beforeExitListenerCount--;
13391357
break;
@@ -1388,6 +1406,7 @@ function synchronizeListeners() {
13881406
if (
13891407
unhandledRejectionListenerCount > 0 ||
13901408
uncaughtExceptionListenerCount > 0 ||
1409+
uncaughtExceptionMonitorListenerCount > 0 ||
13911410
_uncaughtExceptionCaptureFn !== null
13921411
) {
13931412
internals.nodeProcessUnhandledRejectionCallback = (event) => {
@@ -1396,9 +1415,20 @@ function synchronizeListeners() {
13961415
// an unhandled rejection occurs and there are no unhandledRejection
13971416
// listeners.
13981417

1399-
event.preventDefault();
1400-
14011418
let reason = event.reason;
1419+
1420+
// The synchronous Module._load path in 01_require.js already invoked
1421+
// process._fatalException for this error. Re-firing here would
1422+
// double-emit 'uncaughtExceptionMonitor' (and 'uncaughtException')
1423+
// for the same value. Skip and let Deno's default unhandled-rejection
1424+
// handling print the error and terminate the runtime.
1425+
if (
1426+
reason !== null && typeof reason === "object" &&
1427+
_dispatchedFatalErrors.has(reason)
1428+
) {
1429+
return;
1430+
}
1431+
14021432
// If the rejection reason is not an Error, wrap it in an
14031433
// ERR_UNHANDLED_REJECTION error, matching Node.js behavior.
14041434
if (!(reason instanceof Error)) {
@@ -1419,7 +1449,12 @@ function synchronizeListeners() {
14191449
reason = err;
14201450
}
14211451

1422-
uncaughtExceptionHandler(reason, "unhandledRejection");
1452+
// Only preventDefault if a registered handler (uncaughtException
1453+
// listener or capture callback) actually consumed the error.
1454+
// Otherwise we want the runtime to terminate normally.
1455+
if (uncaughtExceptionHandler(reason, "unhandledRejection")) {
1456+
event.preventDefault();
1457+
}
14231458
return;
14241459
}
14251460

@@ -1441,7 +1476,9 @@ function synchronizeListeners() {
14411476
}
14421477

14431478
if (
1444-
uncaughtExceptionListenerCount > 0 || _uncaughtExceptionCaptureFn !== null
1479+
uncaughtExceptionListenerCount > 0 ||
1480+
uncaughtExceptionMonitorListenerCount > 0 ||
1481+
_uncaughtExceptionCaptureFn !== null
14451482
) {
14461483
globalThis.addEventListener("error", processOnError);
14471484
} else {
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
{
2+
"tempDir": true,
3+
"tests": {
4+
"sync_top_level_cjs_throw": {
5+
"args": "run --allow-read entry.cjs",
6+
"output": "entry.out"
7+
},
8+
"monitor_origin_for_unhandled_rejection": {
9+
"args": "run --allow-read rejection.cjs",
10+
"output": "rejection.out"
11+
}
12+
}
13+
}
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
"use strict";
2+
3+
// Verify that a synchronous top-level throw in a CommonJS entry module fires
4+
// 'uncaughtExceptionMonitor' (and 'uncaughtException') with origin
5+
// 'uncaughtException', matching Node.js semantics. Without the
6+
// Module._load entry-catch in 01_require.js, the throw would surface as a
7+
// module-evaluation rejection and reach the listener with origin
8+
// 'unhandledRejection'.
9+
10+
const theErr = new Error("boom");
11+
12+
process.on("uncaughtExceptionMonitor", (err, origin) => {
13+
console.log(
14+
"monitor",
15+
origin,
16+
err === theErr ? "same-error" : "different-error",
17+
);
18+
});
19+
20+
process.on("uncaughtException", (err, origin) => {
21+
console.log(
22+
"uncaught",
23+
origin,
24+
err === theErr ? "same-error" : "different-error",
25+
);
26+
27+
process.nextTick(() => {
28+
process.setUncaughtExceptionCaptureCallback((capturedErr) => {
29+
console.log(
30+
"capture",
31+
capturedErr === theErr ? "same-error" : "different-error",
32+
);
33+
});
34+
throw theErr;
35+
});
36+
});
37+
38+
throw theErr;
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
monitor uncaughtException same-error
2+
uncaught uncaughtException same-error
3+
monitor uncaughtException same-error
4+
capture same-error
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
"use strict";
2+
3+
// Verify that a real unhandled promise rejection (not a top-level CJS throw)
4+
// still fires 'uncaughtExceptionMonitor' with origin 'unhandledRejection'.
5+
6+
const theErr = new Error("rejected");
7+
8+
process.on("uncaughtExceptionMonitor", (err, origin) => {
9+
console.log(
10+
"monitor",
11+
origin,
12+
err === theErr ? "same-error" : "different-error",
13+
);
14+
});
15+
16+
process.on("uncaughtException", (err, origin) => {
17+
console.log(
18+
"uncaught",
19+
origin,
20+
err === theErr ? "same-error" : "different-error",
21+
);
22+
process.exit(0);
23+
});
24+
25+
// Cause an unhandled promise rejection from an async context (not a
26+
// top-level sync throw). With no 'unhandledRejection' listener, this falls
27+
// back through the polyfill to 'uncaughtException' with origin
28+
// 'unhandledRejection'.
29+
setImmediate(() => {
30+
Promise.reject(theErr);
31+
});
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
monitor unhandledRejection same-error
2+
uncaught unhandledRejection same-error

0 commit comments

Comments
 (0)