Run Evaluate requests with timeout by lionel- · Pull Request #1309 · posit-dev/ark

lionel- · 2026-07-01T15:47:58Z

Adds with_timeout() to run R code with a timeout and use that in the Evaluate handlers (in notebook and console paths). This prevents an inflooping (or long-running) expression in the Watch Pane from freezing the kernel.

with_timeout() runs a closure in a context where interrupts and polled events are restored. This allows R to check for interrupts, and allows polled events to check for a timeout.
We check for a timeout from polled events to avoid the complexity of checking time in a side thread. When a timeout is detected, we record that fact in a thread-local variable and signal an R interrupt.
The interrupt is either caught by with_timeout()'s try_catch(), or by an inner one running in the closure. Either way, with_timeout() returns whether the expression timed out to its caller.

This setup allows inner code to recover from the interrupt, which is not ideal. However this is much simpler than trying to propagate the interrupt with a Rust panic, since the latter approach would require making sure the panic does not cross a C stack which would be UB and likely to corrupt R's state. The simpler approach of propagating with Rust errors could in principle lead to surprising behaviour, but is simpler and safer to implement.

Regarding the safety of longjumping from polled events with an R interrupt, this should be sufficiently safe because:

This is equivalent to an R-level error and should be caught the same way by proper safeguards like try_catch().
This can only happen from within with_timeout(), which is a controlled environment that expects the interrupt to fire any time R is called.

We should still assume it's possible for some Rust destructors to get bypassed by the interrupt if the calling context is not super vigilant about wrapping in try_catch(). However the alternative of having Ark freeze with runaway evals is much worse.

This is Unix-only until we figure out #1222

DavisVaughan

Mostly want to talk through the ultimate plan with #1222 before committing to this, because relying on being able to poke an R API from polled-events / process-events might box us in a bit.

DavisVaughan · 2026-07-02T19:49:50Z

        }

+        // Interrupt any in-flight evaluations that have outlived their timeout
+        crate::timeout::check_timeout();


But if we do #1222, where does this go?

#1222 removed polled_events() entirely.

In the current state of that PR, we are left with a process_events() hook for the debug_filter to still be checked occasionally (#1222 (comment)) but as you discovered in #1222 (comment) we shouldn't actually take over ptr_R_ProcessEvents because other packages like Quartz may use it instead.

So we are going to have to do something else there. I really wasn't thrilled with the idea of setting Callback (i.e. R_ProcessEvents) on Windows and R_PolledEvents on Unix as mentioned in #1222 (review). I remember sending this slack message about it

i would really rather not mix * setting `R_ProcessEvents` on Windows via `CallBack` * setting `R_PolledEvents` on Unix that sounds like something that is likely to just confuse me greatly in the future, and is part of what im trying to get away from in this pr. it is very hard to keep what these do (and when they are called) straight in my head since: * we cant use `R_PolledEvents` on windows (doesnt exist) * we cant use `R_ProcessEvents` on mac (Quartz needs it) and because we only have 1 thing we need to regularly check, the `debug_filter` thing, i vote we get out of the business of setting `R_PolledEvents` and `R_ProcessEvents` at all and instead do one of two things * the side thread that checks `debug_filter` and eat the 2mb cost * drop caring about `debug_filter` at all for this nice use case. it isn't worth this amount of trouble IMO. IMO not setting `R_PolledEvents` and `R_ProcessEvents` would be a huge win for us in terms of complexity and never having to think about this again

With the main conclusion there being that it would be nice to not use EITHER R_PolledEvents or R_ProcessEvents, to try to avoid all of the funny business with those, and instead maybe use a "watcher" thread of some kind.

The watcher thread could probably handle debug_filter flushing, but I don't think it could directly do crate::timeout::check_timeout() since that pokes an R API via crate::signals::set_interrupts_pending(true).

I'll also add that I'm not sure that calling this at R_PolledEvents / R_ProcessEvents time really reliably saves us. It still relies on someone calling these events handlers, and I'm pretty sure that arbitrary R code running in an Evaluate could just never check those, right? But maybe if that is the case, it means they wouldn't be able to respect an interrupt request either, so that's okay?

So I'm not really sure what the right thing to do is! But I would hate to add this and box ourselves out from being able to finish off #1222 when we were soooooo close to getting rid of interrupt tasks, so I think I'd like us to have a plan before merging this.

DavisVaughan · 2026-07-02T19:59:38Z

+            // A longjump that escaped `DapHandler::evaluate()`'s own `try_catch`
+            // (e.g. while building the response), caught by the try-idle sandbox.
+            Err(err) => return Ok(self.error_response(seq, "evaluate", &err.to_string())),


It looks to me like DapHandler::evaluate cannot actually jump.

Instead DapHandler::evaluate has this exact kind of comment about Dap::evaluate, so I would have thought that this Err case would have already been handled fully by DapHandler::evaluate

DavisVaughan · 2026-07-02T20:00:18Z

+            // An R longjump that escaped `Dap::evaluate()`'s own `try_catch`.
+            Err(err) => return EvaluateOutcome::Error(err.to_string()),


This is the comment i was referring too, this seems like where the longjmp is actually handled?

DavisVaughan · 2026-07-02T20:01:51Z

    }

-    done_rx.recv().log_err()?;
+    if done_rx.recv().is_err() {


is an Err normal behavior here?

DavisVaughan · 2026-07-02T20:03:26Z

            let rsp = match DapHandler::evaluate(&state, &expression, frame_id, &mut capture) {
-                Ok(body) => req.success(body),
-                Err(err) => req.error(&format!("Error: {err}")),
+                EvaluateOutcome::Ok(body) => req.success(body),
+                EvaluateOutcome::TimedOut => req.error("Evaluation timed out"),
+                EvaluateOutcome::Error(err) => req.error(&format!("Error: {err}")),


Yea like here you are relying on DapHandler::evaluate to fully handle that longjmp case for you, I think, and translate it into a EvaluateOutcome::Error already.

So that other reference in dap_jupyter_handler.rs to longjmp does seem not quite right? I think?

DavisVaughan · 2026-07-02T20:04:46Z

+use harp::raii::RLocalInterruptsSuspended;
+
+/// How long an evaluation in `with_timeout()` may run before we interrupt it.
+pub(crate) const EVAL_TIMEOUT: Duration = Duration::from_secs(1);


That's pretty darn aggressive for slow windows machines! 😬

hmm maybe we should bump that on CI to avoid flakes? The main thing I want to avoid is 10 Watch Pane expressions hanging the console for 30sec

DavisVaughan · 2026-07-02T20:08:10Z

+    let _polled = harp::raii::RLocal::new(
+        Some(crate::console::r_polled_events as unsafe extern "C-unwind" fn()),
+        unsafe { libr::R_PolledEvents },
+    );


hmm you kind of want the opposite of RLocalPolledEventsSuspended, but that's a harp thing...

lionel- added 2 commits July 1, 2026 15:33

Run notebook Evaluate requests with timeout

59d33fc

Run console Evaluate requests with timeout

e9e6620

lionel- requested a review from DavisVaughan July 1, 2026 15:57

DavisVaughan reviewed Jul 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Run Evaluate requests with timeout#1309

Run Evaluate requests with timeout#1309
lionel- wants to merge 2 commits into
mainfrom
debugger/evaluate-timeout

lionel- commented Jul 1, 2026

Uh oh!

DavisVaughan left a comment

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

lionel- Jul 3, 2026

Uh oh!

DavisVaughan Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// An R longjump that escaped `Dap::evaluate()`'s own `try_catch`.
		Err(err) => return EvaluateOutcome::Error(err.to_string()),

Uh oh!

Conversation

lionel- commented Jul 1, 2026

Uh oh!

DavisVaughan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants