Co-operatively cancel all RPC Calls #576
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nathanwoctopusdeploy
changed the base branch from
main
to
sast/test-exception-contracts
| cancellationTokenSource.Cancel(); | ||
| await task; | ||
| })).And.Message.Should().Contain($"An error occurred when sending a request to 'https://20.5.79.31:10933/', after the request began: The operation was canceled."); | ||
| })).And.Message.Should().Contain($"An error occurred when sending a request to 'https://20.5.79.31:10933/', after the request began: The Request was cancelled while Connecting."); |
There was a problem hiding this comment.
Exception contract change - The error message changes
| (await AssertException.Throws<HalibutClientException>(async () => await client.SayHelloAsync("Hello", new(cancellationTokenSource.Token, CancellationToken.None)))) | ||
| .And.Message.Should().Contain("An error occurred when sending a request to 'https://20.5.79.31:10933/', after the request began: The operation was canceled."); | ||
| (await AssertException.Throws<HalibutClientException>(async () => await client.SayHelloAsync("Hello", new(cancellationTokenSource.Token)))) | ||
| .And.Message.Should().Contain("An error occurred when sending a request to 'https://20.5.79.31:10933/', after the request began: The Request was cancelled while Connecting."); |
There was a problem hiding this comment.
Exception contract change - The error message changes
| exception.Message.Should().Be($"An error occurred when sending a request to '{clientAndService.ServiceUri}', after the request began: The ReadAsync operation was cancelled."); | ||
| #endif | ||
| var exception = (await AssertException.Throws<HalibutClientException>(async () => await client.SayHelloAsync("Hello", new(cancellationTokenSource.Token)))).And; | ||
| exception.Message.Should().Be($"An error occurred when sending a request to '{clientAndService.ServiceUri}', after the request began: The Request was cancelled while Connecting."); |
There was a problem hiding this comment.
Exception contract change - The error message changes
| (await AssertException.Throws<HalibutClientException>(async () => await doSomeActionClient.ActionAsync(new(cancellationTokenSource.Token, cancellationTokenSource.Token)))) | ||
| .And.Message.Should().Contain($"An error occurred when sending a request to '{clientAndService.ServiceUri}', after the request began: The ReadAsync operation was cancelled."); | ||
| (await AssertException.Throws<HalibutClientException>(async () => await doSomeActionClient.ActionAsync(new(cancellationTokenSource.Token)))) | ||
| .And.Message.Should().Contain($"An error occurred when sending a request to '{clientAndService.ServiceUri}', after the request began: The Request was cancelled while Transferring."); |
There was a problem hiding this comment.
Exception contract change - The error message changes
| @@ -240,15 +240,8 @@ public async Task WhenTheListeningRequestIsCancelledWhileConnecting_AndTheConnec | |||
|
|
|||
| var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(5)); | |||
|
|
|||
| #if NETFRAMEWORK | |||
There was a problem hiding this comment.
Nice that we could get rid of this 😄
source/Halibut.Tests/ServiceModel/PendingRequestQueueFixture.cs
Outdated
Show resolved
Hide resolved
source/Halibut.Tests/WhenCancellingARequestForAPollingTentacle.cs
Outdated
Show resolved
Hide resolved
sburmanoctopus
approved these changes
Dec 6, 2023
LukeButters
reviewed
Dec 6, 2023
| namespace Halibut.Tests.Support.PendingRequestQueueFactories | ||
| { | ||
| /// <summary> | ||
| /// CancelWhenRequestDequeuedPendingRequestQueueFactory cancels the cancellation token source when a request is queued |
There was a problem hiding this comment.
when request is dequeued?
LukeButters
approved these changes
Dec 6, 2023
nathanwoctopusdeploy
force-pushed
the
sast/cancel-polling-requests-to-the-socket
branch
from
8c406ed to
0274644
Compare
Combine connecting and in progress cancellation tokens
nathanwoctopusdeploy
force-pushed
the
sast/cancel-polling-requests-to-the-socket
branch
from
0274644 to
0dd6357
Compare
3 tasks
nathanwoctopusdeploy
deleted the
sast/cancel-polling-requests-to-the-socket
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a combination of the reverted PRs #525 and #558 combined and rebased
Background
[sc-58429]
Before Async Halibut was introduced, Halibut only supported cancellation of an RPC call when the request was in the connecting state, queued for a Polling Request and the TCP Connection being established for a Listening Request. Once the RPC was in flight, dequeued for a Polling Request, and the connection established for a Listening Request, cancellation was not supported.
As the cancellation of an in-flight request was not supported, a caller would only have the option to walk away from an RPC call and leave threads and IO running.
When Async Halibut was introduced, cooperative cancellation was introduced down to the socket for RPC calls to Listening Service and to the queue for Polling Services. Cancellation could be performed if the RPC call was connecting and in-flight. So as not to force clients to change there existing behaviour, two cancellation tokens were introduced, one to cancel when connecting and one to cancel when in-flight.
Results
This PR extends the previous cancellation support so that an in-flight Polling Request can be cancelled to the Socket. Terminating any in-flight Reads or Writes on the TCP Connection.
The two cancellation tokens have been combined into one to simplify the cancellation logic. The ability to cancel just when connecting is no longer required in TentacleClient. Instead, the desire is to co-operatively cancel an RPC call regardless of it's current state.
Tentacle Client does need to know if the cancelled RPC call was connecting or in flight to make the most efficient decisions about what it has to do next, so cancellation will throw specific errors based on the state of the RPC call when it was cancelled.
net48 does not support full cancellation due to the usage of a deflate stream, which uses APM Begin and End methods and loses the cancellation token. As TentacleClient only supports net6.0, this is not an issue.
How to review this PR
Quality ✔️
Pre-requisites