Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

tobias-tengler
Copy link
Contributor

@tobias-tengler tobias-tengler commented Oct 7, 2023 •
edited
Loading

Fixes #4472

@tobias-tengler tobias-tengler changed the title Ensure documents are synced before calculating completions [relay-lsp] Ensure documents are synced before calculating completions Oct 7, 2023
@tobias-tengler tobias-tengler mentioned this pull request Oct 7, 2023
Open
@captbaritone
Copy link
Contributor

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

@tobias-tengler
Copy link
Contributor Author

tobias-tengler commented Oct 8, 2023 •
edited
Loading

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 tobias-tengler marked this pull request as draft October 8, 2023 20:30
@tobias-tengler tobias-tengler force-pushed the lock-completions branch 2 times, most recently from d92878d to 3faf115 Compare October 23, 2023 20:00
@tobias-tengler tobias-tengler marked this pull request as ready for review October 23, 2023 20:01
@tobias-tengler
Copy link
Contributor Author

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 tobias-tengler mentioned this pull request Oct 24, 2023
Open
@tobias-tengler
Copy link
Contributor Author

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
Copy link
Contributor Author

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 tobias-tengler marked this pull request as draft January 13, 2024 19:37
@tobias-tengler tobias-tengler force-pushed the lock-completions branch 2 times, most recently from b8ef5e9 to b0e6731 Compare January 30, 2024 21:53
@tobias-tengler tobias-tengler marked this pull request as ready for review January 30, 2024 21:53
@tobias-tengler
Copy link
Contributor Author

@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

@facebook-github-bot
Copy link
Contributor

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@captbaritone
Copy link
Contributor

Sorry for the delay here. We have an internal infra issue where our PR import bot is unable to import this diff. Actually getting this merged got lost in the internal discussion to resolve the import issue.

Let me see if I can just manually recreate the diff via copy/paste.

facebook-github-bot pushed a commit that referenced this pull request Jul 11, 2024
@captbaritone @facebook-github-bot
Ensure documents are synced before calculating completions #4473
2b50288 
Summary:
There's an infra issue with the Relay repo and our PR import code which meant the automated import (D58590396) failed.

 This is a manual import of #4472 needed due to an infra issue on Meta's side. Full credit to tobias-tengler for the contents of this commit.

Reviewed By: tyao1

Differential Revision: D59645988

fbshipit-source-id: ad252f0fa3229a9e45a8b779e4eb52ead7b5b7e8
@captbaritone
Copy link
Contributor

Landed as 2b50288

@captbaritone
Copy link
Contributor

Thanks again for this excellent PR and your patience!

monicatang pushed a commit to monicatang/relay that referenced this pull request Aug 1, 2024
@captbaritone @monicatang
Ensure documents are synced before calculating completions facebook#4473
82e322f 


Summary:
There's an infra issue with the Relay repo and our PR import code which meant the automated import (D58590396) failed.

 This is a manual import of facebook#4472 needed due to an infra issue on Meta's side. Full credit to tobias-tengler for the contents of this commit.

Reviewed By: tyao1

Differential Revision: D59645988

fbshipit-source-id: ad252f0fa3229a9e45a8b779e4eb52ead7b5b7e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[relay-lsp] Suggestions from wrong context
3 participants
@tobias-tengler @captbaritone @facebook-github-bot