Skip to content

Remove interrupt tasks entirely#1222

Merged
DavisVaughan merged 23 commits into
mainfrom
feature/task-timing
Jul 8, 2026
Merged

Remove interrupt tasks entirely#1222
DavisVaughan merged 23 commits into
mainfrom
feature/task-timing

Conversation

@DavisVaughan

@DavisVaughan DavisVaughan commented May 19, 2026

Copy link
Copy Markdown
Contributor

Closes #1187

In #1187 we realized that our "interrupt time" tasks have never been running on Windows, and...no one has complained.

Practically, this means that LSP features like completions or hover have not been running on Windows when R is busy, i.e. when R is running some long running computation, even if that code is checking for interrupts periodically via R_CheckUserInterrupt(). Unix machines, on the other hand, do return completions or hover results at interrupt time, but it is fairly unreliable and highly subject to the R code being run, i.e. this video shows how when you run a long running lm(), you can basically hang the LSP on a Mac because it almost never calls R_CheckUserInterrupt():

Screen.Recording.2026-05-15.at.12.21.07.PM.mov

We've long through that doing extensive work at R interrupt time is rather dangerous (you could accidentally load a namespace mid-R execution, which would be wild), so we've wanted to remove these. And with oak, we are going to rely on interrupt time less and less anyways, because more things can be done statically without talking to the main R session.

So this PR removes interrupt tasks entirely. I'll leave more notes inline.

Comment thread crates/ark/src/console/console_repl.rs
Comment thread crates/ark/src/console/console_repl.rs
Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment on lines 2496 to 2505
@@ -2475,33 +2503,6 @@ impl Console {
if let Some(text) = self.debug_filter.check_timeout() {
self.emit_stdout(text);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have kept the debug_filter check as part of an R_ProcessEvents() hook, which is now also being run on Windows.

This is now the ONLY THING special that we do at interrupt time. And it doesn't talk to the R API in any way, so should be very safe.

It's important we run this regularly-ish because the whole point is to not hold onto user output that might (for whatever reason) look like debug: output too long during long running computation

We talked about moving this to another thread, which would let us remove interrupt time hooks altogether. I still think that idea is kind of nice in theory, but I wasn't quite sure how that thread would work.

  • Does it just busy loop with a 25ms sleep between checks?
  • Also, it would force DebugFilterState to be wrapped in a Mutex so it could be shared between Console and this extra thread, and that felt gross

I think we should keep this as is for this PR and if we still feel like we want to move it to a separate thread, we should do a follow up PR and aim for the simplest strategy possible (or, again, explore removing this check entirely)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing that gives me pause about this thread is using up 2mb stack space just for that. If a tokio task, that becomes much nicer, with the huge caveat that now Console depends on tokio :P

Comment thread crates/ark/src/console/console_repl.rs
Comment thread crates/ark/src/lsp/backend.rs
Comment thread crates/ark/src/r_task.rs
Comment thread crates/harp/src/sys/unix/polled_events.rs Outdated
Comment thread crates/harp/src/exec.rs Outdated
@DavisVaughan DavisVaughan requested a review from lionel- May 19, 2026 16:05

@lionel- lionel- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a step in the right direction!

The main thing I'd like us to be careful about is to not break native infrastructure that could potentially be used outside Positron:

  • Native graphics devices: GraphApp-based on Windows, Cocoa/Quartz on macOS, X11 on Linux.
  • Less important but comes up in practice: tcl/tk GUIs. R prompts user for a CRAN repo with a tcl/tk menu.

For some reason the tcltk package refuses to even load on my machine (R freezes) so I couldn't verify that it still works. But I could verify the Quartz graphics device no longer works, even though it does work on main. That's because Ark now sets ptr_R_ProcessEvents, see inline comment.

I did a deep dive in R's messy ecosystem of events loop (see below). I think we could simplify things and keep everything working with the following setup:

Windows:

pub fn pump_idle_events() {
    // R_ProcessEvents on Windows contains peekevent/doevent (GraphApp pump),
    // time limits, UserBreak, ptr_ProcessEvents (our hook), and R_Tcl_do.
    // No R_runHandlers / input-handler concept on Windows.
    unsafe { R_ProcessEvents() };
}

pub fn setup_r(args: &Vec<String>) {
    (*params).CallBack = Some(r_flush_debug_filter);
}

Unix:

pub fn pump_idle_events() {
    // ptr_R_ProcessEvents (Quartz), R_PolledEvents (debug_filter + tcltk),
    // R_CheckTimeLimits.
    unsafe { R_ProcessEvents() };

    // Drain ready input handlers (Quartz pipe, X11, later, help server).
    let mut fdset = unsafe { R_checkActivity(0, 1) };
    while !fdset.is_null() {
        unsafe { R_runHandlers(libr::get(R_InputHandlers), fdset) };
        fdset = unsafe { R_checkActivity(0, 1) };
    }

    // Match R's sys-std.c:1136 idiom: one final call with NULL fires
    // Rg_PolledEvents (Cairo X11 buffered display flush) and R_PolledEvents
    // (re-fires debug_filter + tcltk; both idempotent).
    unsafe { R_runHandlers(libr::get(R_InputHandlers), std::ptr::null_mut()) };
}

pub fn setup_r(args: &Vec<String>) {
    libr::set(R_PolledEvents, Some(r_flush_debug_filter));
    libr::set(R_wait_usec, 10000);
    // ptr_R_ProcessEvents intentionally not set as it's reserved for Quartz.
}

The other thing I'm wondering for a follow-up PR is if we should wait on the activity hanlder fds directly, on Unix. R_InputHandlers is a public linked list so it should be possible, I think Kevin had a comment about this (might have been deleted since): https://github.com/r-devel/r-svn/blob/714af4bf22bebd387437e31c397167996b4d5fbc/src/include/R_ext/eventloop.h#L93

The advantage would be that we'd run {later} callbacks and R's help server requests as soon as the events come in, without any timeout delays.


Notes from deep dive:

Interrupt-time: R_CheckUserInterrupt() calls R_ProcessEvents()

Unix R_ProcessEvents()

  • Ptr_R_ProcessEvents()
  • R_PolledEvents()
  • R_CheckTimeLimits()

Windows R_ProcessEvents()

  • GraphApp events
  • R_CheckTimeLimits()
  • Interrupt check
  • Ptr_R_ProcessEvents()
  • R_Tcl_do()

R_runHandlers (Unix only)

  • Run activity/input handlers registered by packages (e.g. grDevices on mac, later on linux and mac)
  • Runs only when R is idle, never when busy. Called by ReadConsole's Unix implementation.

Ptr_R_ProcessEvents

  • macOS: Set by Cocoa, only if pointer still NULL. We can't set this without breaking Cocoa GUI.
  • Windows: Mapped to Rp->Callback

R_PolledEvents (Unix only): Called from ProcessEvents()
-> Global pointer, only set by tcl/tk in stack-like fashion (calls old pointer)

Rg_PolledEvents: Separate hook for X11, called alongside R_PolledEvents when R_runHandlers is passed a NULL readMask (when R_checkActivity() returns NULL).

tcl/tk (Unix):

  • Interrupt-time: Sets R_PolledEvents on Unix (in a stack-like pattern, calling the old handler if any is set), R_Tcl_do on Windows
  • Idle: Does not register an activity input handler
  • Bidirectional: Sets a TCL event loop handler to call R_runHandlers when TCL is itself busy (e.g. running a modal dialog).

X11 (Unix): Runs on the R thread

  • Interrupt-time: nothing. X11 does not register Ptr_R_ProcessEvents, so X11
    windows freeze during R computation. (Cairo-buffered X11 devices additionally
    set Rg_PolledEvents = CairoHandler for periodic buffer flushes, but
    Rg_PolledEvents only fires from the legacy R_runHandlers(handlers, NULL)
    branch, which ark never invokes.)
  • Idle: Registers an activity input handler on the X11 connection fd
    (addInputHandler(R_InputHandlers, ConnectionNumber(display), R_ProcessX11Events, XActivity)). The X
    server pushes events to the connection fd, select() sees the fd ready,
    R_runHandlers dispatches to R_ProcessX11Events.

Quartz/Cocoa: Runs on the R thread

  • Interrupt-time: Registers Ptr_R_ProcessEvents if NULL
  • Idle-time: Registers an activity input handler for idle time events. Tickled by a background thread that sets a byte on the write end every 100ms.
  • If Ptr_R_ProcessEvents is NULL, also initialises Cocoa GUI in the current process, which sets it as a full-blown foreground process visible in the Dock.

HTML help server (Unix): Server runs on the R thread

  • Interrupt-time: Nothing, Help server is unresponsive when R is busy
  • Idle-time: Registers an activity input handler on the listening TCP socket
    (srv_input_handler).
    When a connection arrives, that handler calls
    accept() and registers another input handler on the per-connection
    socket (worker_input_handler) to serve the request. So each in-flight
    request has its own input handler.

HTML help server (Windows): Server runs in a separate thread.

later (Unix):

  • Interrupt-time: Nothing
  • Idle-time: Registers an activity input handler (that checks call stack is empty in case of debugger read-prompt)

later (Windows):

  • Spawns a message-only window that checks every 10ms

Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment on lines 2496 to 2505
@@ -2475,33 +2503,6 @@ impl Console {
if let Some(text) = self.debug_filter.check_timeout() {
self.emit_stdout(text);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing that gives me pause about this thread is using up 2mb stack space just for that. If a tokio task, that becomes much nicer, with the huge caveat that now Console depends on tokio :P

Comment thread crates/ark/src/console/console_repl.rs
Comment thread crates/ark/src/sys/unix/console.rs Outdated
libr::set(ptr_R_ShowMessage, Some(r_show_message));
libr::set(ptr_R_Busy, Some(r_busy));
libr::set(ptr_R_Suicide, Some(r_suicide));
libr::set(ptr_R_ProcessEvents, Some(r_process_events));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the quartz device on macOS because it checks that this hook is still NULL before initialising: https://github.com/r-devel/r-svn/blob/714af4bf22bebd387437e31c397167996b4d5fbc/src/library/grDevices/src/qdCocoa.m#L712-L713

options(device = "quartz")
plot(1)

While none of this is used in Positron, I think we should avoid unnecessary regressions on this front to make it easier to integrate Ark in other contexts, e.g. jupyter-based REPLs.

Comment thread crates/ark/src/console/console_repl.rs Outdated
if let Err(err) = r_sandbox(|| Console::get_mut().polled_events()) {
panic!("Unexpected longjump while polling events: {err:?}");
pub unsafe extern "C-unwind" fn r_process_events() {
if let Err(err) = r_sandbox(|| Console::get_mut().process_events()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the sandbox here, and document prominently that no R code should ever run in process_events()?

Or we keep it but mention here that is purely defensive against future changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document prominently that no R code should ever run in process_events()?

With your later PR that is going to poke R_interrupts_pending from inside here, this will no longer be true, so I'm going to keep the sandbox

Comment thread crates/harp/src/exec.rs Outdated
lionel- added a commit that referenced this pull request Jul 6, 2026
Progress towards #689 and
#691
Follow-up to #1161 (see
`console_comm.rs` changes)

See #1075 for context: we're moving
comms out of their own threads to run on the R thread, to simplify the
concurrency structure of Ark and remove the risk of concurrent R
evaluations which are UB (although these are now also tackled under a
different angle, see #1222).

In #1161 I made progress towards phasing out unsafe mutation on Console
state (see #1145) by using
interior mutability in Console methods. To manage comm state in
particular, I went for a take/move approach that removes a comm from the
map of comms owned by the Console, and passes the moved comm by value to
its handlers. This allowed data explorer comm handlers to manipulate the
comm map and insert new explorer comms (when an inline explorer gets
promoted to a full explorer).

Moving the comm was awkward with the UI comm though, which needed to be
pinned down because its handlers might cause Console events that
themselves require the UI comm (e.g. to send a message to the frontend).
For this reason the UI comm used a different dispatch strategy with a
shared borrow, that required ad hoc infrastructure.

The Help comm is also accessible from Console methods and is going to
need a similar setup, so I thought I'd go ahead and try to simplify
this:

- All comms now live in the map, including the UI comm. But they are now
wrapped in an `Rc`.

- The dispatcher now holds an `Rc` clone of the comm rather than an
exclusively owned value. The comm stays in the Console-owned map during
dispatch, which is how they remain accessible for reentrancy while a
handler is running. Thanks to the Rc wrapping and cloning, the comm map
remains fully owned by the Console during dispatch and can be
manipulated while a handler is running, allowing data explorers to
clone/promote themselves.

Note that the handler's `&mut` comes from interior mutability of the
comm handler inside the `ConsoleComm`. So the one invariant we need to
guarantee is that we never dispatch a comm's handler reentrantly.

- The UI comm still gets some ad hoc handling because of its "pinned"
nature (there can only be one UI comm at any time, so opening a new one
drops the old comm if any).
lionel- added a commit that referenced this pull request Jul 6, 2026
Branched from #1283 
Progress towards #689 and
#691

See #1075 for context: we're moving
comms out of their own threads to run on the R thread, to simplify the
concurrency structure of Ark and remove the risk of concurrent R
evaluations which are UB (although these are now also tackled under a
different angle, see #1222).

The help comm no longer has its own thread and event loop. Instead it
implements `CommHandler` and its handlers get run on the R thread.

- Like the UI comm, it's a pinned comm that is accessible via methods on
Console, so it uses the patterns established in #1283.

- `browseURL()` emits a Help event to the frontend. Now that the message
is emitted from the R thread, instead of the old comm thread, the
message is guaranteed to be emitted within the busy/idle window of the
execute-request of the `browseURL()` call. This sort of simplification
of message ordering is exactly what enables accurate and stable
protocol-level tests.

- New kernel-level integration test for the Help comm (progress towards
#1074).
We no longer need this. The potential issue was that we'd call some R code while on the LSP thread, and behind our back while on that LSP thread R would run finalizers that were only meant to be run on the main R thread. This can no longer happen because `r_task()`s are always run on the main R thread. There is no longer any R code that runs on the LSP thread.

We could run `R_RunPendingFinalizers()` ourselves more aggressively after each top level command finishes, but RStudio does not do this, so I don't currently see a need for us to either.
We now only run interrupt tasks at idle time, so in a follow up commit we can remove interrupt tasks as a concept.

We still run activity handlers regularly, particularly important for the later R package

We still call `R_ProcessEvents()` somewhat regularly while idling, mostly out of good faith to whatever R's default methods do, but this is also where our `self.debug_filter` draining occurs now - and this is newly now also being called on Windows, so that's good. I think if we do use `R_ProcessEvents()` for more going forward, it should only be for "non critical" things that can happen at any arbitrary time.
So LSP didChange notifications can invalidate breakpoints while paused in the debugger
We have some indirect tests that get at this too, but a direct test feels quite useful and explanatory
This seems a bit odd, because the location that `R_wait_usec` is defined at makes me think it would only be used by `R_PolledEvents()`, which we no longer set, but in reality it is also used in `Rsleep()` as the interval to check interrupts on
@DavisVaughan DavisVaughan force-pushed the feature/task-timing branch from 8fb98da to 5ab7a85 Compare July 7, 2026 14:02
…sole_notifications()` loop

We switched from `RTask::interrupt()` (no capture) to `RTask::idle_any_prompt()` (capture) for spawning this infinite `process_console_notifications()` event loop. That had the side effect of meaning that `options(warn = 1)` was set permanently for the user's R session, because this loop never closed and the `ConsoleOutputCapture` was not `Drop`ped until R quit.

This _also_ had the awful side effect of a panic on shutdown with this message `cannot access a Thread Local Storage value during or after destruction: AccessError`. `Console` itself was holding all `pending_futures`, including this permanent one. When R restarts, the R thread exits, the `CONSOLE` thread local destructor runs, and any `pending_futures` are dropped. This runs `ConsoleOutputCapture::drop` for any `pending_futures` that were started with capture support on, and `ConsoleOutputCapture::drop()` tries to access `CONSOLE` with `Console::get_mut()`. But since `CONSOLE` is already being destructed, we get this panic! Avoiding capture support entirely fixes this local issue, but we should probably also harden `Console::get_mut()` by using `CONSOLE.try_with()`, which I think we can use to build our own safer `with_borrow_mut()` variant (or at least throw a good panic message).
@DavisVaughan DavisVaughan requested a review from lionel- July 7, 2026 17:05

@lionel- lionel- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thanks for getting this over the line!

Comment thread crates/ark/src/console/console_filter.rs
Comment thread crates/ark/src/console/console_repl.rs Outdated
// Use idle-any so DAP breakpoint invalidation still fires when the LSP signals a
// document change while R is paused at `browser()`. Run without capture because
// this is a long running loop and we don't want capturing to permanently pin the
// `warn` global option to `1` for the lifetime of the loop. We don't emit any R

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should toggle warn when the future is parked/reentered, the same way we toggle capture.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #1322

Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment thread crates/ark/src/console/console_repl.rs Outdated
Comment thread crates/ark/src/console/console_repl.rs Outdated
@DavisVaughan DavisVaughan merged commit 73a5e8c into main Jul 8, 2026
17 checks passed
@DavisVaughan DavisVaughan deleted the feature/task-timing branch July 8, 2026 16:30
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

On Windows, call some equivalent to R_PolledEvents

2 participants