Fix a race condition when setting diagnostic signs #949
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.
I believe that this will fix #927
I was seeing an issue where the gutter diagnostic signs would often stick around even after the errors had been cleared.
The issue was that our order of operations was to
LanguageClient-neovim/src/language_server_protocol.rs
Line 2519 in 90e49e5
LanguageClient-neovim/src/language_server_protocol.rs
Lines 2528 to 2529 in 90e49e5
LanguageClient-neovim/src/language_server_protocol.rs
Lines 2550 to 2551 in 90e49e5
LanguageClient-neovim/src/language_server_protocol.rs
Line 2552 in 90e49e5
The update is calculated from the state read in step 1, but only applied in step 4. And it's sent to vim in between. So if there are two threads that run as follows:
Thread B will calculate a diff that doesn't take into account the operations from Thread A. They will both send signs to vim, and both try to store their signs in the state. Vim stores those signs by ID, but the state stores them by line number. If two of these new signs have the same line number, only one of them will end up in the state, effectively leaking the other sign.
My fix is to move all of these steps into a single
self.updateblock, so that it's all one atomic operation. Since we're reading and updating as a single transaction, it should be impossible to get into a bad state now.This made the most sense to me, but I'm not sure if it's the right approach. Happy to refactor if there's a better way to fix it :)