Skip to content

Commit

Permalink
Handle errors thrown by reportError in queueMicrotask
Browse files Browse the repository at this point in the history
  • Loading branch information
jasnell committed Sep 24, 2024
1 parent 8458817 commit e9bfcc4
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 1 deletion.
6 changes: 6 additions & 0 deletions src/workerd/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,9 @@ wd_test(
args = ["--experimental"],
data = ["tests/cross-context-promise-test.js"],
)

wd_test(
src = "tests/error-in-error-event-test.wd-test",
args = ["--experimental"],
data = ["tests/error-in-error-event-test.js"],
)
15 changes: 14 additions & 1 deletion src/workerd/api/global-scope.c++
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,20 @@ void ServiceWorkerGlobalScope::queueMicrotask(jsg::Lock& js, v8::Local<v8::Funct
return jsg::check(
function->Call(js.v8Context(), js.v8Null(), argv.size(), argv.data()));
}, [&](jsg::Value exception) {
reportError(js, jsg::JsValue(exception.getHandle(js)));
// The reportError call itself can potentially throw errors. Let's catch
// and report them as well.
js.tryCatch([&] { reportError(js, jsg::JsValue(exception.getHandle(js))); },
[&](jsg::Value exception) {
if (IoContext::hasCurrent()) {
// If we're running in a request context then we want to abort the
// request with the exception here.
IoContext::current().abort(js.exceptionToKj(kj::mv(exception)));
} else {
// If we're running outside of a request context. There's nothing to
// abort but we at least want to log the exception.
js.reportError(jsg::JsValue(exception.getHandle(js)));
}
});
return js.v8Undefined();
});
}));
Expand Down
30 changes: 30 additions & 0 deletions src/workerd/api/tests/error-in-error-event-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { strictEqual } from 'assert';

export const test = {
async test(_, env) {
try {
await env.subrequest.fetch('http://example.org');
} catch (e) {
strictEqual(e.message, 'boom (real)');
}
},
};

export default {
async fetch() {
// Throwing an error in the error event listener should not be catchable directly
// but should cause the IoContext to be aborted with the error.
addEventListener(
'error',
() => {
throw new Error('boom (real)');
},
{ once: true }
);
queueMicrotask(() => {
throw new Error('boom (unused)');
});
await scheduler.wait(1000);
return new Response('should not see this');
},
};
18 changes: 18 additions & 0 deletions src/workerd/api/tests/error-in-error-event-test.wd-test
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "error-in-error-event-test",
worker = (
modules = [
(name = "worker", esModule = embed "error-in-error-event-test.js")
],
compatibilityDate = "2024-09-15",
compatibilityFlags = ["nodejs_compat_v2"],
bindings = [
(name = "subrequest", service = "error-in-error-event-test")
],
)
),
],
);

0 comments on commit e9bfcc4

Please sign in to comment.