cli/src/executor.rs starvation bug with setTimeout(..., 0)

[ivankra]

Boa CLI's custom executor cli/src/executor.rs has issues with some Atomics.waitAsync tests using a setTimeout(..., 0) loop:

test/built-ins/Atomics/waitAsync/bigint/true-for-timeout.js
test/built-ins/Atomics/waitAsync/returns-result-object-value-is-promise-resolves-to-timed-out.js
test/built-ins/Atomics/waitAsync/true-for-timeout.js

These tests pass under boa_tester, which uses SimpleJobExecutor, but hang under the CLI executor, resulting in these failures: Test262:AsyncTestFailure:Test262Error: Test timed out.

Minimal reproduction:

$ cat min.js
let done = false;
let start = $262.agent.monotonicNow();
let i64a = new BigInt64Array(new SharedArrayBuffer(BigInt64Array.BYTES_PER_ELEMENT));

(function wait() {
  if (done) { print("ok"); return; }
  if ($262.agent.monotonicNow() - start > 1000) { print("timeout"); return; }
  setTimeout(wait, 0);
})();

Atomics.waitAsync(i64a, 0, 0n, true).value.then((result) => { done = true; });

$ ./target/release/boa --test262-object min.js
timeout

Swapping it for SimpleJobExecutor like below allows CLI to pass - perhaps this should be the default executor, at least for non-interactive runs (is CLI executor designed for REPL)?

patch

diff --git a/cli/src/main.rs b/cli/src/main.rs
index 3b15df54..fe6b4885 100644
--- a/cli/src/main.rs
+++ b/cli/src/main.rs
@@ -17,7 +17,7 @@ use crate::logger::SharedExternalPrinterLogger;
 use async_channel::Sender;
 use boa_engine::JsValue;
 use boa_engine::error::JsErasedError;
-use boa_engine::job::NativeAsyncJob;
+use boa_engine::job::{NativeAsyncJob, SimpleJobExecutor};
 use boa_engine::{
     Context, JsError, Source,
     builtins::promise::PromiseState,
@@ -557,10 +557,15 @@ fn main() -> Result<()> {
     let (sender, receiver) = async_channel::unbounded();
     let printer = SharedExternalPrinterLogger::new();
 
+    let interactive = args.files.is_empty() && args.expression.is_none() && io::stdin().is_terminal();
     let executor = Rc::new(Executor::new(printer.clone()));
     let loader = Rc::new(SimpleModuleLoader::new(&args.root).map_err(|e| eyre!(e.to_string()))?);
-    let context = &mut ContextBuilder::new()
-        .job_executor(executor.clone())
+    let context = if interactive {
+        ContextBuilder::new().job_executor(executor.clone())
+    } else {
+        ContextBuilder::new().job_executor(Rc::new(SimpleJobExecutor::new()))
+    };
+    let context = &mut context
         .module_loader(loader.clone())
         .can_block(!args.no_can_block)
         .build()

[jedel1043]

is CLI executor designed for REPL

Yes, it's designed for a REPL, so we need to patch the bug instead of replacing it with the simple job executor.

About the bug, I think it's because enqueueing timeouts of 0 ms makes it such that the task in charge of pushing the async jobs forward is always busy, so it never yields to the event loop for other tasks to execute. I see two solutions for this:

1. yield_now() after any async job executes, which solves the problem but is kind of inefficient if you have multiple async jobs that already finished.
2. Define a minimum timeout of 5 ms or something such that no timeouts can just starve the event loop.

[jedel1043]

FWIW I think most JS runtimes will just define a minimum timeout, so it might be better to do the same.

[ivankra]

Yeah, that sounds reasonable and fixes the test failure.

I see Node is doing the same with a minimum of 1ms: https://nodejs.org/api/timers.html#settimeoutcallback-delay-args