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
denoland/deno#24098) effectively boiled down
to this:

```ts
// main.ts
Deno.serve(async function (_req: Request): Promise<Response> {
    await import("./repro.ts");
    console.log("imported module");
    return new Response("done");
});

// repro.ts
await new Promise<void>((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

@@ -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::<v8::Promise>::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);
 

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<v8::Local<'s, v8::Function>> {
-  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,

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,

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<Output = ()> {
+  futures::future::pending::<()>()
+}

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 {

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);

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");

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,