Skip to content

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented May 23, 2025

Summary

This PR implements the following releated features:

  • Support re-trying requests after they failed due to a salsa Cancellation (some content in the db was modified)
  • Support for the LSP cancel request: Don't retry requests that were cancelled. Don't run background requests that were cancelled before they run on the background thread

This required significant refactoring of the server's request/notification/respond model because tracking the pending requests requires access to Session, which we don't have in background tasks.

This PR:

  • 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.

Closes astral-sh/ty#75

Test Plan

  • I added a timeout in the background task scheduling and verified that requests weren't processed that were cancelled by the client
  • I added a timeout to completions (that now has retry enabled) and verified that the request is re-executed after a salsa cancellation
  • I tested that a panic on the main loop still shows a message box
  • I tested that a panic in a background task shows a message box
  • I tested that the server correctly restarts when issuing the restart server command

@MichaReiser MichaReiser added server Related to the LSP server ty Multi-file analysis & type inference labels May 23, 2025
Copy link
Contributor

github-actions bot commented May 23, 2025

mypy_primer results

No ecosystem changes detected ✅

@MichaReiser MichaReiser force-pushed the micha/server-cancellation-handling branch from 1aacb8c to 038c831 Compare May 23, 2025 09:28
@MichaReiser MichaReiser marked this pull request as ready for review May 23, 2025 09:35
@MichaReiser MichaReiser force-pushed the micha/server-cancellation-handling branch from 038c831 to 5bc3aee Compare May 23, 2025 10:42
@MichaReiser MichaReiser force-pushed the micha/remove-unnecessary-lifetimes branch from 397f1bc to 8bd8ab7 Compare May 26, 2025 12:41
Base automatically changed from micha/remove-unnecessary-lifetimes to main May 26, 2025 12:44
@MichaReiser MichaReiser force-pushed the micha/server-cancellation-handling branch from f5ece05 to 775a29a Compare May 26, 2025 12:57
@AlexWaygood AlexWaygood removed their request for review May 26, 2025 17:52
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you very much. This all looks very reasonable, but I can not review this as deeply as it might be necessary. I'm not (yet) familiar with the LSP server code.

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.

This is great! It simplifies a bunch of stuff.

A few questions / notes going through the diff:

  • The spec mentions that cancellation would make it possible to return partial results. I don't think that's supported right now and I don't even think it's required or necessary now or in the near future until we've completed most of the capabilities. I don't know if partial results is going to be useful and if so how much.
  • Does the current implementation only "records" that this request has been cancelled and not actually cancel the running thread?

@MichaReiser
Copy link
Member Author

The spec mentions that cancellation would make it possible to return partial results. I don't think that's supported right now and I don't even think it's required or necessary now or in the near future until we've completed most of the capabilities. I don't know if partial results is going to be useful and if so how much.

That's correct. The current implementation always omits the entire result. Partial results would complicate things a bit because

The server can't send the cancelled response immediately if the request is already in progress. We also need to pass the CancellationToken to the request handler so that it can decide itself what it wants to do if it gets cancelled. I'm not sure how well that would work overall because cancellations are mainly issued after changes and changes result in a write to the db -> any db access unwinds with a Salsa::Cancelled payload. But definetely something worth exploring in the future

Does the current implementation only "records" that this request has been cancelled and not actually cancel the running thread?

This is correct. However, I don't think it should matter in most cases because the main reason for a client to send a cancellation is when some content changed, which results in mutating the database. Salsa implements mutation by unwinding all other db clones when they are accessed the next time. What this means in practice is that salsa cancels in-flight requests. However, we could pass the CancellationToken to the request handler if we have some computationally intensive logic that doesn't perform any database access.

@MichaReiser MichaReiser force-pushed the micha/server-cancellation-handling branch from 775a29a to de5ab7f Compare May 28, 2025 08:17
@MichaReiser MichaReiser force-pushed the micha/server-cancellation-handling branch from ec656fa to cc0f233 Compare May 28, 2025 08:47
@MichaReiser MichaReiser merged commit 66ba1d8 into main May 28, 2025
35 checks passed
@MichaReiser MichaReiser deleted the micha/server-cancellation-handling branch May 28, 2025 08:59
dcreager added a commit that referenced this pull request May 28, 2025
* main:
  [ty] Support ephemeral uv virtual environments (#18335)
  Add a `ViolationMetadata::rule` method (#18234)
  Return `DiagnosticGuard` from `Checker::report_diagnostic` (#18232)
  [flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (#18337)
  [ty] Support cancellation and retry in the server (#18273)
  [ty] Synthetic function-like callables (#18242)
  [ty] Support publishing diagnostics in the server (#18309)
  Add Autofix for ISC003 (#18256)
  [`pyupgrade`]: new rule UP050 (`useless-class-metaclass-type`) (#18334)
  [pycodestyle] Make `E712` suggestion not assume a context (#18328)
carljm added a commit to MatthewMckee4/ruff that referenced this pull request May 28, 2025
* main: (246 commits)
  [ty] Simplify signature types, use them in `CallableType` (astral-sh#18344)
  [ty] Support ephemeral uv virtual environments (astral-sh#18335)
  Add a `ViolationMetadata::rule` method (astral-sh#18234)
  Return `DiagnosticGuard` from `Checker::report_diagnostic` (astral-sh#18232)
  [flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (astral-sh#18337)
  [ty] Support cancellation and retry in the server (astral-sh#18273)
  [ty] Synthetic function-like callables (astral-sh#18242)
  [ty] Support publishing diagnostics in the server (astral-sh#18309)
  Add Autofix for ISC003 (astral-sh#18256)
  [`pyupgrade`]: new rule UP050 (`useless-class-metaclass-type`) (astral-sh#18334)
  [pycodestyle] Make `E712` suggestion not assume a context (astral-sh#18328)
  put similar dunder-call tests next to each other (astral-sh#18343)
  [ty] Derive `PartialOrd, Ord` for `KnownInstanceType` (astral-sh#18340)
  [ty] Simplify `Type::try_bool()` (astral-sh#18342)
  [ty] Simplify `Type::normalized` slightly (astral-sh#18339)
  [ty] Move arviz off the list of selected primer projects (astral-sh#18336)
  [ty] Add --config-file CLI arg (astral-sh#18083)
  [ty] Tell the user why we inferred a certain Python version when reporting version-specific syntax errors (astral-sh#18295)
  [ty] Implement implicit inheritance from `Generic[]` for PEP-695 generic classes (astral-sh#18283)
  [ty] Add hint if async context manager is used in non-async with statement (astral-sh#18299)
  ...
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 ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle cancellation requests in LSP
4 participants