Restore request cancellation with correct implementation #3791
Conversation
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
Maybe someone can help test this in Neovim? @adam12, @natematykiewicz, or @dzirtusss? I think it's enough to add the following to the Gemfile: gem "ruby-lsp", github: "janko/ruby-lsp", branch: "restore-request-cancellation" |
vinistock
added
bugfix
vinistock
left a comment
vinistock
left a comment
There was a problem hiding this comment.
Thanks for the PR and good eye on spotting the problem! You're 100% correct that the issue was next applying to any block and thus not skipping the iteration as intended 🤦.
It seems like this should work as expected, but since it caused lots of issues for the NeoVim crowd, let's just make sure at least one user confirms it works well for them
|
I'll try to test this week. |
|
I've observed no issues running this branch over the last few days. 👍 |
Ruby LSP doesn't currently cancel requests, because it processes `$/cancelRequest` notifications in the same queue as other requests, so previous requests will get processed before they get the chance to get cancelled. This was initially addressed in Shopify#2939, with cancelled requests returning an error response. This is matching the LSP spec, which requires either an incomplete or an error response. However, this started causing issues with Neovim as reported in Shopify#3019, so that implementation was ultimately reverted. The problem wasn't actually the error responses, it was that Ruby LSP was also returning a regular response, *on top of* the error response. So, a cancellation notification was resulting in *two* responses being returned for the same request, which rightfully caused errors in Neovim, as it didn't know how to handle the 2nd response. This happened because the `next` keyword breaks out of the nearest loop *or* closure, which was the `Mutex#synchronize` block, it didn't actually skip to the next iteration. This caused `process_message` to execute even if the cancel response was already sent, which ended up sending another (regular) response. To avoid introducing boolean local variables, I extracted the loop body into a separate method, so that I can just use `return` to break out. I verified that this works as expected in Zed, so I also expect it to work well in Neovim.
janko
force-pushed
the
restore-request-cancellation
branch
from
76e1ace to
4c90517
Compare
|
Good to ship once the CLA bot is happy 👍 |
|
I have signed the CLA! |
|
Thank you for the contribution! |
3 participants
Motivation
Closes #3063
The project symbol search is currently slow in Zed, because it sends a
workspace/symbolrequest for every new letter typed in, and attempts to cancel previous requests aren't successful, as Ruby LSP currently doesn't actually cancel requests (it enqueues$/cancelRequestmessages instead of processing them synchronously).Implementation
Fix for request cancellation was already attempted in #3063, but it caused issues with Neovim (#3019), so it was ultimately reverted.
The problem wasn't actually in the new error responses, it was that the attempted fix contained a bug, where it was returning a regular response in addition to the error response. So, request cancellation would result in two responses being returned for the same request, which rightfully tripped Neovim up.
The bug was in attempting to skip the regular response using
next. In addition to advancing iteration of nearest loop,nextalso breaks out of the nearest closure, whichever comes first. In this case, the surroundingMutex#synchronizeblock was the nearest, so it broke out of that, and proceeded to execute#process_message. This resulted in regular response being sent back even if the cancelled response was previously sent.To avoid introducing a boolean local variable, I extracted the handling logic into a method, so that I can break out using
return.Automated Tests
I updated the tests to match the original implementation. However, I didn't know how to reliably assert that the 2nd response isn't getting sent, because I don't know how to wait until the worker becomes idle. I feel like I don't have enough control over the concurrency.
Manual Tests
I tested this in Zed, and everything looked correct in the RCP message log.