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

LSP: Send replies for malformed and unhandled RPC requests #6058

Merged
merged 1 commit into from
Mar 8, 2023
Merged

LSP: Send replies for malformed and unhandled RPC requests #6058

merged 1 commit into from
Mar 8, 2023

Conversation

the-mikedavis
Copy link
Member

Previously we did not respond to malformed or unhandled LSP requests. The JSONRPC spec says that all non-notification requests must have responses:

When a rpc call is made, the Server MUST reply with a Response,
except for in the case of Notifications

(Note that Helix is the "Server" in this case. Also from the spec: "The Server is defined as the origin of Response objects and the handler of Request objects.")

So this change sends error replies for requests which can't be parsed or handled. Request IDs are also now added to the log messages for unhandled requests.

Fixes #5958

@the-mikedavis
LSP: Send replies for malformed and unhandled RPC requests
fdc7774 
Previously we did not respond to malformed or unhandled LSP requests.
The JSONRPC spec says that all non-notification requests must have
responses:

> When a rpc call is made, the Server MUST reply with a Response,
> except for in the case of Notifications

(Note that Helix is the "Server" in this case. Also from the spec:
"The Server is defined as the origin of Response objects and the
handler of Request objects.")

So this change sends error replies for requests which can't be parsed
or handled. Request IDs are also now added to the log messages for
unhandled requests.
@the-mikedavis the-mikedavis added C-bug Category: This is a bug A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 19, 2023
@the-mikedavis the-mikedavis marked this pull request as ready for review February 19, 2023 23:15
@ghost
Copy link

ghost commented Feb 20, 2023

My issue (#6043) got closed as a duplicate to #5958 and couldn't find any solution, then saw that you this PR is the supposed fix, so I hope it gets approved and thank you for taking the time for fixing this.

@the-mikedavis the-mikedavis force-pushed the md-reply-for-bad-lsp-requests branch from 83e554b to fdc7774 Compare February 20, 2023 19:36
@the-mikedavis the-mikedavis mentioned this pull request Feb 20, 2023
Merged
pascalkuthe
pascalkuthe approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even tough this turned out to be solvable upstream after all it's still good to adhere to the spec here and avoid accidently locking up servers 👍 LGTM

@the-mikedavis
Copy link
Member Author

This change is still needed separately from the change upstream: pyright will lock up if we don't respond to the workspace/configuration request and this change will respond with an error (second Err branch), preventing the lock-up.

The first Err branch for sending an error for unhandled methods causes a separate problem with pyright where it will exit instead of locking up. That part will be fixed with the upstream PR. So if you're looking to use Helix with pyright 1.1.293 or above, you will need to wait on this PR and the one upstream in pyright. Pyright 1.1.292 should work with the current Helix master.

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 21, 2023 •
edited
Loading

This change is still needed separately from the change upstream: pyright will lock up if we don't respond to the workspace/configuration request and this change will respond with an error (second Err branch), preventing the lock-up.

The first Err branch for sending an error for unhandled methods causes a separate problem with pyright where it will exit instead of locking up. That part will be fixed with the upstream PR. So if you're looking to use Helix with pyright 1.1.293 or above, you will need to wait on this PR and the one upstream in pyright. Pyright 1.1.292 should work with the current Helix master.

Ah right I rember now pyright was sending two separate invalid requests. One for dynamic registration and an invalid condiguration request. Sorry I had mixed those two up just now!

@the-mikedavis the-mikedavis mentioned this pull request Mar 4, 2023
Open
@archseer archseer merged commit 1705931 into helix-editor:master Mar 8, 2023
@the-mikedavis the-mikedavis deleted the md-reply-for-bad-lsp-requests branch March 9, 2023 17:09
@the-mikedavis the-mikedavis mentioned this pull request Mar 9, 2023
Closed
@sun617 sun617 mentioned this pull request Mar 10, 2023
Closed
sagnibak pushed a commit to sagnibak/helix that referenced this pull request Mar 21, 2023
@the-mikedavis
LSP: Send replies for malformed and unhandled RPC requests (helix-edi…
4ffee15 
…tor#6058)

Previously we did not respond to malformed or unhandled LSP requests.
The JSONRPC spec says that all non-notification requests must have
responses:

> When a rpc call is made, the Server MUST reply with a Response,
> except for in the case of Notifications

(Note that Helix is the "Server" in this case. Also from the spec:
"The Server is defined as the origin of Response objects and the
handler of Request objects.")

So this change sends error replies for requests which can't be parsed
or handled. Request IDs are also now added to the log messages for
unhandled requests.
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
@the-mikedavis @wes-adams
LSP: Send replies for malformed and unhandled RPC requests (helix-edi…
89155ad 
…tor#6058)

Previously we did not respond to malformed or unhandled LSP requests.
The JSONRPC spec says that all non-notification requests must have
responses:

> When a rpc call is made, the Server MUST reply with a Response,
> except for in the case of Notifications

(Note that Helix is the "Server" in this case. Also from the spec:
"The Server is defined as the origin of Response objects and the
handler of Request objects.")

So this change sends error replies for requests which can't be parsed
or handled. Request IDs are also now added to the log messages for
unhandled requests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalkuthe pascalkuthe pascalkuthe approved these changes

Assignees
No one assigned
Labels
A-language-server Area: Language server client C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issues with pyright-langserver
3 participants
@the-mikedavis @pascalkuthe @archseer