From 42af85bee852ecb7bb3d8e43d5793e1898d3f99a Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 24 May 2021 14:22:10 -0400 Subject: [PATCH] Fix the token included by HttpClient.HandleFailure (#53133) --- .../src/System/Net/Http/HttpClient.cs | 26 +++++++--- .../tests/FunctionalTests/HttpClientTest.cs | 47 +++++++++++++------ 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs index 5716b669c4461..b09850ec9a4b0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs @@ -583,17 +583,31 @@ private void HandleFailure(Exception e, bool telemetryStarted, HttpResponseMessa Exception? toThrow = null; - if (e is OperationCanceledException oce && !cancellationToken.IsCancellationRequested && !pendingRequestsCts.IsCancellationRequested) + if (e is OperationCanceledException oce) { - // If this exception is for cancellation, but cancellation wasn't requested, either by the caller's token or by the pending requests source, - // the only other cause could be a timeout. Treat it as such. - e = toThrow = new TaskCanceledException(SR.Format(SR.net_http_request_timedout, _timeout.TotalSeconds), new TimeoutException(e.Message, e), oce.CancellationToken); + if (cancellationToken.IsCancellationRequested) + { + if (oce.CancellationToken != cancellationToken) + { + // We got a cancellation exception, and the caller requested cancellation, but the exception doesn't contain that token. + // Massage things so that the cancellation exception we propagate appropriately contains the caller's token (it's possible + // multiple things caused cancellation, in which case we can attribute it to the caller's token, or it's possible the + // exception contains the linked token source, in which case that token isn't meaningful to the caller). + e = toThrow = new TaskCanceledException(oce.Message, oce.InnerException, cancellationToken); + } + } + else if (!pendingRequestsCts.IsCancellationRequested) + { + // If this exception is for cancellation, but cancellation wasn't requested, either by the caller's token or by the pending requests source, + // the only other cause could be a timeout. Treat it as such. + e = toThrow = new TaskCanceledException(SR.Format(SR.net_http_request_timedout, _timeout.TotalSeconds), new TimeoutException(e.Message, e), oce.CancellationToken); + } } - else if (cts.IsCancellationRequested && e is HttpRequestException) // if cancellationToken is canceled, cts will also be canceled + else if (e is HttpRequestException && cts.IsCancellationRequested) // if cancellationToken is canceled, cts will also be canceled { // If the cancellation token source was canceled, race conditions abound, and we consider the failure to be // caused by the cancellation (e.g. WebException when reading from canceled response stream). - e = toThrow = new OperationCanceledException(cts.Token); + e = toThrow = new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : cts.Token); } if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, e); diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index 883bb8cf72583..06d3f2a5ed47d 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -289,11 +289,18 @@ public async Task GetPutPostDeleteAsync_Canceled_Throws() cts.Cancel(); - await Assert.ThrowsAsync(() => t1); - await Assert.ThrowsAsync(() => t2); - await Assert.ThrowsAsync(() => t3); - await Assert.ThrowsAsync(() => t4); - await Assert.ThrowsAsync(() => t5); + async Task ValidateCancellationAsync(Task t) + { + TaskCanceledException tce = await Assert.ThrowsAsync(() => t); + Assert.Equal(cts.Token, tce.CancellationToken); + + } + + await ValidateCancellationAsync(t1); + await ValidateCancellationAsync(t2); + await ValidateCancellationAsync(t3); + await ValidateCancellationAsync(t4); + await ValidateCancellationAsync(t5); } } @@ -382,7 +389,9 @@ await LoopbackServer.CreateClientAndServerAsync( var cts = new CancellationTokenSource(); cts.Cancel(); - await Assert.ThrowsAsync(() => httpClient.GetStringAsync(uri, cts.Token)); + TaskCanceledException tce = await Assert.ThrowsAsync(() => httpClient.GetStringAsync(uri, cts.Token)); + Assert.Equal(cts.Token, tce.CancellationToken); + onClientFinished.Release(); }, async server => @@ -402,7 +411,8 @@ await LoopbackServer.CreateClientAndServerAsync( { using HttpClient httpClient = CreateHttpClient(); - await Assert.ThrowsAsync(() => httpClient.GetStringAsync(uri, cts.Token)); + TaskCanceledException tce = await Assert.ThrowsAsync(() => httpClient.GetStringAsync(uri, cts.Token)); + Assert.Equal(cts.Token, tce.CancellationToken); }, async server => { @@ -549,7 +559,9 @@ await LoopbackServer.CreateClientAndServerAsync( var cts = new CancellationTokenSource(); cts.Cancel(); - await Assert.ThrowsAsync(() => httpClient.GetByteArrayAsync(uri, cts.Token)); + TaskCanceledException tce = await Assert.ThrowsAsync(() => httpClient.GetByteArrayAsync(uri, cts.Token)); + Assert.Equal(cts.Token, tce.CancellationToken); + onClientFinished.Release(); }, async server => @@ -569,7 +581,8 @@ await LoopbackServer.CreateClientAndServerAsync( { using HttpClient httpClient = CreateHttpClient(); - await Assert.ThrowsAsync(() => httpClient.GetByteArrayAsync(uri, cts.Token)); + TaskCanceledException tce = await Assert.ThrowsAsync(() => httpClient.GetByteArrayAsync(uri, cts.Token)); + Assert.Equal(cts.Token, tce.CancellationToken); }, async server => { @@ -623,7 +636,9 @@ await LoopbackServer.CreateClientAndServerAsync( var cts = new CancellationTokenSource(); cts.Cancel(); - await Assert.ThrowsAsync(() => httpClient.GetStreamAsync(uri, cts.Token)); + TaskCanceledException tce = await Assert.ThrowsAsync(() => httpClient.GetStreamAsync(uri, cts.Token)); + Assert.Equal(cts.Token, tce.CancellationToken); + onClientFinished.Release(); }, async server => @@ -643,7 +658,8 @@ await LoopbackServer.CreateClientAndServerAsync( { using HttpClient httpClient = CreateHttpClient(); - await Assert.ThrowsAsync(() => httpClient.GetStreamAsync(uri, cts.Token)); + TaskCanceledException tce = await Assert.ThrowsAsync(() => httpClient.GetStreamAsync(uri, cts.Token)); + Assert.Equal(cts.Token, tce.CancellationToken); }, async server => { @@ -729,7 +745,7 @@ public async Task Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(H cts.Cancel(); Task task = client.GetAsync(CreateFakeUri(), completionOption, token); OperationCanceledException e = await Assert.ThrowsAnyAsync(async () => await task); - Assert.Null(e.InnerException); + Assert.Equal(e.CancellationToken, token); } } @@ -746,7 +762,7 @@ public void Timeout_CallerCanceledTokenBeforeTimeout_TimeoutIsNotDetected(HttpCo Task task = client.GetAsync(CreateFakeUri(), completionOption, cts.Token); cts.Cancel(); OperationCanceledException e = Assert.ThrowsAny(() => task.GetAwaiter().GetResult()); - Assert.Null(e.InnerException); + Assert.Equal(e.CancellationToken, cts.Token); } } @@ -821,7 +837,8 @@ public async Task PatchAsync_Canceled_Throws() cts.Cancel(); - await Assert.ThrowsAsync(() => t1); + TaskCanceledException tce = await Assert.ThrowsAsync(() => t1); + Assert.Equal(cts.Token, tce.CancellationToken); } } @@ -966,6 +983,7 @@ await LoopbackServer.CreateClientAndServerAsync( }); TaskCanceledException ex = await Assert.ThrowsAsync(() => sendTask); + Assert.Equal(cts.Token, ex.CancellationToken); Assert.IsNotType(ex.InnerException); }, async server => @@ -1050,6 +1068,7 @@ await LoopbackServer.CreateClientAndServerAsync( }); TaskCanceledException ex = await Assert.ThrowsAsync(() => sendTask); + Assert.Equal(cts.Token, ex.CancellationToken); Assert.IsNotType(ex.InnerException); }, async server =>