Skip to content

Commit ab2c2f1

Browse files
committed
Update
1 parent 3f93a6b commit ab2c2f1

File tree

5 files changed

+91
-41
lines changed

5 files changed

+91
-41
lines changed

src/Grpc.Net.Client/Internal/GrpcCall.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,7 @@ private async Task RunCall(HttpRequestMessage request, TimeSpan? timeout)
639639
if (_responseTcs != null)
640640
{
641641
_responseTcs.TrySetException(resolvedException);
642-
642+
643643
// Always observe cancellation-like exceptions.
644644
if (IsCancellationOrDeadlineException(ex))
645645
{

src/Grpc.Net.Client/Internal/Retry/HedgingCall.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,16 @@ private async Task StartCall(Action<GrpcCall<TRequest, TResponse>> startCallFunc
202202
{
203203
if (CommitedCallTask.IsCompletedSuccessfully() && CommitedCallTask.Result == call)
204204
{
205+
// Ensure response task is created before waiting to the end.
206+
// Allows cancellation exceptions to be observed in cleanup.
207+
_ = GetResponseAsync();
208+
205209
// Wait until the commited call is finished and then clean up hedging call.
206210
// Force yield here to prevent continuation running with any locks.
207-
await CompatibilityHelpers.AwaitWithYieldAsync(call.CallTask).ConfigureAwait(false);
208-
Cleanup();
211+
var status = await CompatibilityHelpers.AwaitWithYieldAsync(call.CallTask).ConfigureAwait(false);
212+
213+
var observeExceptions = status.StatusCode is StatusCode.Cancelled or StatusCode.DeadlineExceeded;
214+
Cleanup(observeExceptions);
209215
}
210216
}
211217
}

src/Grpc.Net.Client/Internal/Retry/RetryCall.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,16 @@ private async Task StartRetry(Action<GrpcCall<TRequest, TResponse>> startCallFun
252252
{
253253
if (CommitedCallTask.Result is GrpcCall<TRequest, TResponse> call)
254254
{
255+
// Ensure response task is created before waiting to the end.
256+
// Allows cancellation exceptions to be observed in cleanup.
257+
_ = GetResponseAsync();
258+
255259
// Wait until the commited call is finished and then clean up retry call.
256260
// Force yield here to prevent continuation running with any locks.
257-
await CompatibilityHelpers.AwaitWithYieldAsync(call.CallTask).ConfigureAwait(false);
258-
Cleanup();
261+
var status = await CompatibilityHelpers.AwaitWithYieldAsync(call.CallTask).ConfigureAwait(false);
262+
263+
var observeExceptions = status.StatusCode is StatusCode.Cancelled or StatusCode.DeadlineExceeded;
264+
Cleanup(observeExceptions);
259265
}
260266
}
261267

src/Grpc.Net.Client/Internal/Retry/RetryCallBase.cs

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,11 @@ public Task<Metadata> GetResponseHeadersAsync()
130130
// ResponseHeadersAsync could be called inside a client interceptor when a call is wrapped.
131131
// Most people won't use the headers result. Observed exception to avoid unobserved exception event.
132132
_responseHeadersTask.ObserveException();
133+
134+
// If there was an error fetching response headers then it's likely the same error is reported
135+
// by response TCS. The user is unlikely to observe both errors.
136+
// Observed exception to avoid unobserved exception event.
137+
_responseTask?.ObserveException();
133138
}
134139

135140
return _responseHeadersTask;
@@ -387,7 +392,7 @@ protected void CommitCall(IGrpcCall<TRequest, TResponse> call, CommitReason comm
387392
// A commited call that has already cleaned up is likely a StatusGrpcCall.
388393
if (call.Disposed)
389394
{
390-
Cleanup();
395+
Cleanup(observeExceptions: false);
391396
}
392397
}
393398
}
@@ -449,19 +454,16 @@ protected virtual void Dispose(bool disposing)
449454

450455
if (disposing)
451456
{
452-
_responseTask?.ObserveException();
453-
_responseHeadersTask?.ObserveException();
454-
455457
if (CommitedCallTask.IsCompletedSuccessfully())
456458
{
457459
CommitedCallTask.Result.Dispose();
458460
}
459461

460-
Cleanup();
462+
Cleanup(observeExceptions: true);
461463
}
462464
}
463465

464-
protected void Cleanup()
466+
protected void Cleanup(bool observeExceptions)
465467
{
466468
Channel.FinishActiveCall(this);
467469

@@ -470,6 +472,12 @@ protected void Cleanup()
470472
CancellationTokenSource.Cancel();
471473

472474
ClearRetryBuffer();
475+
476+
if (observeExceptions)
477+
{
478+
_responseTask?.ObserveException();
479+
_responseHeadersTask?.ObserveException();
480+
}
473481
}
474482

475483
internal bool TryAddToRetryBuffer(ReadOnlyMemory<byte> message)

test/Grpc.Net.Client.Tests/Retry/RetryTests.cs

Lines changed: 60 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,14 +1072,35 @@ public enum ResponseHandleAction
10721072
Nothing
10731073
}
10741074

1075-
[Test]
1076-
//[TestCase(0, false, ResponseHandleAction.ResponseAsync)]
1077-
//[TestCase(0, true, ResponseHandleAction.ResponseAsync)]
1078-
[TestCase(0, false, ResponseHandleAction.ResponseHeadersAsync)]
1079-
//[TestCase(0, false, ResponseHandleAction.Dispose)]
1080-
//[TestCase(1, false, ResponseHandleAction.Nothing)]
1081-
public async Task AsyncUnaryCall_CallFailed_NoUnobservedExceptions(int expectedUnobservedExceptions, bool addClientInterceptor, ResponseHandleAction action)
1075+
public static object[] NoUnobservedExceptionsCases
1076+
{
1077+
get
1078+
{
1079+
var cases = new List<object[]>();
1080+
AddCases(0, addClientInterceptor: false, throwCancellationError: false, ResponseHandleAction.ResponseAsync);
1081+
AddCases(0, addClientInterceptor: true, throwCancellationError: false, ResponseHandleAction.ResponseAsync);
1082+
AddCases(0, addClientInterceptor: false, throwCancellationError: false, ResponseHandleAction.ResponseHeadersAsync);
1083+
AddCases(0, addClientInterceptor: false, throwCancellationError: false, ResponseHandleAction.Dispose);
1084+
AddCases(1, addClientInterceptor: false, throwCancellationError: false, ResponseHandleAction.Nothing);
1085+
AddCases(0, addClientInterceptor: false, throwCancellationError: true, ResponseHandleAction.Nothing);
1086+
return cases.ToArray();
1087+
1088+
void AddCases(int expectedUnobservedExceptions, bool addClientInterceptor, bool throwCancellationError, ResponseHandleAction action)
1089+
{
1090+
cases.Add(new object[] { expectedUnobservedExceptions, true, addClientInterceptor, throwCancellationError, action });
1091+
cases.Add(new object[] { expectedUnobservedExceptions, false, addClientInterceptor, throwCancellationError, action });
1092+
}
1093+
}
1094+
}
1095+
1096+
[TestCaseSource(nameof(NoUnobservedExceptionsCases))]
1097+
public async Task AsyncUnaryCall_CallFailed_NoUnobservedExceptions(int expectedUnobservedExceptions, bool isAsync, bool addClientInterceptor, bool throwCancellationError, ResponseHandleAction action)
10821098
{
1099+
// Provoke the garbage collector to find the unobserved exception.
1100+
GC.Collect();
1101+
// Wait for any failed tasks to be garbage collected
1102+
GC.WaitForPendingFinalizers();
1103+
10831104
// Arrange
10841105
var services = new ServiceCollection();
10851106
services.AddNUnitLogger();
@@ -1099,9 +1120,20 @@ public async Task AsyncUnaryCall_CallFailed_NoUnobservedExceptions(int expectedU
10991120

11001121
try
11011122
{
1102-
var httpClient = ClientTestHelpers.CreateTestClient(request =>
1123+
var httpClient = ClientTestHelpers.CreateTestClient(async request =>
11031124
{
1104-
throw new Exception("Test error");
1125+
if (isAsync)
1126+
{
1127+
await Task.Delay(50);
1128+
}
1129+
if (throwCancellationError)
1130+
{
1131+
throw new OperationCanceledException();
1132+
}
1133+
else
1134+
{
1135+
throw new Exception("Test error");
1136+
}
11051137
});
11061138
var serviceConfig = ServiceConfigHelpers.CreateRetryServiceConfig();
11071139
CallInvoker invoker = HttpClientCallInvokerFactory.Create(httpClient, loggerFactory: loggerFactory, serviceConfig: serviceConfig);
@@ -1112,32 +1144,28 @@ public async Task AsyncUnaryCall_CallFailed_NoUnobservedExceptions(int expectedU
11121144

11131145
// Act
11141146
logger.LogDebug("Starting call");
1115-
var awaitedException = await MakeGrpcCallAsync(logger, invoker, action);
1147+
await MakeGrpcCallAsync(logger, invoker, action);
11161148

11171149
logger.LogDebug("Waiting for finalizers");
1118-
// Provoke the garbage collector to find the unobserved exception.
1119-
GC.Collect();
1120-
// Wait for any failed tasks to be garbage collected
1121-
GC.WaitForPendingFinalizers();
1150+
for (var i = 0; i < 5; i++)
1151+
{
1152+
// Provoke the garbage collector to find the unobserved exception.
1153+
GC.Collect();
1154+
// Wait for any failed tasks to be garbage collected
1155+
GC.WaitForPendingFinalizers();
1156+
1157+
await Task.Delay(10);
1158+
}
11221159

11231160
foreach (var exception in unobservedExceptions)
11241161
{
11251162
logger.LogCritical(exception, "Unobserved task exception");
11261163
}
11271164

11281165
// Assert
1129-
try
1130-
{
1131-
Assert.AreEqual(expectedUnobservedExceptions, unobservedExceptions.Count);
1132-
logger.LogDebug("Expected number of observed exceptions");
1133-
}
1134-
catch
1135-
{
1136-
Assert.AreSame(unobservedExceptions.Single().InnerException, awaitedException);
1137-
logger.LogDebug("Observed exception was awaited by the test");
1138-
}
1166+
Assert.AreEqual(expectedUnobservedExceptions, unobservedExceptions.Count);
11391167

1140-
static async Task<Exception?> MakeGrpcCallAsync(ILogger logger, CallInvoker invoker, ResponseHandleAction action)
1168+
static async Task MakeGrpcCallAsync(ILogger logger, CallInvoker invoker, ResponseHandleAction action)
11411169
{
11421170
var runTask = Task.Run(async () =>
11431171
{
@@ -1146,21 +1174,23 @@ public async Task AsyncUnaryCall_CallFailed_NoUnobservedExceptions(int expectedU
11461174
switch (action)
11471175
{
11481176
case ResponseHandleAction.ResponseAsync:
1149-
return await ExceptionAssert.ThrowsAsync<RpcException>(() => call.ResponseAsync);
1177+
await ExceptionAssert.ThrowsAsync<RpcException>(() => call.ResponseAsync);
1178+
break;
11501179
case ResponseHandleAction.ResponseHeadersAsync:
1151-
return await ExceptionAssert.ThrowsAsync<RpcException>(() => call.ResponseHeadersAsync);
1180+
await ExceptionAssert.ThrowsAsync<RpcException>(() => call.ResponseHeadersAsync);
1181+
break;
11521182
case ResponseHandleAction.Dispose:
11531183
await WaitForCallCompleteAsync(logger, call);
11541184
call.Dispose();
1155-
return null;
1185+
break;
11561186
default:
11571187
// Do nothing (but wait until call is finished)
11581188
await WaitForCallCompleteAsync(logger, call);
1159-
return null;
1189+
break;
11601190
}
11611191
});
11621192

1163-
return await runTask;
1193+
await runTask;
11641194
}
11651195
}
11661196
finally

0 commit comments

Comments
 (0)