Fix LSP hangs - #1369
Merged
Merged
Conversation
r-120d1428 [R] 2026-07-29T16:01:40.429044Z WARN Cannot read sources directory: C:\Users\runneradmin\AppData\Local\oak\source/v1/r\v1_4.6.1\datasets\R r-120d1428 [R] at crates\oak_scan\src\packages.rs:223 And add some behavioural tests
lionel-
force-pushed
the
oak-sources/1-background-hang
branch
from
July 30, 2026 13:12
59e1cf6 to
ba9db11
Compare
This was referenced Jul 30, 2026
Contributor
Author
|
Positron tests running at posit-dev/positron#15229 |
jmcphers
approved these changes
Jul 31, 2026
jmcphers
left a comment
Contributor
There was a problem hiding this comment.
LGTM, and in my testing this made the statement range provider actually responsive even during indexing (judging on CPU activity). Nice work.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
From analysing logs, the LSP hangs in posit-dev/positron#15211 were due to #1352.
We're now extracting R sources from installed packages, or downloading them from CRAN in blocking tasks. Since the handling is abstracted over different handlers (R process versus CRAN downloads), it made sense to use a blocking task instead of fetching sources in async tokio tasks.
Importantly, the blocking thread pool was capped at 2 jobs.
Background diagnostics were also launched on the blocking thread pool. These diagnostic tasks hold a read-only Salsa database handle. If source-fetching tasks were hogging up the blocking thread pool, the diagnostic jobs would be parked and hold a ref to the DB.
If a write notification (e.g. user typed) arrives in the meantime, the main LSP loop takes a write
&muthandle to the DB, which causes Salsa to notify a cancellation and blocks until all readers have finished. But diagnostic jobs parked behind fetching jobs never read the Salsa DB, and thus never get cancelled and never drop their read DB handle. The fetching job are not cancelled by Salsa either. Hence the hang.Measures taken to fix this:
Background diagnostics are now run on a dedicated thread pool that we manage ourselves, instead of via tokio. This thread pool is exclusively for tasks that use Salsa, or otherwise actively check for cancellation. This prevents the thread pool from being blocked by uncancellable tasks. The thread pool uses 4 workers at max, capped with
std::thread::available_parallelism().Two other thread pools of type
IoPool: one for the file scanner (capped at 1 for now), and one for fetching sources (2 threads). This way fetching sources can never block other tasks. I considered moving them to the tokio async pool but this would have required more work with the R process handler. I'd prefer to wait until Davis' return to discuss this.Hopefully this PR fixes this class of hangs. If it doesn't fix it fully, log reports should be clear about what's going on and we shouldn't have to deduce events from partial indications. A new watchdog thread checks that a main loop tick doesn't take more than 5 secs. After that deadline, it logs every 5 secs in prod, or aborts the process in tests.
That's a lot of new threads! By default each takes up 2mb of stack space. So I've introduced a new
stdext::spawn_with_stack_size()helper. We use it to spawn 512 kb (SMALL_STACK_SIZE) threads, such as the scanner threads, and 256kb (TINY_STACK_SIZE) threads, such as for the watchdog thread. Our existing stream capture thread now also uses a tiny stack size.Positron Release Notes
New Features
Bug Fixes