relay
relay copied to clipboard
[relay-lsp] Ensure documents are synced before calculating completions
Fixes #4472
Interesting! Looks reasonable, but are you able to validate this change on its own, or do we need the VSCode fix as well?
I would say it catches 40% of the race conditions between textdocument/didchange
and textdocument/completion
. I validated this in my testing by checking the occurence of the debug statement I added.
It definitely also needs a fix or input on a strategy to use from the VSCode team though. I'll try to create an issue with a reproduction for them today or over the course of this week.
It seems like all the issues were being caused by the TaskQueue essentially fire-and-forgetting the processing of tasks. I opted to execute all the notification tasks concerning the document content, e.g. textdocument/didChange
, serially i.e. blocking so that no other tasks can be processed while the content of a text document is being updated. All features relying on the current state of the text document should now no longer run into race conditions.
I also verified that this fixes the problems in my PR adding folding features: #4480
Have been using a local build of the LSP (from this PR) in our project for two days now and can verify that I haven't had a wrong completion since 🤩
While this already works really reliably, I'd like to extend the solution with something a little bit more sophisticated to also wait for ongoing textdocument/*
requests to finish, before processing notifications that alter the document state serially.
Will need to smarten up on Rust synchronization mechanisms first though 😅
@captbaritone @alunyov this would be ready to review :)
I've also made a quick recording to showcase what exactly this solves (it might also be the source of other subtle LSP bugs):
Before | After |
---|---|
Completions are shown from wrong context, because the completions request finishes faster than the documentChange notification that precedes it |
Completions are shown in correct context, as notifications that modify the LSP state are now executed in isolation (serially) and other requests stay in the queue until the notification has been processed |