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

Stop running cancel request in the main thread #3028

Merged

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jan 9, 2025

Motivation

This PR is another temporary solution for #3019. It reverts the other part of #2939.

Implementation

Essentially, we go back to pushing $/cancelRequest notifications to our incoming queue, rather than executing it straight away in the main thread.

This behaviour is incorrect because it essentially means no request will ever be cancelled. The order of operations is:

  1. Get a request. Push it to the queue
  2. Receive a cancellation notification for that request. Push it to the queue
  3. Pop the request (messages are popped in order FIFO)
  4. Fully process the request even though it was cancelled. Return a response
  5. Now, we see the cancellation, which is of course too late. We already responded

There's no way this sequence of operations is correct, but for whatever reason reverting back to this fixes the LSP in NeoVim.

There are clearly things I'm not understanding about request cancellation and what editor/server are expected to do, so I will need to dig deeper to understand the underlying issue and how to properly implement it.

Note

The consequence of reverting back to the old behaviour is performance. Executing a request for which cancellation happened makes the server waste time generating a response that the editor is going to ignore anyway.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock added server This pull request should be included in the server gem's release notes bugfix This PR will fix an existing bug labels Jan 9, 2025 — with Graphite App
@vinistock vinistock requested review from andyw8 and st0012 January 9, 2025 14:04
@vinistock vinistock marked this pull request as ready for review January 9, 2025 14:04
@vinistock vinistock requested a review from a team as a code owner January 9, 2025 14:04
@vinistock vinistock merged commit 2376123 into main Jan 9, 2025
42 of 43 checks passed
@vinistock vinistock deleted the 01-09-stop_running_cancel_request_in_the_main_thread branch January 9, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants