Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ext/web): Add error events for event listener and timer errors #14159

Merged

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Mar 30, 2022

feat(ext/web): Add error events for event listener and timer errors

- feat: Add handleable error event for even listener errors
- feat: Add handleable error event for setTimeout()/setInterval() errors
- feat: Add Deno.core.destructureError()
- feat: Add Deno.core.terminate()
- fix: Don't throw listener errors from dispatchEvent()
- fix: Use biased mode when selecting between mod_evaluate() and
  run_event_loop() results

@nayeemrmn
Copy link
Collaborator Author

@lucacasonato @bartlomieju Ready for review

ext/web/02_event.js Outdated Show resolved Hide resolved
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Just some minor comments.

ext/web/lib.deno_web.d.ts Outdated Show resolved Hide resolved
cli/fmt_errors.rs Outdated Show resolved Hide resolved
ext/web/02_event.js Outdated Show resolved Hide resolved
ext/web/02_event.js Outdated Show resolved Hide resolved
ext/web/02_timers.js Outdated Show resolved Hide resolved
runtime/js/99_main.js Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

globalThis.onerror = (e) => { ... } is not working (but globalThis.addEventListener("error", () => {}); is working), I think we should address it in this PR before landing.

This is very close now @nayeemrmn thanks for working on this, very intricate problem.

@lucacasonato
Copy link
Member

I wanted to avoid the cancel_terminate_execution(); re_terminate_execution() in this code path because I didn't know what consequences it would have. Looks like it sometimes causes us to re-enter exception_to_err_result and get a generic termination error.

That is the strangest thing! I just tried it locally, and indeed! How strange! I will debug this further. It is very strange.

@lucacasonato
Copy link
Member

lucacasonato commented Apr 7, 2022

I have figured it out:

  • the first invocation of exception_to_err_result happens in mod_evaluate.
  • the second invocation of exception_to_err_result happens in JsRuntime::poll_event_loop (called by JsRuntime::run_event_loop).
  • mod_evaluate is called in parallel with JsRuntime::run_event_loop.
  • Because of poll order, the error from JsRuntime::run_event_loop can sometimes be returned instead of the one from mod_evaluate().

I am not sure if the current code in this PR is susceptible to this issue too.

@lucacasonato
Copy link
Member

lucacasonato commented Apr 7, 2022

Unfortunately, there are issues. Run this 10 times and see it vary in output:

addEventListener("foo", () => {
  queueMicrotask(() => console.log("queueMicrotask"));
  setTimeout(() => console.log("timer"), 0);
  throw new Error("bar");
});
console.log(1);
Deno.core.setNextTickCallback(() => console.log("nextTick"));
Deno.core.setHasTickScheduled(true);
dispatchEvent(new CustomEvent("foo"));
console.log(2);

Another fun one (#14158):

Deno.core.setNextTickCallback(() => {
  console.log("nextTick");
  Deno.core.setHasTickScheduled(false);
});
Deno.core.setHasTickScheduled(true);
addEventListener("foo", () => {
  queueMicrotask(() => console.log("queueMicrotask"));
  setTimeout(() => console.log("timer"), 0);
  throw new Error("bar");
});
console.log(1);
queueMicrotask(() => {
  dispatchEvent(new CustomEvent("foo"));
});
console.log(2);

@nayeemrmn
Copy link
Collaborator Author

@lucacasonato 4e8df5c most likely removes any non-deterministic behaviour that isn't warranted (no IO etc). Without that you sometimes get a single poll_event_loop() call even though the module evaluated synchronously.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple nit picks

core/runtime.rs Outdated Show resolved Hide resolved
ext/web/02_timers.js Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nayeemrmn while this now looks good to land, I feel we should discuss it with the rest of the team before actually landing. This PR adds a way to prevent termination of program on uncaught error, which was not possible before (it's similar to adding window.onunhandledrejection, which is quite controversial). Hope that's okay

@nayeemrmn
Copy link
Collaborator Author

This PR adds a way to prevent termination of program on uncaught error, which was not possible before (it's similar to adding window.onunhandledrejection, which is quite controversial)

This error handling affects calbacks for runtime APIs whose invocations are controlled by the host, when you think about it it's normal to have handlers for stuff like that in comparison to global vanilla error hooks.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with everyone, got no objections to land. Thanks @nayeemrmn!

Just one last thing before I land this: can you add an itest case for the scenario I had described above with the flaky output?

@bartlomieju
Copy link
Member

@nayeemrmn please draft a commit message

@nayeemrmn
Copy link
Collaborator Author

@nayeemrmn please draft a commit message

Added to PR description

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great that you managed to get my cleanup working with that biased mode select.

Thanks so much for this, and sorry for the long arduous review!

@lucacasonato lucacasonato merged commit 4d18f55 into denoland:main Apr 13, 2022
@nayeemrmn nayeemrmn deleted the error-event-for-events-and-timers branch April 13, 2022 12:30
dsherret pushed a commit that referenced this pull request Apr 14, 2022
…14159)

- feat: Add handleable error event for even listener errors
- feat: Add handleable error event for setTimeout()/setInterval() errors
- feat: Add Deno.core.destructureError()
- feat: Add Deno.core.terminate()
- fix: Don't throw listener errors from dispatchEvent()
- fix: Use biased mode when selecting between mod_evaluate() and
  run_event_loop() results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants