Remove interrupt tasks entirely by DavisVaughan · Pull Request #1222 · posit-dev/ark

DavisVaughan · 2026-05-19T15:14:23Z

Wait for the next Positron release, merge 1 day after that so we have time to test

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.

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

Just to be extra safe

We have some indirect tests that get at this too, but a direct test feels quite useful and explanatory

DavisVaughan · 2026-05-19T15:15:07Z

-        r_task::spawn(RTask::interrupt({
+        //
+        // Use `idle_any_prompt` so DAP breakpoint invalidation still fires when
+        // the LSP signals a document change while R is paused at `browser()`.
+        r_task::spawn(RTask::idle_any_prompt({
            let dap_clone = console.debug_dap.clone();
-            async move || {
+            async move |_| {
                Console::process_console_notifications(console_notification_rx, dap_clone).await


Added a test for this as well in dap_breakpoints.rs

DavisVaughan · 2026-05-19T15:16:52Z

+        // For Sync tasks (i.e. only `r_task()`s), we want to log excessive waiting,
+        // because we are blocking the calling thread
+        if matches!(task, QueuedRTask::Sync(_)) {


We used to log "excessive waiting" for interrupt tasks

We don't have interrupt tasks anymore, but we do have Sync tasks, which are only used by r_task().

I thought it was appropriate to change this to log excessive waiting for r_task()s, since those are blocking, and since they previously were interrupt tasks. So it felt like the spirit was the same.

DavisVaughan · 2026-05-19T15:23:48Z

@@ -2475,33 +2503,6 @@ impl Console {
        if let Some(text) = self.debug_filter.check_timeout() {
            self.emit_stdout(text);
        }


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)

DavisVaughan · 2026-05-19T15:24:15Z

-        // Run pending finalizers. We need to do this eagerly as otherwise finalizers
-        // might end up being executed on the LSP thread.
-        // https://github.com/rstudio/positron/issues/431
-        unsafe { R_RunPendingFinalizers() };


No longer needed since we don't run R code off the main R thread

DavisVaughan · 2026-05-19T15:26:14Z

-    // NOTE: This is a legit use of interrupt-time task. No R access here, and
-    // we need to go through Console since it owns the UI comm.
-    r_task(|| {
-        if let Some(ui) = Console::get().ui_comm() {
-            ui.show_message(String::from(user_message));
-        }
+    let request = client.send_request::<request::ShowMessageRequest>(ShowMessageRequestParams {
+        typ: MessageType::ERROR,
+        message: String::from(user_message),
+        actions: None,
    });
+    match tokio::time::timeout(Duration::from_secs(5), request).await {
+        Ok(result) => {
+            result.log_err();
+        },
+        Err(_) => {
+            log::warn!("Timed out waiting for frontend to acknowledge LSP crash notification");
+        },
+    }


report_crash() moves off needing to use the ui comm!

DavisVaughan · 2026-05-19T15:27:15Z

+    /// For rare performance sensitive cases where you'd like to avoid the cost of
+    /// poking R options via [Self::pin_with_capture()] and you know you don't need
+    /// the safety of capturing output because you aren't running R code
+    pub(crate) fn send_idle_without_capture<F, Fut>(fun: F) -> Self


I did end up wanting this for Drop in RThreadSafe

DavisVaughan · 2026-05-19T15:28:17Z

No longer relevant since R_PolledEvents is a no-op if no one sets it, and we don't set this anymore

DavisVaughan · 2026-05-19T15:28:51Z

+    // TODO: These days this just means `RLocalInterruptsSuspended`.
+    // Should we simplify the name from `r_sandbox()` to `r_interrupts_suspended()`?
    let _scope = crate::raii::RLocalSandbox::new();


I was curious about your thoughts on this, since r_sandbox() does less now

DavisVaughan added 11 commits May 15, 2026 11:28

Note why it is preferable to never supply NULL to R_runHandlers()

6cdcae8

Implement report_crash() via ShowMessageRequest

707ecd2

Move r_task() to IDLE_ANY_TASKS and drop INTERRUPT_TASKS

7b22393

Tweak comment

c4548a4

Use idle_any_prompt() for process_console_notifications()

9ffbc42

So LSP didChange notifications can invalidate breakpoints while paused in the debugger

report_crash() now has a 5 second timeout

835b75c

Just to be extra safe

Directly test that r_task() runs at browser() prompts

7ee58e7

We have some indirect tests that get at this too, but a direct test feels quite useful and explanatory

Use slightly more standard "inner" suffix

5bc20f6

Tweak message

5aa75f9

DavisVaughan commented May 19, 2026

View reviewed changes

DavisVaughan added 2 commits May 19, 2026 11:30

Fix clippy lint

73798ce

check_timeout() is now used on all platforms

36b248f

DavisVaughan requested a review from lionel- May 19, 2026 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove interrupt tasks entirely#1222

Remove interrupt tasks entirely#1222
DavisVaughan wants to merge 13 commits into
mainfrom
feature/task-timing

DavisVaughan commented May 19, 2026 •

edited

Loading

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

DavisVaughan May 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

DavisVaughan commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

1 participant

DavisVaughan commented May 19, 2026 •

edited

Loading