Skip to content

Fix cancellation token reported when using retries #2345

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Fix cancellation token reported when using retries #2345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b84d772
Fix cancellation token reported when using retries
JamesNK Jan 2, 2024
8a6b8b8
Comment
JamesNK Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/Grpc.Net.Client/Internal/Retry/RetryCallBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ protected void HandleUnexpectedError(Exception ex)
CommitReason commitReason;

// Cancellation token triggered by dispose could throw here.
if (ex is OperationCanceledException && CancellationTokenSource.IsCancellationRequested)
if (ex is OperationCanceledException operationCanceledException && CancellationTokenSource.IsCancellationRequested)
{
// Cancellation could have been caused by an exceeded deadline.
if (IsDeadlineExceeded())
Expand All @@ -542,7 +542,21 @@ protected void HandleUnexpectedError(Exception ex)
else
{
commitReason = CommitReason.Canceled;
resolvedCall = CreateStatusCall(Disposed ? GrpcProtocolConstants.CreateDisposeCanceledStatus(ex) : GrpcProtocolConstants.CreateClientCanceledStatus(ex));
Status status;
if (Disposed)
{
status = GrpcProtocolConstants.CreateDisposeCanceledStatus(exception: null);
}
else
{
// Replace the OCE from CancellationTokenSource with an OCE that has the passed in cancellation token if it is canceled.
if (Options.CancellationToken.IsCancellationRequested && Options.CancellationToken != operationCanceledException.CancellationToken)
Copy link
Contributor

@mgravell mgravell Jan 2, 2024

Choose a reason for hiding this comment

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

I just want to check the intent here... "if something cancelled (ex), and Options is also cancelled, but isn't the thing being reported by ex: replace ex with the cancellation of Options" - is that right? are we happy to simply lose the original ex (which is a different calcellation)? should it be an inner-exception?

Copy link
Member Author

@JamesNK JamesNK Jan 2, 2024

Choose a reason for hiding this comment

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

This is replacing the OCE from CancellationTokenSource with an OCE using Options.CancellationToken if it is canceled. I added a comment.

The original exception is from an internal CT so it isn't valuable to keep. An OCE nested inside an OCE would be confusing.

{
ex = new OperationCanceledException(Options.CancellationToken);
}
status = GrpcProtocolConstants.CreateClientCanceledStatus(ex);
}
resolvedCall = CreateStatusCall(status);
}
}
else
Expand Down
13 changes: 11 additions & 2 deletions src/Grpc.Net.Client/Internal/Retry/StatusGrpcCall.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,21 @@ public void Dispose()

public Task<TResponse> GetResponseAsync()
{
return Task.FromException<TResponse>(new RpcException(_status));
return CreateErrorTask<TResponse>();
}

public Task<Metadata> GetResponseHeadersAsync()
{
return Task.FromException<Metadata>(new RpcException(_status));
return CreateErrorTask<Metadata>();
}

private Task<T> CreateErrorTask<T>()
{
if (_channel.ThrowOperationCanceledOnCancellation && _status.DebugException is OperationCanceledException ex)
{
return Task.FromException<T>(ex);
}
return Task.FromException<T>(new RpcException(_status));
}

public Status GetStatus()
Expand Down
33 changes: 33 additions & 0 deletions test/FunctionalTests/Client/CancellationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,39 @@ async Task<DataMessage> UnaryMethod(DataMessage request, ServerCallContext conte
Assert.AreEqual(StatusCode.Cancelled, call.GetStatus().StatusCode);
}

[Test]
public async Task Unary_Retry_CancellationImmediately_TokenMatchesSource()
{
var tcs = new TaskCompletionSource<object?>(TaskCreationOptions.RunContinuationsAsynchronously);
async Task<DataMessage> UnaryMethod(DataMessage request, ServerCallContext context)
{
await tcs.Task;
return new DataMessage();
}

SetExpectedErrorsFilter(writeContext =>
{
return true;
});

// Arrange
var method = Fixture.DynamicGrpc.AddUnaryMethod<DataMessage, DataMessage>(UnaryMethod);
var serviceConfig = ServiceConfigHelpers.CreateRetryServiceConfig();
var channel = CreateChannel(throwOperationCanceledOnCancellation: true, serviceConfig: serviceConfig);
var client = TestClientFactory.Create(channel, method);

// Act
var cts = new CancellationTokenSource();
cts.Cancel();

var call = client.UnaryCall(new DataMessage(), new CallOptions(cancellationToken: cts.Token));

// Assert
var ex = await ExceptionAssert.ThrowsAsync<OperationCanceledException>(() => call.ResponseAsync).DefaultTimeout();
Assert.AreEqual(cts.Token, ex.CancellationToken);
Assert.AreEqual(StatusCode.Cancelled, call.GetStatus().StatusCode);
}

[Test]
public async Task ServerStreaming_CancellationDuringCall_TokenMatchesSource()
{
Expand Down