thebadking/obscura
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

revert(js): drop the "cancellation-safe" event loop pumping

Browse Source 
The premise was wrong. deno_core 0.350's run_event_loop is:

    pub async fn run_event_loop(&mut self, opts) -> Result<(), CoreError> {
        poll_fn(|cx| self.poll_event_loop(cx, opts)).await
    }

so replacing `timeout(budget, run_event_loop())` with a hand-rolled
`poll_fn(poll_event_loop)` loop is the same code with the same cancellation
semantics, not safer ones. The mechanism I described cannot happen either:
`poll` is a synchronous `&mut self` call, a future can only be dropped between
polls, and the HandleScope/ContextScope live inside poll_event_loop as stack
locals that are destroyed before it returns. The isolate is exited between
polls both before and after that change.

What actually fixes #430 is confining each connection's isolates to their own
OS thread, which the next commits do. The regression test still reproduces the
abort on the pre-thread-per-connection tree and still passes here without this
commit, so nothing depended on it.

It was not merely a no-op. Two regressions it introduced:

- resolve_promises_until inverted the meaning of tick_ms. It was "pump the
  event loop for up to tick_ms, returning early on idle"; it became one single
  poll followed by an unconditional sleep(tick_ms). With the 1->50ms backoff
  that settles at 20 event-loop turns per second, so an awaited promise chain
  needing many turns crawls, and past the budget the caller silently reads a
  stale __obscura_await_meta.
- drive_module_eval replaced `e?` with `let _ = poll_event_loop(...)`,
  discarding event-loop errors. A module stalled on a top-level await used to
  exit immediately with a logged error; it instead spun for the whole budget
  logging nothing -- the exact #374 starvation the surrounding comment claims
  to prevent.

Keep the drive_module_eval extraction (it genuinely dedupes load_module and
load_inline_module) but restore the original select!-based body.

Refs #430.
This commit is contained in:
sycatle committed 2026-07-20 18:41:00 +02:00
1 parent 3015f7d8ad
commit 319c6034fe
2 files changed
+62 -136
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

crates/obscura-browser/src/page.rs
+13 -15
View File
@@ -763,13 +763,13 @@ impl Page {
if tokio::time::Instant::now() >= deadline {
break;
}
// Cancellation-safe single tick: a per-poll `tokio::time::timeout`
// wrapping `run_event_loop` would drop that future mid-poll and
// leave this isolate entered on the thread, aborting the process
// under concurrent pages (#430). `poll_event_loop_once` exits the
// isolate before returning, so the tick is safe to bound.
match js.poll_event_loop_once().await {
Some(Ok(())) => {
let result = tokio::time::timeout(
tokio::time::Duration::from_millis(10),
js.run_event_loop(),
).await;
match result {
Ok(Ok(())) => {
if self.http_client.active_requests() == 0 {
idle_count += 1;
if idle_count >= 2 {
@@ -781,10 +781,9 @@ impl Page {
tokio::time::sleep(tokio::time::Duration::from_millis(1)).await;
}
}
Some(Err(_)) => break,
None => {
Ok(Err(_)) => break,
Err(_) => {
idle_count = 0;
tokio::time::sleep(tokio::time::Duration::from_millis(1)).await;
}
}
}
@@ -1227,11 +1226,10 @@ impl Page {
}
if let Some(js) = &mut self.js {
// Cancellation-safe tick (see #430): never drop a
// `run_event_loop` future mid-poll, which leaks the entered
// isolate. `poll_event_loop_once` exits the isolate per tick.
let _ = js.poll_event_loop_once().await;
tokio::time::sleep(tokio::time::Duration::from_millis(1)).await;
let _ = tokio::time::timeout(
tokio::time::Duration::from_millis(50),
js.run_event_loop(),
).await;
} else {
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
}
crates/obscura-js/src/runtime.rs
+49 -121
View File
@@ -702,41 +702,38 @@ impl ObscuraJsRuntime {
}
/// Drive a just-started module evaluation to completion, or up to
/// `budget_ms`, WITHOUT the #430 isolate leak: pump the event loop one
/// self-contained tick at a time (`poll_event_loop` exits the isolate per
/// call) and poll the `mod_evaluate` future alongside it, instead of
/// cancelling a `run_event_loop` future with a tokio timeout (a fired timeout
/// drops that future mid-poll and leaves the isolate entered on the thread).
/// A module eval error or timeout is logged under `what` and swallowed:
/// neither is fatal to rendering the rest of the page.
/// `budget_ms`. Returns as soon as the module finishes rather than waiting
/// for the event loop to go idle: a page timer (setInterval) keeps the loop
/// busy forever and would otherwise burn the whole budget, abandoning a
/// module that had already evaluated (issue #374).
///
/// A module eval error or a timeout is logged under `what` and swallowed:
/// neither is fatal to rendering the rest of the page. An event-loop error
/// is propagated out of the select and handled the same way -- it must not
/// be discarded, or a module stalled on a top-level await spins here for the
/// whole budget with nothing logged.
async fn drive_module_eval(&mut self, module_id: deno_core::ModuleId, budget_ms: u64, what: &str) {
let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(budget_ms);
let mut result = Box::pin(self.runtime.mod_evaluate(module_id));
loop {
let done = std::future::poll_fn(|cx| {
let _ = self
.runtime
.poll_event_loop(cx, deno_core::PollEventLoopOptions::default());
std::task::Poll::Ready(match std::future::Future::poll(result.as_mut(), cx) {
std::task::Poll::Ready(r) => Some(r),
std::task::Poll::Pending => None,
})
})
.await;
match done {
Some(Ok(())) => break,
Some(Err(e)) => {
tracing::warn!("{} eval error: {}", what, e);
break;
}
None => {
if tokio::time::Instant::now() >= deadline {
tracing::warn!("{} evaluation timed out after {}ms", what, budget_ms);
break;
}
tokio::time::sleep(tokio::time::Duration::from_millis(1)).await;
}
let budget = tokio::time::Duration::from_millis(budget_ms);
let result = self.runtime.mod_evaluate(module_id);
tokio::pin!(result);
let outcome = tokio::time::timeout(budget, async {
let event_loop = self
.runtime
.run_event_loop(deno_core::PollEventLoopOptions::default());
tokio::pin!(event_loop);
tokio::select! {
biased;
r = &mut result => r,
e = &mut event_loop => { e?; (&mut result).await }
}
})
.await;
match outcome {
Ok(Ok(())) => {}
Ok(Err(e)) => tracing::warn!("{} eval error: {}", what, e),
Err(_) => tracing::warn!("{} evaluation timed out after {}ms", what, budget_ms),
}
}
@@ -862,31 +859,6 @@ impl ObscuraJsRuntime {
.map_err(|e| format!("Event loop error: {}", e))
}
/// One self-contained event-loop poll (cancellation-safe). `poll_event_loop`
/// creates and drops its own isolate scope within the call, so this future
/// holds no V8 scope across its (single, immediate) await point: dropping it
/// can never leave the isolate ENTERED on the thread. Callers that pump in a
/// loop with a per-tick tokio timeout MUST use this instead of
/// `run_event_loop`, whose future, dropped mid-poll, leaks the entered
/// isolate and aborts the process under concurrent pages (#430).
///
/// Returns `Some(result)` if the loop settled this tick, `None` if work is
/// still pending.
pub async fn poll_event_loop_once(&mut self) -> Option<Result<(), String>> {
std::future::poll_fn(|cx| {
std::task::Poll::Ready(
match self
.runtime
.poll_event_loop(cx, deno_core::PollEventLoopOptions::default())
{
std::task::Poll::Ready(r) => Some(r.map_err(|e| e.to_string())),
std::task::Poll::Pending => None,
},
)
})
.await
}
/// Arm a hard wall-clock backstop on synchronous V8 work. A page stuck in a
/// synchronous loop or a microtask storm pins the OS thread inside V8, so
/// `tokio::time::timeout` (which can only cancel at await points) never
@@ -927,50 +899,20 @@ impl ObscuraJsRuntime {
/// idle (tokio timeout) and synchronous hangs (V8 watchdog). A microtask
/// storm that pins the thread is terminated ~500ms past the budget; a
/// well-behaved page returns as soon as the loop goes idle.
/// Drive the event loop until it settles (idle or error) or `deadline`
/// passes, WITHOUT the isolate-leak hazard of cancelling `run_event_loop`
/// with a tokio timeout. `JsRuntime::poll_event_loop` creates and drops its
/// own isolate scope on every call, so a `poll_fn` that only calls it holds
/// no V8 scope between polls; racing that `poll_fn` against a timer and
/// dropping the loser can never leave the isolate ENTERED on the OS thread.
/// Dropping a `run_event_loop` future mid-poll can, and that is the #430
/// abort (`heap->isolate() == Isolate::TryGetCurrent()`) once a second page
/// enters its own isolate on the same thread.
///
/// Returns `Some(result)` if the loop settled within the deadline, `None`
/// if the deadline fired first (the normal "page still has live timers"
/// exit).
async fn pump_event_loop_until(
&mut self,
deadline: tokio::time::Instant,
) -> Option<Result<(), String>> {
let pump = std::future::poll_fn(|cx| {
self.runtime
.poll_event_loop(cx, deno_core::PollEventLoopOptions::default())
});
tokio::pin!(pump);
tokio::select! {
biased;
r = &mut pump => Some(r.map_err(|e| e.to_string())),
_ = tokio::time::sleep_until(deadline) => None,
}
}
pub async fn run_event_loop_bounded(&mut self, budget_ms: u64) -> Result<(), String> {
if budget_ms == 0 {
return self.run_event_loop().await;
}
let budget = std::time::Duration::from_millis(budget_ms);
let deadline = tokio::time::Instant::now() + budget;
let token = self.arm_watchdog(budget + std::time::Duration::from_millis(500));
let result = self.pump_event_loop_until(deadline).await;
let result = tokio::time::timeout(budget, self.run_event_loop()).await;
self.disarm_watchdog(token);
match result {
// deadline fired: the normal "settled / live timers remain" exit.
None => Ok(()),
Some(Ok(())) => Ok(()),
Some(Err(e)) if e.contains("execution terminated") => Ok(()),
Some(Err(e)) => Err(e),
Ok(Ok(())) => Ok(()),
Ok(Err(e)) if e.contains("execution terminated") => Ok(()),
Ok(Err(e)) => Err(e),
// tokio idle-timeout is the normal "settled" exit, not an error.
Err(_) => Ok(()),
}
}
@@ -1004,11 +946,11 @@ impl ObscuraJsRuntime {
}
pub async fn resolve_promises(&mut self) {
// Default settle: pump until idle or 5s, cancellation-safe so the
// isolate is never left entered on the thread (#430, see
// pump_event_loop_until).
let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_secs(5);
let _ = self.pump_event_loop_until(deadline).await;
// Default settle: just pump until idle or 5s.
let _ = tokio::time::timeout(
tokio::time::Duration::from_secs(5),
self.runtime.run_event_loop(deno_core::PollEventLoopOptions::default()),
).await;
}
/// Pump the event loop until `done_check` returns true (e.g. an IIFE
@@ -1035,28 +977,14 @@ impl ObscuraJsRuntime {
if tokio::time::Instant::now() >= deadline {
return;
}
// Drive the event loop one self-contained tick at a time via
// `poll_event_loop` instead of cancelling `run_event_loop` with a
// tokio timeout. A timeout that fires DROPS the `run_event_loop`
// future mid-poll and can leave this isolate ENTERED on the OS
// thread; the next page's `execute_script` then trips V8's
// `heap->isolate() == Isolate::TryGetCurrent()` and aborts the whole
// process under concurrent CDP work (#430). `poll_event_loop`
// creates and drops its own isolate scope within the call, so the
// isolate is always exited between ticks — the only await point
// below (the sleep) runs with no isolate entered.
let _ = std::future::poll_fn(|cx| {
std::task::Poll::Ready(
self.runtime
.poll_event_loop(cx, deno_core::PollEventLoopOptions::default())
.is_ready(),
)
})
.await;
// Backoff so a hung promise doesn't burn CPU, and so long-lived
// timers don't starve the predicate re-check (why this helper
// exists). Caps at 50ms; worst case we miss the result by <50ms.
tokio::time::sleep(tokio::time::Duration::from_millis(tick_ms)).await;
// Pump for a short slice. If the loop returns idle in <tick_ms,
// run_event_loop returns Ok and we check the predicate again.
let _ = tokio::time::timeout(
tokio::time::Duration::from_millis(tick_ms),
self.runtime.run_event_loop(deno_core::PollEventLoopOptions::default()),
).await;
// Backoff so a hung promise doesn't burn CPU. Caps at 50ms;
// worst case we miss the result by <50ms.
if tick_ms < 50 { tick_ms = (tick_ms * 2).min(50); }
}
}