Repository: denoland/deno_core
Pull request #647 (2024-03-08 01:06:49)

fix(core): don't error out with unref'd ops in TLA

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/denoland/deno_core/pull/647?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) Report
Attention: Patch coverage is `68.42105%` with `6 lines` in your changes are missing coverage. Please review.
> Project coverage is 80.77%. Comparing base [(`f7007a7`)](https://app.codecov.io/gh/denoland/deno_core/commit/f7007a7536ba85fc2b5dccf6dc4f2fa0d0495ab2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) to head [(`09ab200`)](https://app.codecov.io/gh/denoland/deno_core/pull/647?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).

| [Files](https://app.codecov.io/gh/denoland/deno_core/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) | Patch % | Lines |
|---|---|---|
| [testing/checkin/runner/mod.rs](https://app.codecov.io/gh/denoland/deno_core/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland#diff-dGVzdGluZy9jaGVja2luL3J1bm5lci9tb2QucnM=) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/denoland/deno_core/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #647      +/-   ##
==========================================
- Coverage   80.78%   80.77%   -0.02%     
==========================================
  Files          97       97              
  Lines       23913    23925      +12     
==========================================
+ Hits        19319    19325       +6     
- Misses       4594     4600       +6     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/denoland/deno_core/pull/647?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).


Patch:
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 113e444..eefc13d 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -1755,7 +1755,7 @@ impl JsRuntime {
     }
 
     if pending_state.has_pending_module_evaluation {
-      if pending_state.has_pending_refed_ops
+      if pending_state.has_pending_ops
         || pending_state.has_pending_dyn_imports
         || pending_state.has_pending_dyn_module_evaluation
         || pending_state.has_pending_background_tasks
@@ -1770,7 +1770,7 @@ impl JsRuntime {
     }
 
     if pending_state.has_pending_dyn_module_evaluation {
-      if pending_state.has_pending_refed_ops
+      if pending_state.has_pending_ops
         || pending_state.has_pending_dyn_imports
         || pending_state.has_pending_background_tasks
         || pending_state.has_tick_scheduled
@@ -1945,6 +1945,7 @@ impl JsRuntimeForSnapshot {
 
 #[derive(Clone, Copy, PartialEq, Eq, Debug)]
 pub(crate) struct EventLoopPendingState {
+  has_pending_ops: bool,
   has_pending_refed_ops: bool,
   has_pending_dyn_imports: bool,
   has_pending_dyn_module_evaluation: bool,
@@ -1964,7 +1965,8 @@ impl EventLoopPendingState {
     let num_unrefed_ops = state.unrefed_ops.borrow().len();
     let num_pending_ops = state.pending_ops.len();
     let has_pending_tasks = state.task_spawner_factory.has_pending_tasks();
-    let has_pending_timers = state.timers.has_pending_timers();
+    let has_pending_timers = !state.timers.is_empty();
+    let has_pending_refed_timers = state.timers.has_pending_timers();
     let has_pending_dyn_imports = modules.has_pending_dynamic_imports();
     let has_pending_dyn_module_evaluation =
       modules.has_pending_dyn_module_evaluation();
@@ -1979,10 +1981,14 @@ impl EventLoopPendingState {
         .pending_handled_promise_rejections
         .borrow()
         .is_empty();
+    let has_pending_refed_ops = has_pending_tasks
+      || has_pending_refed_timers
+      || num_pending_ops > num_unrefed_ops;
     EventLoopPendingState {
-      has_pending_refed_ops: has_pending_tasks
+      has_pending_ops: has_pending_refed_ops
         || has_pending_timers
-        || num_pending_ops > num_unrefed_ops,
+        || (num_pending_ops > 0),
+      has_pending_refed_ops,
       has_pending_dyn_imports,
       has_pending_dyn_module_evaluation,
       has_pending_module_evaluation,
diff --git a/core/web_timeout.rs b/core/web_timeout.rs
index 036f727..641f78e 100644
--- a/core/web_timeout.rs
+++ b/core/web_timeout.rs
@@ -476,7 +476,6 @@ impl<T: Clone> WebTimers<T> {
   }
 
   /// Is this set of timers empty?
-  #[cfg(test)]
   pub fn is_empty(&self) -> bool {
     self.data_map.borrow().is_empty()
   }


Test patch:
diff --git a/testing/checkin/runner/mod.rs b/testing/checkin/runner/mod.rs
index fe9df59..b7374a4 100644
--- a/testing/checkin/runner/mod.rs
+++ b/testing/checkin/runner/mod.rs
@@ -177,8 +177,17 @@ async fn run_integration_test_task(
     for line in e.to_string().split('\n') {
       output.line(format!("[ERR] {line}"));
     }
+  } else {
+    // Only await the module if we didn't fail
+    if let Err(e) = f.await {
+      let state = runtime.op_state().clone();
+      let state = state.borrow();
+      let output: &Output = state.borrow();
+      for line in e.to_string().split('\n') {
+        output.line(format!("[ERR] {line}"));
+      }
+    }
   }
-  f.await?;
   let mut lines = runtime.op_state().borrow_mut().take::<Output>().take();
   lines.push(String::new());
   let mut expected_output = String::new();
diff --git a/testing/integration/pending_unref_op_tla/pending_unref_op_tla.out b/testing/integration/pending_unref_op_tla/pending_unref_op_tla.out
new file mode 100644
index 0000000..7cf44be
--- /dev/null
+++ b/testing/integration/pending_unref_op_tla/pending_unref_op_tla.out
@@ -0,0 +1,2 @@
+should not panic
+didn't panic!
diff --git a/testing/integration/pending_unref_op_tla/pending_unref_op_tla.ts b/testing/integration/pending_unref_op_tla/pending_unref_op_tla.ts
new file mode 100644
index 0000000..a30a8aa
--- /dev/null
+++ b/testing/integration/pending_unref_op_tla/pending_unref_op_tla.ts
@@ -0,0 +1,7 @@
+// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
+console.log("should not panic");
+await new Promise((r) => {
+  const id = setTimeout(r, 1000);
+  Deno.unrefTimer(id);
+});
+console.log("didn't panic!");
diff --git a/testing/lib.rs b/testing/lib.rs
index e3ec71e..5933a29 100644
--- a/testing/lib.rs
+++ b/testing/lib.rs
@@ -58,6 +58,7 @@ integration_test!(
   error_ext_stack,
   error_with_stack,
   error_without_stack,
+  pending_unref_op_tla,
   smoke_test,
   timer_ref,
   timer_ref_and_cancel,