Skip to content

fix: Cancel ongoing build scripts/workspace discovery on exit#22738

Open
ChayimFriedman2 wants to merge 1 commit into
rust-lang:masterfrom
ChayimFriedman2:kill-on-exit
Open

fix: Cancel ongoing build scripts/workspace discovery on exit#22738
ChayimFriedman2 wants to merge 1 commit into
rust-lang:masterfrom
ChayimFriedman2:kill-on-exit

Conversation

@ChayimFriedman2

Copy link
Copy Markdown
Contributor

Flycheck and proc macro server process handles are stored in the GlobalState, using wrappers (JodChild or CommandHandle) that kill the process on drop. So when we exit and GlobalState is dropped, they're automatically cancelled.

However, build scripts and workspace discovery are triggered inside thread pool tasks. When GlobalState is dropped, they're left untouched, and when r-a process exits and the handles are destroyed, the processes are detached.

The way I went to solve this is by adding an option to cancel all tasks on a thread pool, by injecting a panic into it. Of course you cannot force a thread to panic (this has the same problems as killing a thread), so instead the thread must check some state periodically. Most of our tasks are have short execution time before executing the next database query, so we can cancel the database, making Salsa inject a panic. Except those that invoke child processes.

So I went and changed JodChild to also check periodically for cancellation. This is non-trivial because the standard library has neither "read with timeout" nor "wait with timeout" primitives for pipes, so we're forced into platform-specific code. I also introduced a JodCommand wrapper around Command to make it harder to use waiting operations by mistake, and added Command and Child to clippy::disallowed_types.

This was still not enough, because the extension itself has not delivered the shutdown event correctly, and also had a too short timeout for it (200ms). So I changed it to send a shutdown request in deactivate(), and increased the timeout to 2s.

I have no idea how to test this automatically, but I tested this manually by setting a build command that waits forever then closing VSCode. Before those changes, the build process is still there; after them, it is killed properly.

Fixes #22732.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2026
@ChayimFriedman2 ChayimFriedman2 force-pushed the kill-on-exit branch 7 times, most recently from 9a993b2 to 0335d89 Compare July 8, 2026 03:15
@ChayimFriedman2 ChayimFriedman2 marked this pull request as draft July 8, 2026 03:16
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2026
@ChayimFriedman2 ChayimFriedman2 force-pushed the kill-on-exit branch 4 times, most recently from b22d17c to 4cbf3df Compare July 8, 2026 05:27
Flycheck and proc macro server process handles are stored in the `GlobalState`, using wrappers (`JodChild` or `CommandHandle`) that kill the process on drop. So when we exit and `GlobalState` is dropped, they're automatically cancelled.

However, build scripts and workspace discovery are triggered inside thread pool tasks. When `GlobalState` is dropped, they're left untouched, and when r-a process exits and the handles are destroyed, the processes are detached.

The way I went to solve this is by adding an option to cancel all tasks on a thread pool, by injecting a panic into it. Of course you cannot force a thread to panic (this has the same problems as killing a thread), so instead the thread must check some state periodically. Most of our tasks are have short execution time before executing the next database query, so we can cancel the database, making Salsa inject a panic. Except those that invoke child processes.

So I went and changed `JodChild` to also check periodically for cancellation. This is non-trivial because the standard library has neither "read with timeout" nor "wait with timeout" primitives for pipes, so we're forced into platform-specific code. I also introduced a `JodCommand` wrapper around `Command` to make it harder to use waiting operations by mistake, and added `Command` and `Child` to clippy::disallowed_types.

This was still not enough, because the extension itself has not delivered the shutdown event correctly, and also had a too short timeout for it (200ms). So I changed it to send a shutdown request in `deactivate()`, and increased the timeout to 2s.

I have no idea how to test this automatically, but I tested this manually by setting a build command that waits forever then closing VSCode. Before those changes, the build process is still there; after them, it is killed properly.
@ChayimFriedman2 ChayimFriedman2 marked this pull request as ready for review July 8, 2026 05:46
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build process keeps running in the background after closing vscode

2 participants