From 208c91b68f4ae1b59e65acbde3de729e7058bb5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Fri, 23 Dec 2022 19:46:24 +0100 Subject: [PATCH] fix(core): run macrotasks and next ticks after polling dynamic imports (#17173) This commit fixes handling of rejected promises in dynamic imports evaluation. Previously we were running callbacks for next ticks and macrotasks _before_ polling dynamic imports and checked for unhandled rejections immediately after. This is wrong, as `unhandledrejection` event is dispatched and its callbacks are run as macrotasks. This commit changes order of actions performed by the event loop to following: - poll async ops - poll dynamic imports - run next tick callbacks - run macrotask callbacks - check for unhandled promise rejections --- cli/tests/run_tests.rs | 13 ++++++++++++ .../import.ts | 5 +++++ .../main.ts | 1 + .../main.ts.out | 3 +++ .../import.ts | 3 +++ .../main.ts | 21 +++++++++++++++++++ .../main.ts.out | 5 +++++ core/runtime.rs | 15 +++++++------ 8 files changed, 58 insertions(+), 8 deletions(-) create mode 100644 cli/tests/testdata/run/unhandled_rejection_dynamic_import/import.ts create mode 100644 cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts create mode 100644 cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts.out create mode 100644 cli/tests/testdata/run/unhandled_rejection_dynamic_import2/import.ts create mode 100644 cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts create mode 100644 cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts.out diff --git a/cli/tests/run_tests.rs b/cli/tests/run_tests.rs index 092546aaf9..d90a48fbbe 100644 --- a/cli/tests/run_tests.rs +++ b/cli/tests/run_tests.rs @@ -2945,6 +2945,19 @@ mod run { output: "run/unhandled_rejection_sync_error.ts.out", }); + // Regression test for https://github.com/denoland/deno/issues/15661 + itest!(unhandled_rejection_dynamic_import { + args: "run --allow-read run/unhandled_rejection_dynamic_import/main.ts", + output: "run/unhandled_rejection_dynamic_import/main.ts.out", + exit_code: 1, + }); + + // Regression test for https://github.com/denoland/deno/issues/16909 + itest!(unhandled_rejection_dynamic_import2 { + args: "run --allow-read run/unhandled_rejection_dynamic_import2/main.ts", + output: "run/unhandled_rejection_dynamic_import2/main.ts.out", + }); + itest!(nested_error { args: "run run/nested_error.ts", output: "run/nested_error.ts.out", diff --git a/cli/tests/testdata/run/unhandled_rejection_dynamic_import/import.ts b/cli/tests/testdata/run/unhandled_rejection_dynamic_import/import.ts new file mode 100644 index 0000000000..b490f2c204 --- /dev/null +++ b/cli/tests/testdata/run/unhandled_rejection_dynamic_import/import.ts @@ -0,0 +1,5 @@ +globalThis.addEventListener("unhandledrejection", () => { + console.log("hey"); +}); +console.log("---"); +Promise.reject(); diff --git a/cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts b/cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts new file mode 100644 index 0000000000..244d844672 --- /dev/null +++ b/cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts @@ -0,0 +1 @@ +await import("./import.ts"); diff --git a/cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts.out b/cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts.out new file mode 100644 index 0000000000..f44c6d2ff8 --- /dev/null +++ b/cli/tests/testdata/run/unhandled_rejection_dynamic_import/main.ts.out @@ -0,0 +1,3 @@ +--- +hey +error: Uncaught (in promise) undefined diff --git a/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/import.ts b/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/import.ts new file mode 100644 index 0000000000..f84297afd9 --- /dev/null +++ b/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/import.ts @@ -0,0 +1,3 @@ +export default { + a: "a", +}; diff --git a/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts b/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts new file mode 100644 index 0000000000..3da2e1d19f --- /dev/null +++ b/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts @@ -0,0 +1,21 @@ +globalThis.addEventListener("unhandledrejection", (e) => { + console.log("unhandled rejection", e.reason); + e.preventDefault(); +}); + +const dummyImport = (await import("./import.ts")).default; + +let a = new Promise((resolve, reject) => { + throw "errA"; +}); + +let i = 0; +while (true) { + await new Promise((resolve) => setTimeout(resolve, 100)); + i++; + console.log("running..."); + + if (i > 3) { + break; + } +} diff --git a/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts.out b/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts.out new file mode 100644 index 0000000000..0a913a4b51 --- /dev/null +++ b/cli/tests/testdata/run/unhandled_rejection_dynamic_import2/main.ts.out @@ -0,0 +1,5 @@ +unhandled rejection errA +running... +running... +running... +running... diff --git a/core/runtime.rs b/core/runtime.rs index 2e417bb9d3..f735f55758 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -1127,12 +1127,7 @@ impl JsRuntime { self.pump_v8_message_loop()?; // Ops - { - self.resolve_async_ops(cx)?; - self.drain_nexttick()?; - self.drain_macrotasks()?; - self.check_promise_exceptions()?; - } + self.resolve_async_ops(cx)?; // Dynamic module loading - ie. modules loaded using "import()" { // Run in a loop so that dynamic imports that only depend on another @@ -1157,9 +1152,13 @@ impl JsRuntime { break; } } - - self.check_promise_exceptions()?; } + // Run all next tick callbacks and macrotasks callbacks and only then + // check for any promise exceptions (`unhandledrejection` handlers are + // run in macrotasks callbacks so we need to let them run first). + self.drain_nexttick()?; + self.drain_macrotasks()?; + self.check_promise_exceptions()?; // Event loop middlewares let mut maybe_scheduling = false;