fix: Cancel ongoing build scripts/workspace discovery on exit#22738
Open
ChayimFriedman2 wants to merge 1 commit into
Open
fix: Cancel ongoing build scripts/workspace discovery on exit#22738ChayimFriedman2 wants to merge 1 commit into
ChayimFriedman2 wants to merge 1 commit into
Conversation
9a993b2 to
0335d89
Compare
b22d17c to
4cbf3df
Compare
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.
4cbf3df to
6414634
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Flycheck and proc macro server process handles are stored in the
GlobalState, using wrappers (JodChildorCommandHandle) that kill the process on drop. So when we exit andGlobalStateis dropped, they're automatically cancelled.However, build scripts and workspace discovery are triggered inside thread pool tasks. When
GlobalStateis 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
JodChildto 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 aJodCommandwrapper aroundCommandto make it harder to use waiting operations by mistake, and addedCommandandChildto 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.