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

Fix hanging clients on RequestDispatch error #423

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

tikue
Copy link
Collaborator

@tikue tikue commented Feb 2, 2024

Previously, when hitting an error, RequestDispatch would stop immediately without doing any cleanup. The pending requests channel would be close on drop, but any requests that were already in the channel would hang indefinitely.

With this PR, RequestDispatch will do a better cleanup job: it will close and drain the the pending requests, completing each request with the same error that caused the RequestDispatch to halt.

Fixes #415

@tikue tikue added the bug label Feb 2, 2024
@tikue tikue self-assigned this Feb 2, 2024
@tikue tikue force-pushed the request_dispatch_race branch 2 times, most recently from 921af67 to 5679bbe Compare February 3, 2024 00:30
It is possible for the sending of a request to race with the dropping of
RequestDispatch: a client could send a request, then the dispatch could
be dropped before processing the request. If that happens, the client
will never receive a shutdown notice.

To fix this, the request dispatch must first close the pending requests
channel, then drain all remaining requests from the channel, and for each
request, complete it with an error.

A few (hopefully minor) breaking changes were made:

- All ChannelError source errors are now wrapped in Arcs, so that the
  errors can be cloned and sent to all pending requests.
- RpcError::Receive was renamed to RpcError::Transport to accommodate
  the range of errors that can now be received by the client. Its source
  error is now a ChannelError.
- RpcError::Send's source error is now the transport error rather than
  ChannelError::Write(Transport::Error), because ChannelError::Write
  wasn't adding any additional information.
@tikue tikue merged commit d579f1c into google:master Feb 3, 2024
5 checks passed
@tikue tikue deleted the request_dispatch_race branch February 3, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On client's read errors quick subsequent requests may wait for response indefinitely
1 participant