Repository: denoland/deno_core
Pull request #769 (2024-06-06 17:46:57)

fix: Wake event loop when dynamic import promise resolves
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<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.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 2674abc..09f022e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2648,9 +2648,9 @@ dependencies = [
 
 [[package]]
 name = "v8"
-version = "0.92.0"
+version = "0.93.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "234589219e37a7496cbce73d971586db8369871be2420372c45a579b6a919b15"
+checksum = "abc3ff12243d345cc697c151b29fbb8ad1f898e3f8b7aa1386701ff9f2c7cbbf"
 dependencies = [
  "bitflags",
  "fslock",
@@ -2758,15 +2758,14 @@ dependencies = [
 
 [[package]]
 name = "which"
-version = "5.0.0"
+version = "6.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9bf3ea8596f3a0dd5980b46430f2058dfe2c36a27ccfbb1845d6fbfcd9ba6e14"
+checksum = "8211e4f58a2b2805adfbefbc07bab82958fc91e3836339b1ab7ae32465dce0d7"
 dependencies = [
  "either",
  "home",
- "once_cell",
  "rustix",
- "windows-sys 0.48.0",
+ "winsafe",
 ]
 
 [[package]]
@@ -2948,6 +2947,12 @@ dependencies = [
  "memchr",
 ]
 
+[[package]]
+name = "winsafe"
+version = "0.0.19"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d135d17ab770252ad95e9a872d365cf3090e3be864a34ab46f48555993efc904"
+
 [[package]]
 name = "wyz"
 version = "0.5.1"
diff --git a/Cargo.toml b/Cargo.toml
index fd79656..2caa451 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -24,7 +24,7 @@ deno_ops = { version = "0.160.0", path = "./ops" }
 serde_v8 = { version = "0.193.0", path = "./serde_v8" }
 deno_core_testing = { path = "./testing" }
 
-v8 = { version = "0.92.0", default-features = false }
+v8 = { version = "0.93.0", default-features = false }
 deno_ast = { version = "=0.35.3", features = ["transpiling"] }
 deno_unsync = "0.3.2"
 deno_core_icudata = "0.0.73"
diff --git a/core/lib.rs b/core/lib.rs
index 727f9a7..abe938c 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -127,6 +127,7 @@ pub use crate::modules::SourceCodeCacheInfo;
 pub use crate::modules::StaticModuleLoader;
 pub use crate::modules::ValidateImportAttributesCb;
 pub use crate::normalize_path::normalize_path;
+pub use crate::ops::ExternalOpsTracker;
 pub use crate::ops::OpId;
 pub use crate::ops::OpMetadata;
 pub use crate::ops::OpState;
diff --git a/core/modules/map.rs b/core/modules/map.rs
index 64f069c..d038950 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::<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);
 
diff --git a/core/ops.rs b/core/ops.rs
index 4709782..f071e01 100644
--- a/core/ops.rs
+++ b/core/ops.rs
@@ -16,6 +16,8 @@ use std::ops::Deref;
 use std::ops::DerefMut;
 use std::ptr::NonNull;
 use std::rc::Rc;
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering;
 use std::sync::Arc;
 use v8::fast_api::CFunctionInfo;
 use v8::fast_api::CTypeInfo;
@@ -250,12 +252,43 @@ impl OpCtx {
   }
 }
 
+/// Allows an embedder to track operations which should
+/// keep the event loop alive.
+#[derive(Debug, Clone)]
+pub struct ExternalOpsTracker {
+  counter: Arc<AtomicUsize>,
+}
+
+impl ExternalOpsTracker {
+  pub fn ref_op(&self) {
+    self.counter.fetch_add(1, Ordering::Relaxed);
+  }
+
+  pub fn unref_op(&self) {
+    let _ =
+      self
+        .counter
+        .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |x| {
+          if x == 0 {
+            None
+          } else {
+            Some(x - 1)
+          }
+        });
+  }
+
+  pub(crate) fn has_pending_ops(&self) -> bool {
+    self.counter.load(Ordering::Relaxed) > 0
+  }
+}
+
 /// Maintains the resources and ops inside a JS runtime.
 pub struct OpState {
   pub resource_table: ResourceTable,
   pub(crate) gotham_state: GothamState,
   pub waker: Arc<AtomicWaker>,
   pub feature_checker: Arc<FeatureChecker>,
+  pub external_ops_tracker: ExternalOpsTracker,
 }
 
 impl OpState {
@@ -265,6 +298,9 @@ impl OpState {
       gotham_state: Default::default(),
       waker: Arc::new(AtomicWaker::new()),
       feature_checker: maybe_feature_checker.unwrap_or_default(),
+      external_ops_tracker: ExternalOpsTracker {
+        counter: Arc::new(AtomicUsize::new(0)),
+      },
     }
   }
 
diff --git a/core/runtime/bindings.rs b/core/runtime/bindings.rs
index e7b8f5c..69c8ad3 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<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,
diff --git a/core/runtime/jsrealm.rs b/core/runtime/jsrealm.rs
index ab8e15c..694f2bb 100644
--- a/core/runtime/jsrealm.rs
+++ b/core/runtime/jsrealm.rs
@@ -11,6 +11,7 @@ use crate::modules::ModuleCodeString;
 use crate::modules::ModuleId;
 use crate::modules::ModuleMap;
 use crate::modules::ModuleName;
+use crate::ops::ExternalOpsTracker;
 use crate::ops::OpCtx;
 use crate::stats::RuntimeActivityTraces;
 use crate::tasks::V8TaskSpawnerFactory;
@@ -68,6 +69,7 @@ pub struct ContextState {
   pub(crate) exception_state: Rc<ExceptionState>,
   pub(crate) has_next_tick_scheduled: Cell<bool>,
   pub(crate) get_error_class_fn: GetErrorClassFn,
+  pub(crate) external_ops_tracker: ExternalOpsTracker,
 }
 
 impl ContextState {
@@ -76,6 +78,7 @@ impl ContextState {
     isolate_ptr: *mut v8::OwnedIsolate,
     get_error_class_fn: GetErrorClassFn,
     op_ctxs: Box<[OpCtx]>,
+    external_ops_tracker: ExternalOpsTracker,
   ) -> Self {
     Self {
       isolate: Some(isolate_ptr),
@@ -91,6 +94,7 @@ impl ContextState {
       task_spawner_factory: Default::default(),
       timers: Default::default(),
       unrefed_ops: Default::default(),
+      external_ops_tracker,
     }
   }
 }
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 8768c3f..2ff5591 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -605,13 +605,9 @@ impl JsRuntime {
   }
 
   /// Only constructor, configuration is done through `options`.
-  pub fn new(mut options: RuntimeOptions) -> JsRuntime {
-    setup::init_v8(
-      options.v8_platform.take(),
-      cfg!(test),
-      options.unsafe_expose_natives_and_gc(),
-    );
-    match JsRuntime::new_inner(options, false) {
+  /// Panics if the runtime cannot be initialized.
+  pub fn new(options: RuntimeOptions) -> JsRuntime {
+    match Self::try_new(options) {
       Ok(runtime) => runtime,
       Err(err) => {
         panic!("Failed to initialize a JsRuntime: {:?}", err);
@@ -619,6 +615,17 @@ impl JsRuntime {
     }
   }
 
+  /// Only constructor, configuration is done through `options`.
+  /// Returns an error if the runtime cannot be initialized.
+  pub fn try_new(mut options: RuntimeOptions) -> Result<JsRuntime, Error> {
+    setup::init_v8(
+      options.v8_platform.take(),
+      cfg!(test),
+      options.unsafe_expose_natives_and_gc(),
+    );
+    JsRuntime::new_inner(options, false)
+  }
+
   pub(crate) fn state_from(isolate: &v8::Isolate) -> Rc<JsRuntimeState> {
     let state_ptr = isolate.get_data(STATE_DATA_OFFSET);
     let state_rc =
@@ -804,6 +811,7 @@ impl JsRuntime {
       isolate_ptr,
       options.get_error_class_fn.unwrap_or(&|_| "Error"),
       op_ctxs,
+      op_state.borrow().external_ops_tracker.clone(),
     ));
 
     // TODO(bartlomieju): factor out
@@ -1802,6 +1810,7 @@ impl JsRuntime {
         || pending_state.has_pending_dyn_imports
         || pending_state.has_pending_dyn_module_evaluation
         || pending_state.has_pending_background_tasks
+        || pending_state.has_pending_external_ops
         || pending_state.has_tick_scheduled
       {
         // pass, will be polled again
@@ -1816,6 +1825,7 @@ impl JsRuntime {
       if pending_state.has_pending_ops
         || pending_state.has_pending_dyn_imports
         || pending_state.has_pending_background_tasks
+        || pending_state.has_pending_external_ops
         || pending_state.has_tick_scheduled
       {
         // pass, will be polled again
@@ -1884,20 +1894,28 @@ fn create_context<'a>(
 }
 
 impl JsRuntimeForSnapshot {
-  pub fn new(mut options: RuntimeOptions) -> JsRuntimeForSnapshot {
+  /// Create a new runtime, panicking if the process fails.
+  pub fn new(options: RuntimeOptions) -> JsRuntimeForSnapshot {
+    match Self::try_new(options) {
+      Ok(runtime) => runtime,
+      Err(err) => {
+        panic!("Failed to initialize JsRuntime for snapshotting: {:?}", err);
+      }
+    }
+  }
+
+  /// Try to create a new runtime, returning an error if the process fails.
+  pub fn try_new(
+    mut options: RuntimeOptions,
+  ) -> Result<JsRuntimeForSnapshot, Error> {
     setup::init_v8(
       options.v8_platform.take(),
       true,
       options.unsafe_expose_natives_and_gc(),
     );
 
-    let runtime = match JsRuntime::new_inner(options, true) {
-      Ok(r) => r,
-      Err(err) => {
-        panic!("Failed to initialize JsRuntime for snapshotting: {:?}", err);
-      }
-    };
-    JsRuntimeForSnapshot(runtime)
+    let runtime = JsRuntime::new_inner(options, true)?;
+    Ok(JsRuntimeForSnapshot(runtime))
   }
 
   /// Takes a snapshot and consumes the runtime.
@@ -1996,6 +2014,7 @@ pub(crate) struct EventLoopPendingState {
   has_pending_background_tasks: bool,
   has_tick_scheduled: bool,
   has_pending_promise_events: bool,
+  has_pending_external_ops: bool,
 }
 
 impl EventLoopPendingState {
@@ -2038,6 +2057,7 @@ impl EventLoopPendingState {
       has_pending_background_tasks: scope.has_pending_background_tasks(),
       has_tick_scheduled: state.has_next_tick_scheduled.get(),
       has_pending_promise_events,
+      has_pending_external_ops: state.external_ops_tracker.has_pending_ops(),
     }
   }
 
@@ -2056,6 +2076,7 @@ impl EventLoopPendingState {
       || self.has_pending_background_tasks
       || self.has_tick_scheduled
       || self.has_pending_promise_events
+      || self.has_pending_external_ops
   }
 }
 


Test patch:
diff --git a/core/runtime/tests/misc.rs b/core/runtime/tests/misc.rs
index ba94cc2..1ba8ff8 100644
--- a/core/runtime/tests/misc.rs
+++ b/core/runtime/tests/misc.rs
@@ -1243,8 +1243,9 @@ async fn global_template_middleware() {
     _key: v8::Local<'s, v8::Name>,
     _args: v8::PropertyCallbackArguments<'s>,
     _rv: v8::ReturnValue,
-  ) {
-    unsafe { CALLS.push("descriptor".to_string()) }
+  ) -> v8::Intercepted {
+    unsafe { CALLS.push("descriptor".to_string()) };
+    v8::Intercepted::No
   }
 
   pub fn setter<'s>(
@@ -1253,10 +1254,11 @@ async fn global_template_middleware() {
     _value: v8::Local<'s, v8::Value>,
     _args: v8::PropertyCallbackArguments<'s>,
     _rv: v8::ReturnValue,
-  ) {
+  ) -> v8::Intercepted {
     unsafe {
       CALLS.push("setter".to_string());
-    }
+    };
+    v8::Intercepted::No
   }
 
   fn definer<'s>(
@@ -1265,10 +1267,11 @@ async fn global_template_middleware() {
     _descriptor: &v8::PropertyDescriptor,
     _args: v8::PropertyCallbackArguments<'s>,
     _rv: v8::ReturnValue,
-  ) {
+  ) -> v8::Intercepted {
     unsafe {
       CALLS.push("definer".to_string());
-    }
+    };
+    v8::Intercepted::No
   }
 
   pub fn gt_middleware<'s>(
diff --git a/testing/checkin/runner/extensions.rs b/testing/checkin/runner/extensions.rs
index 0f2fcf7..c8a162c 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 b15d16a..8d21a1f 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<Output = ()> {
+  futures::future::pending::<()>()
+}
diff --git a/testing/checkin/runtime/async.ts b/testing/checkin/runtime/async.ts
index 333e666..acd265d 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 0000000..3daf93e
--- /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 0000000..c3b859d
--- /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 0000000..80568d8
--- /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 0655022..5a6d683 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,