From 68c4885b171380b23bd3576c2fb841c965da4155 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 6 Jun 2024 16:26:13 -0700 Subject: [PATCH] fix: Wake event loop when dynamic import promise resolves (#769) Fixes a hang that could occur when a module contained a dynamic import that doesn't resolve immediately, and there was a pending op outstanding. The original reproduction (ref https://github.com/denoland/deno/issues/24098) effectively boiled down to this: ```ts // main.ts Deno.serve(async function (_req: Request): Promise { await import("./repro.ts"); console.log("imported module"); return new Response("done"); }); // repro.ts await new Promise((resolve) => setTimeout(resolve, 0)); console.log("module executed"); ``` Upon getting a request, it would hang when trying to import `"repro.ts"`. What was happening here was that `Deno.serve` created an op (`op_http_serve`) that stayed pending indefinitely, so we [assumed](https://github.com/denoland/deno_core/blob/4beb54e862805be6b2c4d8fb2bfc49ea72705799/core/runtime/jsruntime.rs#L1816C6-L1822C9) that we would poll again, but that didn't actually hold. Unlike non-dynamic module evaluation, we weren't waking the event loop for polling when the dynamic import evaluation promise resolved. The fix here is to attaching callbacks to the dynamic import promise to wake the event loop when it resolves, similar to what we do for normal module evaluation. --- core/modules/map.rs | 29 +++++++++++++++---- core/runtime/bindings.rs | 19 ------------ testing/checkin/runner/extensions.rs | 1 + testing/checkin/runner/ops_async.rs | 5 ++++ testing/checkin/runtime/async.ts | 7 +++++ .../dyn_import_no_hang/dyn_import_no_hang.js | 15 ++++++++++ .../dyn_import_no_hang/dyn_import_no_hang.out | 2 ++ .../integration/dyn_import_no_hang/dynamic.js | 8 +++++ testing/lib.rs | 1 + 9 files changed, 62 insertions(+), 25 deletions(-) create mode 100644 testing/integration/dyn_import_no_hang/dyn_import_no_hang.js create mode 100644 testing/integration/dyn_import_no_hang/dyn_import_no_hang.out create mode 100644 testing/integration/dyn_import_no_hang/dynamic.js diff --git a/core/modules/map.rs b/core/modules/map.rs index 64f069c91..d038950be 100644 --- a/core/modules/map.rs +++ b/core/modules/map.rs @@ -1191,9 +1191,9 @@ impl ModuleMap { // of module evaluation is a promise. // // This promise is internal, and not the same one that gets returned to - // the user. We add an empty `.catch()` handler so that it does not result - // in an exception if it rejects. That will instead happen for the other - // promise if not handled by the user. + // the user. We add handlers to wake the event loop when the promise resolves + // (or rejects). The catch handler also serves to prevent an exception if the internal promise + // rejects. That will instead happen for the other if not handled by the user. // // For more details see: // https://github.com/denoland/deno/issues/4908 @@ -1210,11 +1210,28 @@ impl ModuleMap { status == v8::ModuleStatus::Evaluated || status == v8::ModuleStatus::Errored ); + + fn wake_module( + scope: &mut v8::HandleScope<'_>, + _args: v8::FunctionCallbackArguments<'_>, + _rv: v8::ReturnValue, + ) { + let module_map = JsRealm::module_map_from(scope); + module_map.module_waker.wake(); + } + let promise = v8::Local::::try_from(value) .expect("Expected to get promise as module evaluation result"); - let empty_fn = - crate::runtime::bindings::create_empty_fn(tc_scope).unwrap(); - promise.catch(tc_scope, empty_fn); + + let wake_module_cb = Function::builder(wake_module).build(tc_scope); + + if let Some(wake_module_cb) = wake_module_cb { + promise.then2(tc_scope, wake_module_cb, wake_module_cb); + } else { + // If the runtime is shutting down, we can't attach the handlers. + // It doesn't really matter though, because they're just for waking the + // event loop. + } let promise_global = v8::Global::new(tc_scope, promise); let module_global = v8::Global::new(tc_scope, module); diff --git a/core/runtime/bindings.rs b/core/runtime/bindings.rs index e7b8f5cdb..69c8ad372 100644 --- a/core/runtime/bindings.rs +++ b/core/runtime/bindings.rs @@ -53,9 +53,6 @@ pub(crate) fn create_external_references( references.push(v8::ExternalReference { function: catch_dynamic_import_promise_error.map_fn_to(), }); - references.push(v8::ExternalReference { - function: empty_fn.map_fn_to(), - }); references.push(v8::ExternalReference { function: op_disabled_fn.map_fn_to(), }); @@ -589,14 +586,6 @@ fn import_meta_resolve( }; } -fn empty_fn( - _scope: &mut v8::HandleScope, - _args: v8::FunctionCallbackArguments, - _rv: v8::ReturnValue, -) { - //Do Nothing -} - pub(crate) fn op_disabled_fn( scope: &mut v8::HandleScope, _args: v8::FunctionCallbackArguments, @@ -607,14 +596,6 @@ pub(crate) fn op_disabled_fn( scope.throw_exception(exception); } -//It creates a reference to an empty function which can be mantained after the snapshots -pub fn create_empty_fn<'s>( - scope: &mut v8::HandleScope<'s>, -) -> Option> { - let empty_fn = v8::FunctionTemplate::new(scope, empty_fn); - empty_fn.get_function(scope) -} - fn catch_dynamic_import_promise_error( scope: &mut v8::HandleScope, args: v8::FunctionCallbackArguments, diff --git a/testing/checkin/runner/extensions.rs b/testing/checkin/runner/extensions.rs index 0f2fcf7e6..c8a162c6f 100644 --- a/testing/checkin/runner/extensions.rs +++ b/testing/checkin/runner/extensions.rs @@ -32,6 +32,7 @@ deno_core::extension!( ops_async::op_async_spin_on_state, ops_async::op_async_make_cppgc_resource, ops_async::op_async_get_cppgc_resource, + ops_async::op_async_never_resolves, ops_error::op_async_throw_error_eager, ops_error::op_async_throw_error_lazy, ops_error::op_async_throw_error_deferred, diff --git a/testing/checkin/runner/ops_async.rs b/testing/checkin/runner/ops_async.rs index b15d16a56..8d21a1fc1 100644 --- a/testing/checkin/runner/ops_async.rs +++ b/testing/checkin/runner/ops_async.rs @@ -78,3 +78,8 @@ pub async fn op_async_get_cppgc_resource( ) -> u32 { resource.value } + +#[op2(async)] +pub fn op_async_never_resolves() -> impl Future { + futures::future::pending::<()>() +} diff --git a/testing/checkin/runtime/async.ts b/testing/checkin/runtime/async.ts index 333e666cc..acd265dba 100644 --- a/testing/checkin/runtime/async.ts +++ b/testing/checkin/runtime/async.ts @@ -8,6 +8,7 @@ const { op_stats_diff, op_stats_dump, op_stats_delete, + op_async_never_resolves, } = Deno .core .ops; @@ -29,6 +30,12 @@ export async function asyncSpin() { await op_async_spin_on_state(); } +export function asyncNeverResolves() { + const prom = op_async_never_resolves(); + Deno.core.refOpPromise(prom); + return prom; +} + let nextStats = 0; export class Stats { diff --git a/testing/integration/dyn_import_no_hang/dyn_import_no_hang.js b/testing/integration/dyn_import_no_hang/dyn_import_no_hang.js new file mode 100644 index 000000000..3daf93e04 --- /dev/null +++ b/testing/integration/dyn_import_no_hang/dyn_import_no_hang.js @@ -0,0 +1,15 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +import { asyncNeverResolves } from "checkin:async"; + +// make a promise that never resolves so we have +// a pending op outstanding +const prom = asyncNeverResolves(); + +// import a module, with the key being that +// this module promise doesn't resolve until a later +// tick of the event loop +await import("./dynamic.js"); +console.log("module imported"); + +// unref to let the event loop exit +Deno.core.unrefOpPromise(prom); diff --git a/testing/integration/dyn_import_no_hang/dyn_import_no_hang.out b/testing/integration/dyn_import_no_hang/dyn_import_no_hang.out new file mode 100644 index 000000000..c3b859d4a --- /dev/null +++ b/testing/integration/dyn_import_no_hang/dyn_import_no_hang.out @@ -0,0 +1,2 @@ +module executed +module imported diff --git a/testing/integration/dyn_import_no_hang/dynamic.js b/testing/integration/dyn_import_no_hang/dynamic.js new file mode 100644 index 000000000..80568d8cd --- /dev/null +++ b/testing/integration/dyn_import_no_hang/dynamic.js @@ -0,0 +1,8 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +await new Promise((resolve) => { + // Resolve the promise after one tick of the event loop. + setTimeout(() => { + resolve(); + }, 0); +}); +console.log("module executed"); diff --git a/testing/lib.rs b/testing/lib.rs index 065502206..5a6d68392 100644 --- a/testing/lib.rs +++ b/testing/lib.rs @@ -55,6 +55,7 @@ integration_test!( builtin_console_test, dyn_import_circular, dyn_import_op, + dyn_import_no_hang, error_async_stack, error_rejection_catch, error_rejection_order,