diff --git a/src/workerd/api/global-scope.c++ b/src/workerd/api/global-scope.c++ index 6c5c23ec648..01fc959435a 100644 --- a/src/workerd/api/global-scope.c++ +++ b/src/workerd/api/global-scope.c++ @@ -734,6 +734,8 @@ void ServiceWorkerGlobalScope::emitPromiseRejection(jsg::Lock& js, }; if (hasHandlers() || hasInspector()) { + unhandledRejections.setUseMicrotasksCompletedCallback( + FeatureFlags::get(js).getUnhandledRejectionAfterMicrotaskCheckpoint()); unhandledRejections.report(js, event, kj::mv(promise), kj::mv(value)); } } diff --git a/src/workerd/api/tests/BUILD.bazel b/src/workerd/api/tests/BUILD.bazel index 4422b8e2dd8..9d010b466d2 100644 --- a/src/workerd/api/tests/BUILD.bazel +++ b/src/workerd/api/tests/BUILD.bazel @@ -277,6 +277,13 @@ wd_test( data = ["global-scope-test.js"], ) +wd_test( + size = "large", + src = "unhandled-rejection-test.wd-test", + args = ["--experimental"], + data = ["unhandled-rejection-test.js"], +) + wd_test( size = "large", src = "htmlrewriter-test.wd-test", diff --git a/src/workerd/api/tests/unhandled-rejection-test.js b/src/workerd/api/tests/unhandled-rejection-test.js new file mode 100644 index 00000000000..0b815cdc433 --- /dev/null +++ b/src/workerd/api/tests/unhandled-rejection-test.js @@ -0,0 +1,223 @@ +// Regression tests for https://github.com/cloudflare/workerd/issues/6020 +// Unhandled rejection should NOT fire for promises that are handled through +// multi-tick promise chains. + +import { strictEqual, ok, rejects } from 'node:assert'; +import { mock } from 'node:test'; + +const asyncFunction = async (name) => { + throw new Error(`this function rejects: ${name}`); +}; + +// Verifies assert.rejects handles rejections without unhandledrejection. +export const assertRejects = { + async test() { + const handler = mock.fn(); + addEventListener('unhandledrejection', handler); + try { + await rejects(async () => asyncFunction('A')); + strictEqual( + handler.mock.callCount(), + 0, + 'unhandledrejection should not fire for assert.rejects' + ); + } finally { + removeEventListener('unhandledrejection', handler); + } + }, +}; + +// Verifies chained .then().catch() handling avoids unhandledrejection. +export const promiseChainCatch = { + async test() { + const handler = mock.fn(); + addEventListener('unhandledrejection', handler); + try { + const error = await Promise.resolve() + .then(() => asyncFunction('B')) + .then(() => null) + .catch((e) => e); + ok(error instanceof Error); + strictEqual(error.message, 'this function rejects: B'); + strictEqual( + handler.mock.callCount(), + 0, + 'unhandledrejection should not fire for .catch() chain' + ); + } finally { + removeEventListener('unhandledrejection', handler); + } + }, +}; + +// Verifies try/catch around awaited chain avoids unhandledrejection. +export const tryCatchAwait = { + async test() { + const handler = mock.fn(); + addEventListener('unhandledrejection', handler); + try { + try { + await Promise.resolve('C').then(asyncFunction); + } catch (error) { + ok(error instanceof Error); + strictEqual(error.message, 'this function rejects: C'); + } + strictEqual( + handler.mock.callCount(), + 0, + 'unhandledrejection should not fire for try/catch' + ); + } finally { + removeEventListener('unhandledrejection', handler); + } + }, +}; + +// Verifies a truly unhandled rejection still emits unhandledrejection. +export const genuineUnhandledRejectionStillFires = { + async test() { + const { promise, resolve } = Promise.withResolvers(); + const handler = mock.fn(() => resolve()); + addEventListener('unhandledrejection', handler, { once: true }); + Promise.reject('boom'); + await promise; + strictEqual( + handler.mock.callCount(), + 1, + 'unhandledrejection should fire for genuinely unhandled rejection' + ); + }, +}; + +// Verifies unhandledrejection fires after a Promise.resolve tick. +export const unhandledRejectionAfterPromiseResolve = { + async test() { + const { promise, resolve } = Promise.withResolvers(); + const handler = mock.fn(() => resolve()); + addEventListener('unhandledrejection', handler, { once: true }); + Promise.reject('boom'); + await Promise.resolve(); + await promise; + strictEqual( + handler.mock.callCount(), + 1, + 'unhandledrejection should fire after Promise.resolve' + ); + }, +}; + +// Verifies unhandledrejection followed by rejectionhandled on late catch. +export const lateHandlerTriggersRejectionhandled = { + async test() { + const { promise: unhandledPromise, resolve: resolveUnhandled } = + Promise.withResolvers(); + const { promise: handledPromise, resolve: resolveHandled } = + Promise.withResolvers(); + let unhandledReason; + let handledReason; + const unhandledHandler = mock.fn((event) => { + unhandledReason = event.reason; + resolveUnhandled(); + }); + const handledHandler = mock.fn((event) => { + handledReason = event.reason; + resolveHandled(); + }); + addEventListener('unhandledrejection', unhandledHandler, { once: true }); + addEventListener('rejectionhandled', handledHandler, { once: true }); + try { + const error = new Error('late'); + const promise = Promise.reject(error); + await unhandledPromise; + promise.catch(() => {}); + await handledPromise; + strictEqual( + unhandledHandler.mock.callCount(), + 1, + 'unhandledrejection should fire once before late handler' + ); + strictEqual( + handledHandler.mock.callCount(), + 1, + 'rejectionhandled should fire after late handler' + ); + ok(unhandledReason instanceof Error); + strictEqual(unhandledReason.message, 'late'); + strictEqual( + handledReason, + undefined, + 'rejectionhandled reason should be undefined' + ); + } finally { + removeEventListener('unhandledrejection', unhandledHandler); + removeEventListener('rejectionhandled', handledHandler); + } + }, +}; + +// Verifies unhandledrejection handler can trigger another unhandled rejection. +export const handlerTriggeredUnhandledRejection = { + async test() { + const { promise, resolve } = Promise.withResolvers(); + const timeout = new Promise((resolveTimeout) => { + setTimeout(resolveTimeout, 25); + }); + const reasons = []; + let callCount = 0; + const handler = mock.fn((event) => { + reasons.push(event.reason); + callCount += 1; + if (callCount === 1) { + queueMicrotask(() => Promise.reject(new Error('second'))); + } + if (callCount === 2) { + resolve(); + } + }); + addEventListener('unhandledrejection', handler); + try { + Promise.reject(new Error('first')); + await Promise.race([promise, timeout]); + strictEqual( + handler.mock.callCount(), + 2, + 'unhandledrejection should fire for rejection triggered by handler' + ); + strictEqual(reasons.length, 2); + ok(reasons[0] instanceof Error); + strictEqual(reasons[0].message, 'first'); + ok(reasons[1] instanceof Error); + strictEqual(reasons[1].message, 'second'); + } finally { + removeEventListener('unhandledrejection', handler); + } + }, +}; + +// Verifies each unhandled rejection emits its own event. +export const multipleUnhandledRejections = { + async test() { + const { promise, resolve } = Promise.withResolvers(); + const timeout = new Promise((resolveTimeout) => { + setTimeout(resolveTimeout, 25); + }); + const handler = mock.fn(() => { + if (handler.mock.callCount() === 2) { + resolve(); + } + }); + addEventListener('unhandledrejection', handler); + try { + Promise.reject(new Error('one')); + Promise.reject(new Error('two')); + await Promise.race([promise, timeout]); + strictEqual( + handler.mock.callCount(), + 2, + 'unhandledrejection should fire for each unhandled rejection' + ); + } finally { + removeEventListener('unhandledrejection', handler); + } + }, +}; diff --git a/src/workerd/api/tests/unhandled-rejection-test.wd-test b/src/workerd/api/tests/unhandled-rejection-test.wd-test new file mode 100644 index 00000000000..17b642f3ddc --- /dev/null +++ b/src/workerd/api/tests/unhandled-rejection-test.wd-test @@ -0,0 +1,17 @@ +using Workerd = import "/workerd/workerd.capnp"; + +const unitTests :Workerd.Config = ( + services = [ + ( name = "unhandled-rejection-test", + worker = ( + modules = [ + (name = "worker", esModule = embed "unhandled-rejection-test.js") + ], + compatibilityFlags = [ + "nodejs_compat", + "unhandled_rejection_after_microtask_checkpoint", + ] + ) + ), + ], +); diff --git a/src/workerd/io/compatibility-date.capnp b/src/workerd/io/compatibility-date.capnp index dfa4f22014f..671d094fd5c 100644 --- a/src/workerd/io/compatibility-date.capnp +++ b/src/workerd/io/compatibility-date.capnp @@ -1378,4 +1378,11 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { # When enabled, the UTF-16le TextDecoder will replace lone surrogates with U+FFFD # (the Unicode replacement character) as required by the spec. Previously, lone # surrogates were passed through unchanged, producing non-well-formed strings. + + unhandledRejectionAfterMicrotaskCheckpoint @160 :Bool + $compatEnableFlag("unhandled_rejection_after_microtask_checkpoint") + $compatDisableFlag("no_unhandled_rejection_after_microtask_checkpoint") + $compatEnableDate("2026-03-03"); + # When enabled, unhandledrejection processing is deferred until the microtask + # checkpoint completes, avoiding misfires on multi-tick promise chains. } diff --git a/src/workerd/jsg/jsg.c++ b/src/workerd/jsg/jsg.c++ index e2af4923d8b..7a371dc0cf7 100644 --- a/src/workerd/jsg/jsg.c++ +++ b/src/workerd/jsg/jsg.c++ @@ -290,6 +290,25 @@ bool Lock::v8HasOwn(v8::Local obj, kj::StringPtr name) { void Lock::runMicrotasks() { v8Isolate->PerformMicrotaskCheckpoint(); + + auto& isolate = IsolateBase::from(v8Isolate); + // We only expect at most a handful of extra checkpoints. Keep a generous cap + // to flush cascaded microtasks but prevent a potential busy loop. + static constexpr uint MAX_EXTRA_MICROTASK_CHECKPOINTS = 64; + for (uint i = 0; i < MAX_EXTRA_MICROTASK_CHECKPOINTS; ++i) { + if (!isolate.takeExtraMicrotaskCheckpointRequested({})) { + return; + } + v8Isolate->PerformMicrotaskCheckpoint(); + } + + if (isolate.takeExtraMicrotaskCheckpointRequested({})) { + KJ_LOG(WARNING, "extra microtask checkpoint limit reached", MAX_EXTRA_MICROTASK_CHECKPOINTS); + } +} + +void Lock::requestExtraMicrotaskCheckpoint() { + IsolateBase::from(v8Isolate).requestExtraMicrotaskCheckpoint({}); } void Lock::terminateNextExecution() { diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index f4a0d15e314..1fefe8f1183 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -2800,6 +2800,9 @@ class Lock { void runMicrotasks(); + // Request an extra microtask checkpoint after the current one completes. + void requestExtraMicrotaskCheckpoint(); + // Sets the terminate-execution flag on the isolate so that the next time code tries to run, it // will be terminated. (But note that V8 only checks the flag at certain times, so it's possible // some code will actually execute before termination kicks in.) diff --git a/src/workerd/jsg/promise.c++ b/src/workerd/jsg/promise.c++ index 8da264a30e8..2faa6732663 100644 --- a/src/workerd/jsg/promise.c++ +++ b/src/workerd/jsg/promise.c++ @@ -35,10 +35,7 @@ UnhandledRejectionHandler::UnhandledRejection::UnhandledRejection(jsg::Lock& js, promise(js.v8Isolate, promise.getHandle(js)), value(js.v8Isolate, value.getHandle(js)), message(js.v8Isolate, message), - asyncContextFrame(getFrameRef(js)) { - this->promise.SetWeak(); - this->value.SetWeak(); -} + asyncContextFrame(getFrameRef(js)) {} void UnhandledRejectionHandler::report( Lock& js, v8::PromiseRejectEvent event, jsg::V8Ref promise, jsg::Value value) { @@ -135,43 +132,77 @@ void UnhandledRejectionHandler::ensureProcessingWarnings(jsg::Lock& js) { return; } scheduled = true; - js.resolvedPromise().then(js, [this](jsg::Lock& js) { - scheduled = false; - warnedRejections.eraseAll([](auto& value) { return !value.isAlive(); }); + if (useMicrotasksCompletedCallback) { + // Schedule processing to run after the microtask checkpoint completes. + // This ensures that promise chains like `.then().catch()` have fully settled + // before we decide a rejection is unhandled. Using a microtask would race + // with V8's internal promise adoption microtasks and fire too early. + // See https://github.com/cloudflare/workerd/issues/6020 + js.v8Isolate->AddMicrotasksCompletedCallback( + &UnhandledRejectionHandler::onMicrotasksCompleted, this); + // Ensure we get another microtask checkpoint to deliver the callback even if + // we're already past the current one. + js.requestExtraMicrotaskCheckpoint(); + } else { + js.resolvedPromise().then(js, [this](jsg::Lock& js) { processWarnings(js); }); + } +} - while (unhandledRejections.size() > 0) { - auto entry = unhandledRejections.release(*unhandledRejections.begin()); +void UnhandledRejectionHandler::onMicrotasksCompleted(v8::Isolate* isolate, void* data) { + auto* handler = static_cast(data); + KJ_DEFER(isolate->RemoveMicrotasksCompletedCallback( + &UnhandledRejectionHandler::onMicrotasksCompleted, data)); + auto& js = Lock::from(isolate); + KJ_IF_SOME(exception, kj::runCatchingExceptions([&]() { + handler->processWarnings(js); + + // Ensure microtasks scheduled by unhandledrejection handlers run promptly. + js.requestExtraMicrotaskCheckpoint(); + })) { + handler->scheduled = false; + KJ_LOG(ERROR, "uncaught exception while processing unhandled rejections", exception); + } +} - if (!entry.isAlive()) { - continue; - } +void UnhandledRejectionHandler::processWarnings(jsg::Lock& js) { + scheduled = false; + warnedRejections.eraseAll([](auto& value) { return !value.isAlive(); }); - auto promise = getLocal(js.v8Isolate, entry.promise); - auto value = getLocal(js.v8Isolate, entry.value); - - AsyncContextFrame::Scope scope(js, tryGetFrame(entry.asyncContextFrame)); - - // Most of the time it shouldn't be found but there are times where it can - // be duplicated -- such as when a promise gets rejected multiple times. - // Check quickly before inserting to avoid a crash. - warnedRejections.upsert( - kj::mv(entry), [](UnhandledRejection& existing, UnhandledRejection&& replacement) { - // We're just going to ignore if the unhandled rejection was already here. - }); - - js.tryCatch([&] { - handler(js, v8::kPromiseRejectWithNoHandler, jsg::HashableV8Ref(js.v8Isolate, promise), - js.v8Ref(value)); - }, [&](Value exception) { - // If any exceptions occur while reporting the event, we will log them - // but otherwise ignore them. We do not want such errors to be fatal here. - if (js.areWarningsLogged()) { - js.logWarning( - kj::str("Exception while logging unhandled rejection:", exception.getHandle(js))); - } - }); + while (unhandledRejections.size() > 0) { + auto entry = unhandledRejections.release(*unhandledRejections.begin()); + + if (!entry.isAlive()) { + continue; } - }); + + auto promise = getLocal(js.v8Isolate, entry.promise); + auto value = getLocal(js.v8Isolate, entry.value); + + AsyncContextFrame::Scope scope(js, tryGetFrame(entry.asyncContextFrame)); + + // Most of the time it shouldn't be found but there are times where it can + // be duplicated -- such as when a promise gets rejected multiple times. + // Check quickly before inserting to avoid a crash. + // Keep strong refs through dispatch, then downgrade to weak to avoid leaks. + entry.promise.SetWeak(); + entry.value.SetWeak(); + warnedRejections.upsert( + kj::mv(entry), [](UnhandledRejection& existing, UnhandledRejection&& replacement) { + // We're just going to ignore if the unhandled rejection was already here. + }); + + js.tryCatch([&] { + handler(js, v8::kPromiseRejectWithNoHandler, jsg::HashableV8Ref(js.v8Isolate, promise), + js.v8Ref(value)); + }, [&](Value exception) { + // If any exceptions occur while reporting the event, we will log them + // but otherwise ignore them. We do not want such errors to be fatal here. + if (js.areWarningsLogged()) { + js.logWarning( + kj::str("Exception while logging unhandled rejection:", exception.getHandle(js))); + } + }); + } } void UnhandledRejectionHandler::UnhandledRejection::visitForMemoryInfo( diff --git a/src/workerd/jsg/promise.h b/src/workerd/jsg/promise.h index e00b4c651f5..1f0bb2c79d1 100644 --- a/src/workerd/jsg/promise.h +++ b/src/workerd/jsg/promise.h @@ -712,6 +712,10 @@ class UnhandledRejectionHandler { jsg::V8Ref promise, jsg::Value value); + void setUseMicrotasksCompletedCallback(bool value) { + useMicrotasksCompletedCallback = value; + } + void clear(); JSG_MEMORY_INFO(UnhandledRejectionHandler) { @@ -804,6 +808,8 @@ class UnhandledRejectionHandler { kj::Function handler; bool scheduled = false; + // Controlled by the unhandled_rejection_after_microtask_checkpoint compat flag. + bool useMicrotasksCompletedCallback = false; using UnhandledRejectionsTable = kj::Table>; @@ -814,6 +820,10 @@ class UnhandledRejectionHandler { void rejectedWithNoHandler(jsg::Lock& js, jsg::V8Ref promise, jsg::Value value); void handledAfterRejection(jsg::Lock& js, jsg::V8Ref promise); void ensureProcessingWarnings(jsg::Lock& js); + void processWarnings(jsg::Lock& js); + + // Must be static: V8 requires a plain C function pointer for this callback. + static void onMicrotasksCompleted(v8::Isolate* isolate, void* data); }; } // namespace workerd::jsg diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index ac0e7ce4acf..7d518b82e83 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -133,6 +133,19 @@ class IsolateBase { return kj::none; } + // Requests an extra microtask checkpoint after the current one completes. + inline void requestExtraMicrotaskCheckpoint(kj::Badge) { + extraMicrotaskCheckpointRequested = true; + } + + // Returns true if an extra microtask checkpoint was requested since the last + // call, and clears the flag. + inline bool takeExtraMicrotaskCheckpointRequested(kj::Badge) { + bool requested = extraMicrotaskCheckpointRequested; + extraMicrotaskCheckpointRequested = false; + return requested; + } + inline void setAllowEval(kj::Badge, bool allow) { if (alwaysAllowEval) return; evalAllowed = allow; @@ -354,6 +367,7 @@ class IsolateBase { bool usingNewModuleRegistry = false; bool usingEnhancedErrorSerialization = false; bool usingFastJsgStruct = false; + bool extraMicrotaskCheckpointRequested = false; // Only used when the original module registry is used. bool throwOnUnrecognizedImportAssertion = false;