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

Use invalid request handler rather than raising key error for requests after shutdown #432

Merged

Conversation

smacke
Copy link
Contributor

@smacke smacke commented Sep 6, 2023

This is to fix #314 (for dealing with requests that come in after shutdown, e.g. from neovim). It requires python-lsp/python-lsp-jsonrpc#20 to work properly, which probably means it shouldn't merge without first bumping the min required version of python-lsp-jsonrpc.

Fixes #314.

@ccordoba12 ccordoba12 added this to the v1.8.0 milestone Sep 7, 2023
@ccordoba12 ccordoba12 changed the title use invalid request handler rather than raising key error for requests after shutdown Use invalid request handler rather than raising key error for requests after shutdown Sep 7, 2023
@ccordoba12 ccordoba12 added the bug Something isn't working label Sep 7, 2023
@ccordoba12
Copy link
Member

It requires python-lsp/python-lsp-jsonrpc#20 to work properly, which probably means it shouldn't merge without first bumping the min required version of python-lsp-jsonrpc.

I just released 1.1.0 of python-lsp-jsonrpc, which includes that PR. So, please bump our requirement to that package here to be >=1.1.0,<2.0.0.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Tiny suggestion for you @smacke, the rest looks good to me.

pylsp/python_lsp.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

One last thing: can we use the improvements done here to fix #114 too?

@smacke
Copy link
Contributor Author

smacke commented Sep 8, 2023

One last thing: can we use the improvements done here to fix #114 too?

Hmm, seems like it may be a separate issue around clients failing to throttle or debounce signature requests. I'm guessing either pylsp gets overwhelmed and starts dropping requests, which is why it can't find the future for cancellation, or it manages to successfully respond before the cancellation request comes in, but all the requests still degrade performance significantly. Flooding the logs seems like a secondary issue that could be fixed just by adjusting the log level of the warning message down to info or debug.

I suspect the way to fix this would be to implement some kind of server-side throttling to handle misbehaving clients. I use neovim; I'll check later if I can repro there.

@ccordoba12
Copy link
Member

Hmm, seems like it may be a separate issue around clients failing to throttle or debounce signature requests

No problem. I thought we could capture the cancel notifications for unknown messages, like you did with the requests after exit, and pass on them to avoid that error.

Flooding the logs seems like a secondary issue that could be fixed just by adjusting the log level of the warning message down to info or debug.

Ok, that's a good idea.

I suspect the way to fix this would be to implement some kind of server-side throttling to handle misbehaving clients. I use neovim; I'll check later if I can repro there.

Thanks @smacke!

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @smacke!

@ccordoba12 ccordoba12 merged commit 0d86844 into python-lsp:develop Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to handle requests after exit
2 participants