Unhandled rejections in workers should not fire an error event on the Worker object

[andreubotella]

The HTML spec defines steps for reporting runtime script errors (with the error event) and unhandled promise rejections (with the unhandledrejection event). Both mechanisms are fairly similar, but they differ on what happens when an error or rejection happens inside a dedicated worker and is not caught by the corresponding event. Unhandled promise rejections will simply be reported to the console, as they would in a window agent, and never make it outside of the worker – while script errors will fire an error event to the corresponding Worker object in the parent agent.

Deno doesn't (yet?) implement the unhandledrejection event, and an unhandled rejection on the main thread will kill the process. However, an unhandled rejection in a worker will instead fire an error event on the corresponding Worker object – which shouldn't happen per the spec. Arguably the process should be killed, as it would if it happened on the main thread.

This is causing some worker tests to hang forever in #12222.

---

Here's a test to compare the behavior in Deno and in browsers.

// main.js
const worker = new Worker(new URL("./worker.js", import.meta.url), {type: "module"});

worker.addEventListener("error", () => {
    console.log("worker.onerror in main thread");
});

window.addEventListener("error", () => {
    console.log("window.onerror in main thread");
});

window.addEventListener("unhandledrejection", () => {
    console.log("window.onunhandledrejection in main thread");
});

// worker.js
self.addEventListener("error", () => {
    console.log("self.onerror in worker");
});

self.addEventListener("unhandledrejection", () => {
    console.log("self.onunhandledrejection in worker");
});

(async () => {
    throw new Error("Error");
})();

All browsers fire the unhandledrejection event inside the worker, report the error, and don't fire any event in the main thread:

While Deno will report the error inside the worker, fire the error event on the main thread's worker object, and then die because the error wasn't handled:

(This screenshot uses Deno canary – 1.14.1 would panic because of #11342.)

[nayeemrmn]

Browsers on unhandled errors in workers will:

1. Print the error.
2. Dispatch an error event within the worker ("error" for sync and "unhandledrejection" for async).
3. Propagate an "error" event up to the Worker object in the host, only for sync.
4. Allow worker code not blocked on the error site to continue.

Deno on unhanded errors in workers will:

1. Print the error.
2. Kill the worker, no events are dispatched within the worker.
3. Propagate an "error" event up to the Worker object in the host, for both sync and async.
4. Not allow any other worker code to continue, the worker has been killed.

Most differences are intentional, but we can improve points 2 and 3.

1. Print the error.
2. Kill the worker, but first dispatch the proper events and allow one more event loop for listeners to run.
   • In consideration since onunhandledrejection for globalThis? #7013, onunhandledrejection for globalThis? #7013 (comment). Would this fix the hangs @andreubotella?
3. Propagate an "error" event up to the Worker object in the host, only for sync. Kill the process on async errors in workers.
   • As you suggested. Since async errors are not supposed to be passed up to the host, killing the entire process on them is the only safe thing left to do. It would encourage handling async errors same as normal.
4. Not allow any other worker code to continue, the worker has been killed.

[andreubotella]

That SGTM.

2. Kill the worker, but first dispatch the proper events and allow one more event loop for listeners to run.
   • In consideration since onunhandledrejection for globalThis? #7013, onunhandledrejection for globalThis? #7013 (comment). Would this fix the hangs @andreubotella?

Having the process exit at some point after the async error is thrown is enough to prevent the hangs. (The real cause of the hangs is that WPT will not try to use setTimeout in non-browser environments, so tests won't time out – and in some tests, a promise rejection in the runtime Deno code seems to be preventing other promises from resolving. In non-worker tests, the unhandled rejection will kill the process, but in worker tests it won't.)

[kevinkassimo]

2. Kill the worker, but first dispatch the proper events and allow one more event loop for listeners to run.

Experimented with dispatching an error event for sync errors (so that it works more similar to browser) in #12242 . There seems to be some issues wrt to main worker exiting too early. Essentially due to

deno/runtime/js/11_workers.js

Lines 267 to 268 in 34a1554

	if (globalThis instanceof Window) {
	throw new Error("Unhandled error event reached main worker.");

As the main thread exits under error, the handler code in the worker would simply die abruptly halfway since its thread handle is getting dropped along with it and cause the isolate to terminate. I wonder if blocking the main worker until the sync error handler code in the target worker is completed is acceptable by any chance?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[Mutefish0]

Is there any update ? Suffering from the same issue.

[nakasyou]

I have the same problem.

[yacinehmito]

@nayeemrmn You wrote "most differences are intentional". What is the rationale behind those differences?

[nayeemrmn]

You wrote "most differences are intentional". What is the rationale behind those differences?

Browsers won't close tabs on JS errors for example, but for a backend runtime we want to exit on errors as is standard

[josephrocca]

This issue might be about something more complicated than what I'm dealing with, but in case it's helpful to others, here's a workaround to prevent unhandledrejection from propagating to the worker.onerror handler in the parent:

// in worker script:
self.addEventListener("unhandledrejection", (event) => {
  console.error("Unhandled rejection:", event.reason);
  event.preventDefault();
  self.close();
});

The event.preventDefault() call doesn't seem to be needed, but it does prevent the error from propagating to the parent when used without self.close(), so I'm leaving it just in case self.close() behavior is not well defined in terms of ordering of the relevant operations here.