Release the state of a cancelled request instead of awaiting its reply - #77
Release the state of a cancelled request instead of awaiting its reply#77rintaro wants to merge 2 commits into
Conversation
…inuation `operation` received the `CheckedContinuation` directly, so the only entity able to resume the awaiting task was `operation` itself. Wrap the continuation in a `CancellableContinuation` that the helper owns and hand that to `operation` instead, so a later change can resume the awaiting task from the cancellation handler. Resuming more than once traps as before, now with a message naming `CancellableContinuation` rather than the underlying checked continuation. This does not change behavior: cancellation still only invokes `cancel` and the awaiting task is resumed exclusively by `operation`.
Cancelling a task that awaits `Connection.send` sent `$/cancelRequest` and then kept waiting for the peer's reply. A task suspended on a continuation is kept alive by that continuation, so a peer that stops replying retained every cancelled request's task, including the request payload it captured, until the connection was closed. A client that keeps issuing requests to an unresponsive peer therefore accumulated them without bound, and neither cancellation nor `withTimeout` could reclaim them. Resume the awaiting task with a `CancellationError` as soon as the cancellation is observed, and discard the reply if the peer sends one after all. Resuming twice for any other reason still traps. Add `Connection.abandonRequest(id:)` so the receiving end of the reply can release its bookkeeping as well, which `JSONRPCConnection` implements by removing the request from `outstandingRequests`. Its ID is remembered so that the reply to an abandoned request is not mistaken for a reply to a request that was never sent. The requirement has a default implementation that does nothing, so conformers holding no per-request state are unaffected.
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
I’m not a fan of abandonRequest if it no longer guarantees that the reply closure to send gets called, which changes local reasoning to global reasoning.
Is cleanup of the state in Connection really critical (this case really shouldn’t happen anyway) or is it just about getting the calling task unstuck? In that case, I think it would be simpler to have some kind of locally reasonable logic inside withCancellableCheckedThrowingContinuation that immediately resumes the calling task with a CancellationError while keeping the continuation in flight, maybe similar to what withTimeoutResult does.
| #endif | ||
|
|
||
| /// The maximum number of request IDs that `JSONRPCConnection` remembers as abandoned. | ||
| private let maxAbandonedRequestIDs = 100 |
There was a problem hiding this comment.
Should we make this match the number of requests we keep in recentlyFinishedRequests? Maybe have one configuration variable for both?
| /// `operation` to deliver a result. `operation` may never deliver one, eg. while | ||
| /// it waits for a reply from an unresponsive peer. A result that arrives after | ||
| /// the cancellation is discarded. | ||
| @_spi(SourceKitLSP) public func withCancellableCheckedThrowingContinuation<Handle: Sendable, Success: Sendable>( |
There was a problem hiding this comment.
Why does Success now need to be Sendable when it didn’t before?
| /// The awaiting task has been resumed with a `CancellationError`. | ||
| /// | ||
| /// The operation may still deliver a result that raced with the cancellation. That result is | ||
| /// discarded because the awaiting task is already resumed. | ||
| case cancelled | ||
|
|
||
| /// The awaiting task has been resumed. | ||
| case resumed |
There was a problem hiding this comment.
Is there any real distinction between these two states or could we just call both of them .done or something like that?
| @_spi(SourceKitLSP) public struct CancellableContinuation<Success: Sendable>: Sendable { | ||
| private enum State: Sendable { | ||
| /// The `CheckedContinuation` that suspends the awaiting task does not exist yet. | ||
| case notStarted |
There was a problem hiding this comment.
I feel like calling this type CancellableContinuation should make it mirror a little more how Continuation works and thus not have a notStarted state. In my opinion, a cleaner modeling would be to have a global withCancellableCheckedThrowingContinuation function that yields a CancellableContinuation to body.
| /// | ||
| /// The default implementation does nothing, which is correct for conformers that don't hold any | ||
| /// state per in-flight request. | ||
| func abandonRequest(id: RequestID) |
There was a problem hiding this comment.
I think we should clarify here whether the Connection is still expected to call the reply closure for the abandoned method. I personally really dislike if reply isn’t guaranteed to be called anymore because it makes it a lot harder to reason about whether eg. a continuation passed to it will get resumed since there’s non-local state.
2 participants
Cancelling a task that awaits
Connection.sendsent$/cancelRequestand then went back to waiting for a reply. A task suspended on a continuation is kept alive by that continuation, so the cancelled task never unwound — it and the request payload it captured stayed alive until the peer replied or the connection closed. A client talking to an unresponsive peer accumulates one such task per cancelled request, without bound, andwithTimeoutdoesn't help: it returns to its caller when the timer fires, but all it does to the request is cancel it.Resume the awaiting task with a
CancellationErroras soon as the cancellation is observed instead of waiting for a reply that may never arrive.Cancellation and the peer's reply are concurrent, so a reply can arrive after the awaiting task was already resumed.
withCancellableCheckedThrowingContinuationnow handsoperationaCancellableContinuationthat it owns, which discards exactly that late result. Resuming twice for any other reason still traps, so a transport delivering two replies for one request remains a hard error rather than a silent no-op.Since the reply handler stays registered once the awaiting task is resumed early,
Connectiongains anabandonRequest(id:)requirement that releases it.JSONRPCConnectionremoves the request fromoutstandingRequestsand remembers its ID — capped, since an unresponsive peer may never reply — so a late reply is discarded quietly instead of logged as a reply for a request that was never sent. The default implementation does nothing, so conformers holding no per-request state are unaffected.A peer is required by the LSP specification to reply to every request, including one cancelled with
$/cancelRequest. This only stops the client from depending on a peer that no longer does.rdar://184364916