relay icon indicating copy to clipboard operation
relay copied to clipboard

[relay-lsp] Ensure documents are synced before calculating completions

Open tobias-tengler opened this issue 8 months ago • 6 comments

Fixes #4472

tobias-tengler avatar Oct 07 '23 20:10 tobias-tengler

Interesting! Looks reasonable, but are you able to validate this change on its own, or do we need the VSCode fix as well?

captbaritone avatar Oct 08 '23 15:10 captbaritone

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.

tobias-tengler avatar Oct 08 '23 17:10 tobias-tengler

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

tobias-tengler avatar Oct 23 '23 20:10 tobias-tengler

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 🤩

tobias-tengler avatar Oct 25 '23 13:10 tobias-tengler

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 😅

tobias-tengler avatar Nov 06 '23 20:11 tobias-tengler

@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
before-completions
Completions are shown from wrong context, because the completions request finishes faster than the documentChange notification that precedes it
after-completions
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

tobias-tengler avatar Jan 30 '24 22:01 tobias-tengler