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.

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

question: why wait for initialized is needed here? #3904

Closed
alissa-tung opened this issue Apr 14, 2024 · 1 comment · Fixed by #4443
Closed

question: why wait for initialized is needed here? #3904

alissa-tung opened this issue Apr 14, 2024 · 1 comment · Fixed by #4443

Comments

@alissa-tung
Copy link
Contributor

See

discard $ st.hIn.readLspNotificationAs "initialized" InitializedParams

in some async language server implementation, for example, async-lsp, client may response the register_lean_watcher request (register_ilean_watcher) before sending initialized notification. So the client side router should take care of the sequence of messages, first sending initialized, then response the register_lean_watcher

@mhuisi
Copy link
Contributor

mhuisi commented Apr 15, 2024

Thanks for the bug report. We should probably just send the watcher requests after the initialization dance is done so that this race condition cannot occur.

github-merge-queue bot pushed a commit that referenced this issue Jun 13, 2024
This PR addresses some non-critical but annoying issues that sometimes
cause the language server to report an error:
- When using global search and replace in VS Code, the language client
sends `textDocument/didChange` notifications for documents that it never
told the server to open first. Instead of emitting an error and crashing
the language server when this occurs, we now instead ignore the
notification. Fixes #4435.
- When terminating the language server, VS Code sometimes still sends
request to the language server even after emitting a `shutdown` request.
The LSP spec explicitly forbids this, but instead of emitting an error
when this occurs, we now error requests and ignore all other messages
until receiving the final `exit` notification. Reported on Zulip several
times over the years but never materialized as an issue, e.g.
https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/Got.20.60shutdown.60.20request.2C.20expected.20an.20.60exit.60.20notification/near/441914289.
- Some language clients attempt to reply to the file watcher
registration request before completing the LSP initialization dance. To
fix this, we now only send this request after the initialization dance
has completed. Fixes #3904.

---------

Co-authored-by: Sebastian Ullrich <[email protected]>
github-actions bot pushed a commit that referenced this issue Jun 13, 2024
This PR addresses some non-critical but annoying issues that sometimes
cause the language server to report an error:
- When using global search and replace in VS Code, the language client
sends `textDocument/didChange` notifications for documents that it never
told the server to open first. Instead of emitting an error and crashing
the language server when this occurs, we now instead ignore the
notification. Fixes #4435.
- When terminating the language server, VS Code sometimes still sends
request to the language server even after emitting a `shutdown` request.
The LSP spec explicitly forbids this, but instead of emitting an error
when this occurs, we now error requests and ignore all other messages
until receiving the final `exit` notification. Reported on Zulip several
times over the years but never materialized as an issue, e.g.
https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/Got.20.60shutdown.60.20request.2C.20expected.20an.20.60exit.60.20notification/near/441914289.
- Some language clients attempt to reply to the file watcher
registration request before completing the LSP initialization dance. To
fix this, we now only send this request after the initialization dance
has completed. Fixes #3904.

---------

Co-authored-by: Sebastian Ullrich <[email protected]>
(cherry picked from commit 3119fd0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants