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

Add an RPC cancellation handler #2090

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Oct 8, 2024

Motivation:

As a service author it's useful to know if the RPC has been cancelled (because it's timed out, the remote peer closed it, the connection dropped etc).

For cases where the stream has already closed this can be surfaced by a read or write failing. However, for cases like server-streaming RPCs where there are no reads and writes can be infrequent it's useful to have a more explicit signal.

Modifications:

  • Add a ServerCancellationManager, this is internal per-stream storage for registering cancellation handlers and storing whether the RPC has been cancelled.
  • Add the RPCCancellationHandle nested within the ServerContext. This holds an instance of the manager and provides higher level APIs allowing users to check if the RPC has been cancellation and to wait until the RPC has been cancelled.
  • Add a top-level withRPCCancellationHandler which registers a callback with the manager.
  • Add a top-level withServerContextRPCCancellationHandle for creating and binding the task local manager. This is intended for use by transport implementations rather than users.
  • Update the in-process transport to cancel RPCs when shutting down gracefully.
  • Update the server executor to cancel RPCs when the timeout fires.

Result:

Users can watch for cancellation using withRPCCancellationHandler.

Motivation:

As a service author it's useful to know if the RPC has been cancelled
(because it's timed out, the remote peer closed it, the connection
dropped etc).

For cases where the stream has already closed this can be surfaced by a
read or write failing. However, for cases like server-streaming RPCs
where there are no reads and writes can be infrequent it's useful to
have a more explicit signal.

Modifications:

- Add a `ServerCancellationManager`, this is internal per-stream storage
  for registering cancellation handlers and storing whether the RPC has
  been cancelled.
- Add the `RPCCancellationHandle` nested within the `ServerContext`.
  This holds an instance of the manager and provides higher level APIs
  allowing users to check if the RPC has been cancellation and to wait
  until the RPC has been cancelled.
- Add a top-level `withRPCCancellationHandler` which registers a
  callback with the manager.
- Add a top-level `withServerContextRPCCancellationHandle` for creating
  and binding the task local manager. This is intended for use by
  transport implementations rather than users.
- Update the in-process transport to cancel RPCs when shutting down
  gracefully.
- Update the server executor to cancel RPCs when the timeout fires.

Result:

Users can watch for cancellation using `withRPCCancellationHandler`.
@glbrntt glbrntt force-pushed the v2/rpc-is-cancelled-lifecycle branch from 9dad9df to 6fa20bd Compare October 8, 2024 15:00
@glbrntt glbrntt added the ⚠️ semver/major Breaks existing public API. label Oct 8, 2024
@glbrntt glbrntt requested a review from gjcairo October 8, 2024 15:00
@glbrntt glbrntt mentioned this pull request Oct 8, 2024
Copy link
Collaborator

@gjcairo gjcairo 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, thanks George. Left only a few minor comments.

public typealias Inbound = RPCAsyncSequence<RPCRequestPart, any Error>
public typealias Outbound = RPCWriter<RPCResponsePart>.Closable

private let newStreams: AsyncStream<RPCStream<Inbound, Outbound>>
private let newStreamsContinuation: AsyncStream<RPCStream<Inbound, Outbound>>.Continuation

private struct State: Sendable {
private var id: UInt64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe call this nextHandleID or something like that?

Comment on lines 96 to 97
/// the duration of the RPC. This function is intended for use when implementing
/// a ``ServerTransport``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe put the second sentence inside an Important block? I think it could be misused.

@glbrntt glbrntt requested a review from gjcairo October 9, 2024 11:53
@glbrntt glbrntt enabled auto-merge (squash) October 9, 2024 12:02
@glbrntt glbrntt merged commit 945cbf6 into grpc:main Oct 9, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants