Repository: denoland/deno_core
Pull request #137 (2023-08-08 08:03:21)

fix: don't swallow rejections in poll_value()
Unblocks https://github.com/denoland/deno/pull/20082.

Fixes three tangled issues:
- `JsRuntime::poll_value(promise)` swallows non-awaited rejections returned from `poll_event_loop()` when `promise` is already settled.
- `JsRuntime::poll_value(promise)` runs the event loop once even if `promise` is already settled.
- `JsRuntime::call_and_await()` needs to explicitly call `check_promise_rejections()` at the end.

Some test expectations might need to add `(in promise)` to error strings. These are correct. There's at least one in CLI repo:
```diff
diff --git a/cli/tests/testdata/node/test.out b/cli/tests/testdata/node/test.out
index 8b7f0780f..3c54a15e8 100644
--- a/cli/tests/testdata/node/test.out
+++ b/cli/tests/testdata/node/test.out
@@ -147,7 +147,7 @@ error: Error: rejected from reject fail
     at [WILDCARD]
 
 ./node/test.js (uncaught error)
-error: Error: rejected from unhandled rejection fail
+error: (in promise) Error: rejected from unhandled rejection fail
   Promise.reject(new Error("rejected from unhandled rejection fail"));
                  ^
     at [WILDCARD]
```

bartlomieju: @nayeemrmn what do you mean by
> After fixing these, JsRuntime::call_and_await() doesn't account for non-awaited rejections.

How that should be handled by the embedder?
nayeemrmn: > @nayeemrmn what do you mean by
> 
> > After fixing these, JsRuntime::call_and_await() doesn't account for non-awaited rejections.
> 
> How that should be handled by the embedder?

I wrote it confusingly. It was a third issue introduced by fixing the first two but it's also fixed.

Patch:
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 86c015c..10ed95c 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -1220,7 +1220,9 @@ impl JsRuntime {
       }
       v8::Global::new(scope, promise.unwrap())
     };
-    self.resolve_value(promise).await
+    let result = self.resolve_value(promise).await;
+    self.check_promise_rejections()?;
+    result
   }
 
   /// Returns the namespace object of a module.
@@ -1315,34 +1317,59 @@ impl JsRuntime {
     global: &v8::Global<v8::Value>,
     cx: &mut Context,
   ) -> Poll<Result<v8::Global<v8::Value>, Error>> {
-    let state = self.poll_event_loop(cx, false);
-
-    let mut scope = self.handle_scope();
-    let local = v8::Local::<v8::Value>::new(&mut scope, global);
-
-    if let Ok(promise) = v8::Local::<v8::Promise>::try_from(local) {
-      match promise.state() {
-        v8::PromiseState::Pending => match state {
-          Poll::Ready(Ok(_)) => {
-            let msg = "Promise resolution is still pending but the event loop has already resolved.";
-            Poll::Ready(Err(generic_error(msg)))
+    // Check if the value is not a promise or already settled before polling the
+    // event loop.
+    let promise_global = {
+      let scope = &mut self.handle_scope();
+      let local = v8::Local::<v8::Value>::new(scope, global);
+      if let Ok(promise) = v8::Local::<v8::Promise>::try_from(local) {
+        match promise.state() {
+          v8::PromiseState::Pending => v8::Global::new(scope, promise),
+          v8::PromiseState::Fulfilled => {
+            let value = promise.result(scope);
+            let value_handle = v8::Global::new(scope, value);
+            return Poll::Ready(Ok(value_handle));
+          }
+          v8::PromiseState::Rejected => {
+            let exception = promise.result(scope);
+            let promise_global = v8::Global::new(scope, promise);
+            JsRealm::state_from_scope(scope)
+              .borrow_mut()
+              .pending_promise_rejections
+              .retain(|(key, _)| key != &promise_global);
+            return Poll::Ready(exception_to_err_result(
+              scope, exception, false,
+            ));
           }
-          Poll::Ready(Err(e)) => Poll::Ready(Err(e)),
-          Poll::Pending => Poll::Pending,
-        },
-        v8::PromiseState::Fulfilled => {
-          let value = promise.result(&mut scope);
-          let value_handle = v8::Global::new(&mut scope, value);
-          Poll::Ready(Ok(value_handle))
         }
-        v8::PromiseState::Rejected => {
-          let exception = promise.result(&mut scope);
-          Poll::Ready(exception_to_err_result(&mut scope, exception, false))
+      } else {
+        return Poll::Ready(Ok(global.clone()));
+      }
+    };
+
+    // Poll the event loop.
+    let event_loop_result = self.poll_event_loop(cx, false)?;
+
+    // Check the promise state again.
+    let scope = &mut self.handle_scope();
+    let promise = v8::Local::new(scope, promise_global);
+    match promise.state() {
+      v8::PromiseState::Pending => match event_loop_result {
+        Poll::Ready(_) => {
+          let msg = "Promise resolution is still pending but the event loop has already resolved.";
+          Poll::Ready(Err(generic_error(msg)))
         }
+        Poll::Pending => Poll::Pending,
+      },
+      v8::PromiseState::Fulfilled => {
+        let value = promise.result(scope);
+        let value_handle = v8::Global::new(scope, value);
+        Poll::Ready(Ok(value_handle))
+      }
+      v8::PromiseState::Rejected => {
+        let exception = promise.result(scope);
+        Poll::Ready(exception_to_err_result(scope, exception, false))
       }
-    } else {
-      let value_handle = v8::Global::new(&mut scope, local);
-      Poll::Ready(Ok(value_handle))
     }
   }
 


Test patch:
diff --git a/core/runtime/tests/snapshot.rs b/core/runtime/tests/snapshot.rs
index 2783733..41e0949 100644
--- a/core/runtime/tests/snapshot.rs
+++ b/core/runtime/tests/snapshot.rs
@@ -365,7 +365,7 @@ pub(crate) fn generic_es_snapshot_without_runtime_module_loader(
   assert!(dyn_import_result.is_err());
   assert_eq!(
     dyn_import_result.err().unwrap().to_string().as_str(),
-    r#"Uncaught TypeError: Module loading is not supported; attempted to load: "ext:module_snapshot/test2.js" from "(no referrer)""#
+    r#"Uncaught (in promise) TypeError: Module loading is not supported; attempted to load: "ext:module_snapshot/test2.js" from "(no referrer)""#
   );
 }
 
@@ -447,7 +447,7 @@ pub(crate) fn generic_preserve_snapshotted_modules_test(
     assert!(dyn_import_result.is_err());
     assert_eq!(
       dyn_import_result.err().unwrap().to_string().as_str(),
-      "Uncaught TypeError: Module loading is not supported; attempted to load: \"test:not-preserved\" from \"(no referrer)\""
+      "Uncaught (in promise) TypeError: Module loading is not supported; attempted to load: \"test:not-preserved\" from \"(no referrer)\""
     );
     // Ensure that we tried to load `test:not-preserved`
     assert_eq!(