Repository: denoland/deno
Pull request: fix(core): always report the first error on unhandled rejection

The root cause of denoland/deno_std#3320, I believe, is that `pending_promise_rejections` is a `HashMap` whose entries are in arbitrary order, and as a result either of the two errors (`AddrInUse` and `TypeError`) may be selected when determining which one to report. I changed the field to a `VecDeque` so that the first error (`AddrInUse` in this case) is always selected.

Minimal reproduction of the issue addressed by this PR:

```js
Promise.reject("first");
Promise.reject("second");
```

This should always print `error: Uncaught (in promise) "first"` but occasionally prints `error: Uncaught (in promise) "second"`.


Changed files:
filename: core/bindings.rs
patch: @@ -499,12 +499,12 @@ pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) {
         let error_global = v8::Global::new(scope, error);
         context_state
           .pending_promise_rejections
-          .insert(promise_global, error_global);
+          .push_back((promise_global, error_global));
       }
       PromiseHandlerAddedAfterReject => {
         context_state
           .pending_promise_rejections
-          .remove(&promise_global);
+          .retain(|(key, _)| key != &promise_global);
       }
       PromiseRejectAfterResolved => {}
       PromiseResolveAfterResolved => {

filename: core/ops_builtin_v8.rs
patch: @@ -894,7 +894,7 @@ fn op_store_pending_promise_rejection<'a>(
   let error_global = v8::Global::new(scope, reason.v8_value);
   context_state
     .pending_promise_rejections
-    .insert(promise_global, error_global);
+    .push_back((promise_global, error_global));
 }
 
 #[op(v8)]
@@ -909,7 +909,7 @@ fn op_remove_pending_promise_rejection<'a>(
   let promise_global = v8::Global::new(scope, promise_value);
   context_state
     .pending_promise_rejections
-    .remove(&promise_global);
+    .retain(|(key, _)| key != &promise_global);
 }
 
 #[op(v8)]
@@ -924,7 +924,8 @@ fn op_has_pending_promise_rejection<'a>(
   let promise_global = v8::Global::new(scope, promise_value);
   context_state
     .pending_promise_rejections
-    .contains_key(&promise_global)
+    .iter()
+    .any(|(key, _)| key == &promise_global)
 }
 
 #[op(v8)]

filename: core/realm.rs
patch: @@ -7,8 +7,8 @@ use crate::runtime::exception_to_err_result;
 use crate::JsRuntime;
 use anyhow::Error;
 use std::cell::RefCell;
-use std::collections::HashMap;
 use std::collections::HashSet;
+use std::collections::VecDeque;
 use std::hash::BuildHasherDefault;
 use std::hash::Hasher;
 use std::option::Option;
@@ -43,7 +43,7 @@ pub(crate) struct ContextState {
   pub(crate) js_format_exception_cb: Option<Rc<v8::Global<v8::Function>>>,
   pub(crate) js_wasm_streaming_cb: Option<Rc<v8::Global<v8::Function>>>,
   pub(crate) pending_promise_rejections:
-    HashMap<v8::Global<v8::Promise>, v8::Global<v8::Value>>,
+    VecDeque<(v8::Global<v8::Promise>, v8::Global<v8::Value>)>,
   pub(crate) unrefed_ops: HashSet<i32, BuildHasherDefault<IdentityHasher>>,
   // We don't explicitly re-read this prop but need the slice to live alongside
   // the context
@@ -270,22 +270,9 @@ impl<'s> JsRealmLocal<'s> {
     let context_state_rc = self.state(scope);
     let mut context_state = context_state_rc.borrow_mut();
 
-    if context_state.pending_promise_rejections.is_empty() {
+    let Some((_, handle)) = context_state.pending_promise_rejections.pop_front() else {
       return Ok(());
-    }
-
-    let key = {
-      context_state
-        .pending_promise_rejections
-        .keys()
-        .next()
-        .unwrap()
-        .clone()
     };
-    let handle = context_state
-      .pending_promise_rejections
-      .remove(&key)
-      .unwrap();
     drop(context_state);
 
     let exception = v8::Local::new(scope, handle);

filename: core/runtime.rs
patch: @@ -1831,7 +1831,7 @@ impl JsRuntime {
             .state(tc_scope)
             .borrow_mut()
             .pending_promise_rejections
-            .remove(&promise_global);
+            .retain(|(key, _)| key != &promise_global);
         }
       }
       let promise_global = v8::Global::new(tc_scope, promise);
@@ -4138,6 +4138,23 @@ Deno.core.opAsync("op_async_serialize_object_with_numbers_as_keys", {
       .contains("JavaScript execution has been terminated"));
   }
 
+  #[tokio::test]
+  async fn test_unhandled_rejection_order() {
+    let mut runtime = JsRuntime::new(Default::default());
+    runtime
+      .execute_script_static(
+        "",
+        r#"
+        for (let i = 0; i < 100; i++) {
+          Promise.reject(i);
+        }
+        "#,
+      )
+      .unwrap();
+    let err = runtime.run_event_loop(false).await.unwrap_err();
+    assert_eq!(err.to_string(), "Uncaught (in promise) 0");
+  }
+
   #[tokio::test]
   async fn test_set_promise_reject_callback() {
     static PROMISE_REJECT: AtomicUsize = AtomicUsize::new(0);