Skip to content

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 11, 2025

Summary

This PR adds support for the LSP's cancellation requests.

This is mainly a backport of the following two ty PRs:

Unlike ty, this PR doesn't add support for retries. Retries aren't necessary because
Ruff doesn't have to handle Salsa cancellations

  • It removes Requester, Responder and Notifier and unifies them under a single Client API. I found them more confusing than helpful, and I don't see any reason why we should restrict background tasks from sending requests.
  • It removes ClientSender: This is mostly a fallout of the above.
  • Split IOThreads out of Connection: This removes the entire complexity around weak references because scoping rules now ensure that Connection (and its senders) are dropped before we join the thread.
  • Remove the global MESSENGER and instead require callers to explicitly pass a Client. This helped find show_message call sites where MESSENGER wasn't initialized. It also removes global state, which is always good
  • Responses are sent to the main loop and not directly to the client. This is required to mark the request as complete (or omit the response if the request was cancelled), and it is only possible with a mut reference to Session that background tasks don't have. This approach is the same as r-a's.
  • Notifications and requests are still sent directly to the client to reduce load on the main loop. We could change that if we want but I decided against it for now.

Test Plan

Started VS code with the new ruff server.

  • Verified that diagnostics still show up.
  • Verified that no ruff process is running after closing VS code
  • I added a 1s sleep to the diagnostic handler. I verified that there are cancellation requests (and the requests are cancelled)

@MichaReiser MichaReiser added the server Related to the LSP server label Jun 11, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 11, 2025

mypy_primer results

No ecosystem changes detected ✅

@github-actions
Copy link
Contributor

github-actions bot commented Jun 11, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser marked this pull request as ready for review June 11, 2025 13:37
@AlexWaygood AlexWaygood removed their request for review June 11, 2025 13:43
@carljm carljm removed their request for review June 11, 2025 18:03
Copy link
Member

@dhruvmanila dhruvmanila 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! I haven't looked very closely at the code but it all looks almost same as the one in ty_server so I've provided a couple of recommendations to de-duplicate if possible. This doesn't need to happen in this PR and we can also open a "help-wanted" issue for this.

Copy link
Member

Choose a reason for hiding this comment

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

This looks exactly the same as the one in the ty_server, can we re-use it?

Copy link
Member

Choose a reason for hiding this comment

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

This also looks the same as the one in ty_server except for the retry method, maybe this could be re-used as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's almost the same with the exception that it doesn't have a retry function.

It's not a "difficult" problem but there is no good place for the files that are shared between ruff and the ty server. And Client depends on Session, which makes the reuse a bit of a challenge.

Base automatically changed from micha/server-options to main June 12, 2025 16:58
@MichaReiser MichaReiser reopened this Jun 12, 2025
@MichaReiser MichaReiser merged commit 0152229 into main Jun 12, 2025
69 of 71 checks passed
@MichaReiser MichaReiser deleted the lsp-refactr branch June 12, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Related to the LSP server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants