From 93bc5b2abc3f507f842879f9529a9d1fa253f26c Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Mon, 27 Jan 2020 19:56:00 +0100 Subject: [PATCH 01/14] HttpClient throws TimeoutException wrapped by TaskCancelledException when request times out --- .../src/Resources/Strings.resx | 5 +- .../src/System/Net/Http/HttpClient.cs | 86 +++++++++++++++++-- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Http/src/Resources/Strings.resx b/src/libraries/System.Net.Http/src/Resources/Strings.resx index 78929d254486f2..1e2e70b45a68fb 100644 --- a/src/libraries/System.Net.Http/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http/src/Resources/Strings.resx @@ -537,4 +537,7 @@ The HTTP/3 server attempted to reference a dynamic table index that does not exist. - + + The request timed out. + + \ No newline at end of file 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 05288da777fec6..672f79517da6a0 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 @@ -5,6 +5,7 @@ using System.Diagnostics; using System.IO; using System.Net.Http.Headers; +using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; @@ -491,13 +492,27 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl CancellationTokenSource cts; bool disposeCts; bool hasTimeout = _timeout != s_infiniteTimeout; + TimeoutState timeoutState = null; // Lazy initialization to avoid unnecessary alocation when timeout is not set + Timer timer = null; if (hasTimeout || cancellationToken.CanBeCanceled) { disposeCts = true; cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token); if (hasTimeout) { - cts.CancelAfter(_timeout); + long timeoutMilliseconds = (long)_timeout.TotalMilliseconds; + if (timeoutMilliseconds < -1 || timeoutMilliseconds > int.MaxValue) + { + throw new ArgumentOutOfRangeException(nameof(_timeout)); + } + + timeoutState = new TimeoutState(cts); + timer = new Timer(s => + { + var state = (TimeoutState)s; + state.IsFired = true; + state.CancellationSource.Cancel(); + }, timeoutState, timeoutMilliseconds, Threading.Timeout.Infinite); } } else @@ -512,6 +527,18 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl { sendTask = base.SendAsync(request, cts.Token); } + catch (TaskCanceledException e) + { + bool isTimedout = TimeoutFired(timeoutState, cancellationToken); + HandleFinishSendAsyncCleanup(cts, disposeCts); + + if (isTimedout) + { + ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); + } + + throw; + } catch { HandleFinishSendAsyncCleanup(cts, disposeCts); @@ -519,12 +546,12 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl } return completionOption == HttpCompletionOption.ResponseContentRead && !string.Equals(request.Method.Method, "HEAD", StringComparison.OrdinalIgnoreCase) ? - FinishSendAsyncBuffered(sendTask, request, cts, disposeCts) : - FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts); + FinishSendAsyncBuffered(sendTask, request, cts, disposeCts, timeoutState, cancellationToken, timer) : + FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts, timeoutState, cancellationToken, timer); } private async Task FinishSendAsyncBuffered( - Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts) + Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, TimeoutState timeoutState, CancellationToken callerToken, Timer timer) { HttpResponseMessage response = null; try @@ -545,6 +572,18 @@ private async Task FinishSendAsyncBuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } + catch (TaskCanceledException e) + { + bool isTimedout = TimeoutFired(timeoutState, callerToken); + HandleFinishSendAsyncCleanup(cts, disposeCts); + + if (isTimedout) + { + ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); + } + + throw; + } catch (Exception e) { response?.Dispose(); @@ -553,12 +592,13 @@ private async Task FinishSendAsyncBuffered( } finally { + timer?.Dispose(); HandleFinishSendAsyncCleanup(cts, disposeCts); } } private async Task FinishSendAsyncUnbuffered( - Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts) + Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, TimeoutState timeoutState, CancellationToken callerToken, Timer timer) { try { @@ -571,6 +611,18 @@ private async Task FinishSendAsyncUnbuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } + catch (TaskCanceledException e) + { + bool isTimedout = TimeoutFired(timeoutState, callerToken); + HandleFinishSendAsyncCleanup(cts, disposeCts); + + if (isTimedout) + { + ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); + } + + throw; + } catch (Exception e) { HandleFinishSendAsyncError(e, cts); @@ -578,10 +630,22 @@ private async Task FinishSendAsyncUnbuffered( } finally { + timer?.Dispose(); HandleFinishSendAsyncCleanup(cts, disposeCts); } } + private bool TimeoutFired(TimeoutState timeoutState, CancellationToken callerToken) + => !callerToken.IsCancellationRequested && !_pendingRequestsCts.IsCancellationRequested && timeoutState != null && timeoutState.IsFired; + + private static void ThrowTimeoutException(ExceptionDispatchInfo exceptionInfo) + { + var originalException = (TaskCanceledException)exceptionInfo.SourceException; + var timeoutException = new TimeoutException(SR.net_http_request_timedout, originalException); + var newTaskCancelledException = new TaskCanceledException(originalException.Message, timeoutException, originalException.CancellationToken); + throw newTaskCancelledException; + } + private void HandleFinishSendAsyncError(Exception e, CancellationTokenSource cts) { if (NetEventSource.IsEnabled) NetEventSource.Error(this, e); @@ -762,6 +826,18 @@ private Uri CreateUri(string uri) => private HttpRequestMessage CreateRequestMessage(HttpMethod method, Uri uri) => new HttpRequestMessage(method, uri) { Version = _defaultRequestVersion }; + + private class TimeoutState + { + public readonly CancellationTokenSource CancellationSource; + + public volatile bool IsFired; + + public TimeoutState(CancellationTokenSource cancellationSource) + { + CancellationSource = cancellationSource; + } + } #endregion Private Helpers } } From 70a0389aa540fd18d07bddb1337b5517cbcbd183 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 28 Jan 2020 14:45:12 +0100 Subject: [PATCH 02/14] Timer is disposed on all exceptions --- .../src/System/Net/Http/HttpClient.cs | 54 +++++++------------ .../tests/FunctionalTests/HttpClientTest.cs | 15 ++++++ 2 files changed, 35 insertions(+), 34 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 672f79517da6a0..df2affa267b935 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 @@ -511,7 +511,12 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl { var state = (TimeoutState)s; state.IsFired = true; - state.CancellationSource.Cancel(); + try + { + state.CancellationSource.Cancel(); + } + catch (ObjectDisposedException) // ObjectDisposedException is expected when the timer fires just after the CTS has been disposed + { } }, timeoutState, timeoutMilliseconds, Threading.Timeout.Infinite); } } @@ -527,21 +532,15 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl { sendTask = base.SendAsync(request, cts.Token); } - catch (TaskCanceledException e) + catch (TaskCanceledException e) when (TimeoutFired(timeoutState, cancellationToken)) { - bool isTimedout = TimeoutFired(timeoutState, cancellationToken); - HandleFinishSendAsyncCleanup(cts, disposeCts); - - if (isTimedout) - { - ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); - } - + HandleFinishSendAsyncCleanup(cts, disposeCts, timer); + ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); throw; } catch { - HandleFinishSendAsyncCleanup(cts, disposeCts); + HandleFinishSendAsyncCleanup(cts, disposeCts, timer); throw; } @@ -572,16 +571,10 @@ private async Task FinishSendAsyncBuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (TaskCanceledException e) + catch (TaskCanceledException e) when (TimeoutFired(timeoutState, callerToken)) { - bool isTimedout = TimeoutFired(timeoutState, callerToken); - HandleFinishSendAsyncCleanup(cts, disposeCts); - - if (isTimedout) - { - ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); - } - + response?.Dispose(); + ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); throw; } catch (Exception e) @@ -592,8 +585,7 @@ private async Task FinishSendAsyncBuffered( } finally { - timer?.Dispose(); - HandleFinishSendAsyncCleanup(cts, disposeCts); + HandleFinishSendAsyncCleanup(cts, disposeCts, timer); } } @@ -611,16 +603,9 @@ private async Task FinishSendAsyncUnbuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (TaskCanceledException e) + catch (TaskCanceledException e) when (TimeoutFired(timeoutState, callerToken)) { - bool isTimedout = TimeoutFired(timeoutState, callerToken); - HandleFinishSendAsyncCleanup(cts, disposeCts); - - if (isTimedout) - { - ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); - } - + ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); throw; } catch (Exception e) @@ -630,8 +615,7 @@ private async Task FinishSendAsyncUnbuffered( } finally { - timer?.Dispose(); - HandleFinishSendAsyncCleanup(cts, disposeCts); + HandleFinishSendAsyncCleanup(cts, disposeCts, timer); } } @@ -659,7 +643,7 @@ private void HandleFinishSendAsyncError(Exception e, CancellationTokenSource cts } } - private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disposeCts) + private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disposeCts, Timer timer) { // Dispose of the CancellationTokenSource if it was created specially for this request // rather than being used across multiple requests. @@ -668,6 +652,8 @@ private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disp cts.Dispose(); } + timer?.Dispose(); + // This method used to also dispose of the request content, e.g.: // request.Content?.Dispose(); // This has multiple problems: diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index 83efb460d574bc..2e7f73cf75ea2f 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -621,6 +621,21 @@ public async Task Timeout_SetTo30AndGetResponseQuickly_Success() } } + [Theory] + [InlineData(HttpCompletionOption.ResponseContentRead)] + [InlineData(HttpCompletionOption.ResponseHeadersRead)] + public async Task Timeout_TimeoutTriggeredOnBufferedResponse_ThrowsNestedTimeoutException(HttpCompletionOption completionOption) + { + using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) + { + client.Timeout = TimeSpan.FromMilliseconds(50); + TaskCanceledException e = await Assert.ThrowsAsync(async () => await client.GetAsync(CreateFakeUri(), completionOption)); + TimeoutException timeoutException = (TimeoutException) e.InnerException; + Assert.NotNull(timeoutException); + Assert.NotNull(timeoutException.InnerException); + } + } + [Fact] public void DefaultProxy_SetNull_Throws() { From 935d893a2bfbf79f466b94bf3be8b99708e390c3 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 28 Jan 2020 16:13:05 +0100 Subject: [PATCH 03/14] Timeout's conversion to long is removed as unnecessary --- .../System.Net.Http/src/System/Net/Http/HttpClient.cs | 8 +------- 1 file changed, 1 insertion(+), 7 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 df2affa267b935..510fcde14d7b14 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 @@ -500,12 +500,6 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token); if (hasTimeout) { - long timeoutMilliseconds = (long)_timeout.TotalMilliseconds; - if (timeoutMilliseconds < -1 || timeoutMilliseconds > int.MaxValue) - { - throw new ArgumentOutOfRangeException(nameof(_timeout)); - } - timeoutState = new TimeoutState(cts); timer = new Timer(s => { @@ -517,7 +511,7 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl } catch (ObjectDisposedException) // ObjectDisposedException is expected when the timer fires just after the CTS has been disposed { } - }, timeoutState, timeoutMilliseconds, Threading.Timeout.Infinite); + }, timeoutState, _timeout, Threading.Timeout.InfiniteTimeSpan); } } else From 355cd801b894ec65d82f2d909c246cbdb17cc584 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 28 Jan 2020 16:20:53 +0100 Subject: [PATCH 04/14] ExceptionDispatchInfo.Capture removed --- .../System.Net.Http/src/System/Net/Http/HttpClient.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 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 510fcde14d7b14..910d778f65ad15 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 @@ -529,7 +529,7 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl catch (TaskCanceledException e) when (TimeoutFired(timeoutState, cancellationToken)) { HandleFinishSendAsyncCleanup(cts, disposeCts, timer); - ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); + ThrowTimeoutException(e); throw; } catch @@ -568,7 +568,7 @@ private async Task FinishSendAsyncBuffered( catch (TaskCanceledException e) when (TimeoutFired(timeoutState, callerToken)) { response?.Dispose(); - ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); + ThrowTimeoutException(e); throw; } catch (Exception e) @@ -599,7 +599,7 @@ private async Task FinishSendAsyncUnbuffered( } catch (TaskCanceledException e) when (TimeoutFired(timeoutState, callerToken)) { - ThrowTimeoutException(ExceptionDispatchInfo.Capture(e)); + ThrowTimeoutException(e); throw; } catch (Exception e) @@ -616,9 +616,8 @@ private async Task FinishSendAsyncUnbuffered( private bool TimeoutFired(TimeoutState timeoutState, CancellationToken callerToken) => !callerToken.IsCancellationRequested && !_pendingRequestsCts.IsCancellationRequested && timeoutState != null && timeoutState.IsFired; - private static void ThrowTimeoutException(ExceptionDispatchInfo exceptionInfo) + private static void ThrowTimeoutException(TaskCanceledException originalException) { - var originalException = (TaskCanceledException)exceptionInfo.SourceException; var timeoutException = new TimeoutException(SR.net_http_request_timedout, originalException); var newTaskCancelledException = new TaskCanceledException(originalException.Message, timeoutException, originalException.CancellationToken); throw newTaskCancelledException; From ad0cad8aa51d57f1a9f06e7cc2e49ae45834644d Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev <55398552+alnikola@users.noreply.github.com> Date: Tue, 28 Jan 2020 16:54:49 +0100 Subject: [PATCH 05/14] Update src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs Co-Authored-By: Stephen Toub --- src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 910d778f65ad15..ca189c9f3845a3 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 @@ -492,7 +492,7 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl CancellationTokenSource cts; bool disposeCts; bool hasTimeout = _timeout != s_infiniteTimeout; - TimeoutState timeoutState = null; // Lazy initialization to avoid unnecessary alocation when timeout is not set + TimeoutState timeoutState = null; // Lazy initialization to avoid unnecessary allocation when timeout is not set Timer timer = null; if (hasTimeout || cancellationToken.CanBeCanceled) { From 8347c673d93681192c4e5bbd1551fe0169bfdf08 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 28 Jan 2020 17:04:57 +0100 Subject: [PATCH 06/14] Redundant using removed --- src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs | 1 - 1 file changed, 1 deletion(-) 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 910d778f65ad15..53efdc62ec5e4f 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 @@ -5,7 +5,6 @@ using System.Diagnostics; using System.IO; using System.Net.Http.Headers; -using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; From 29aaa4aaffd7176474325e2032aad723ae0ba02c Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 28 Jan 2020 17:55:38 +0100 Subject: [PATCH 07/14] - _pendingRequestCts check removed - TimeoutException is checked in an existing test - Timer rooting is tested --- .../src/System/Net/Http/HttpClient.cs | 2 +- .../tests/FunctionalTests/HttpClientTest.cs | 54 ++++++++++++------- 2 files changed, 36 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 7d85610e04cfd5..122ace90a9e06c 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 @@ -613,7 +613,7 @@ private async Task FinishSendAsyncUnbuffered( } private bool TimeoutFired(TimeoutState timeoutState, CancellationToken callerToken) - => !callerToken.IsCancellationRequested && !_pendingRequestsCts.IsCancellationRequested && timeoutState != null && timeoutState.IsFired; + => !callerToken.IsCancellationRequested && timeoutState != null && timeoutState.IsFired; private static void ThrowTimeoutException(TaskCanceledException originalException) { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index 2e7f73cf75ea2f..bfc2c16712932b 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -6,6 +6,7 @@ using System.IO; using System.Linq; using System.Net.Test.Common; +using System.Reflection; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -593,14 +594,44 @@ public void CancelAllPending_AllPendingOperationsCanceled(bool withInfiniteTimeo } } - [Fact] - public void Timeout_TooShort_AllPendingOperationsCanceled() + [Theory] + [InlineData(HttpCompletionOption.ResponseContentRead)] + [InlineData(HttpCompletionOption.ResponseHeadersRead)] + public void Timeout_TooShort_AllPendingOperationsCanceled(HttpCompletionOption completionOption) { using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) { client.Timeout = TimeSpan.FromMilliseconds(1); - Task[] tasks = Enumerable.Range(0, 3).Select(_ => client.GetAsync(CreateFakeUri())).ToArray(); - Assert.All(tasks, task => Assert.Throws(() => task.GetAwaiter().GetResult())); + Task[] tasks = Enumerable.Range(0, 3).Select(_ => client.GetAsync(CreateFakeUri(), completionOption)).ToArray(); + Assert.All(tasks, task => { + TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); + TimeoutException timeoutException = (TimeoutException)e.InnerException; + Assert.NotNull(timeoutException); + Assert.NotNull(timeoutException.InnerException); + }); + } + } + + + + [Theory] + [OuterLoop("Two second delay to ensure Full GC can finish before timer fires")] + [InlineData(HttpCompletionOption.ResponseContentRead)] + [InlineData(HttpCompletionOption.ResponseHeadersRead)] + public void Timeout_TimeoutTriggeredAfterFullGC_Success(HttpCompletionOption completionOption) + { + using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) + { + client.Timeout = TimeSpan.FromSeconds(2); + Task task = Assert.ThrowsAsync(async () => await client.GetAsync(CreateFakeUri(), completionOption)); + Runtime.GCSettings.LargeObjectHeapCompactionMode = Runtime.GCLargeObjectHeapCompactionMode.CompactOnce; + GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, true, true); + GC.WaitForPendingFinalizers(); + Assert.True(task.Wait(TimeSpan.FromSeconds(4))); + TaskCanceledException e = task.Result; + TimeoutException timeoutException = (TimeoutException)e.InnerException; + Assert.NotNull(timeoutException); + Assert.NotNull(timeoutException.InnerException); } } @@ -621,21 +652,6 @@ public async Task Timeout_SetTo30AndGetResponseQuickly_Success() } } - [Theory] - [InlineData(HttpCompletionOption.ResponseContentRead)] - [InlineData(HttpCompletionOption.ResponseHeadersRead)] - public async Task Timeout_TimeoutTriggeredOnBufferedResponse_ThrowsNestedTimeoutException(HttpCompletionOption completionOption) - { - using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) - { - client.Timeout = TimeSpan.FromMilliseconds(50); - TaskCanceledException e = await Assert.ThrowsAsync(async () => await client.GetAsync(CreateFakeUri(), completionOption)); - TimeoutException timeoutException = (TimeoutException) e.InnerException; - Assert.NotNull(timeoutException); - Assert.NotNull(timeoutException.InnerException); - } - } - [Fact] public void DefaultProxy_SetNull_Throws() { From aa543d0a62e12d5dad7a91f9b582b2a35dacba92 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Wed, 29 Jan 2020 16:09:13 +0100 Subject: [PATCH 08/14] - CancelAfter is brought back - Timing out is determined based on Environment.TickCount64 --- .../src/System/Net/Http/HttpClient.cs | 61 ++++++------------- .../tests/FunctionalTests/HttpClientTest.cs | 24 -------- 2 files changed, 18 insertions(+), 67 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 122ace90a9e06c..a370b7aa64b9d1 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 @@ -5,6 +5,7 @@ using System.Diagnostics; using System.IO; using System.Net.Http.Headers; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -491,26 +492,15 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl CancellationTokenSource cts; bool disposeCts; bool hasTimeout = _timeout != s_infiniteTimeout; - TimeoutState timeoutState = null; // Lazy initialization to avoid unnecessary allocation when timeout is not set - Timer timer = null; + long timeoutTime = 0; if (hasTimeout || cancellationToken.CanBeCanceled) { disposeCts = true; cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token); if (hasTimeout) { - timeoutState = new TimeoutState(cts); - timer = new Timer(s => - { - var state = (TimeoutState)s; - state.IsFired = true; - try - { - state.CancellationSource.Cancel(); - } - catch (ObjectDisposedException) // ObjectDisposedException is expected when the timer fires just after the CTS has been disposed - { } - }, timeoutState, _timeout, Threading.Timeout.InfiniteTimeSpan); + timeoutTime = Environment.TickCount64 + (long) _timeout.TotalMilliseconds; + cts.CancelAfter(_timeout); } } else @@ -525,25 +515,25 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl { sendTask = base.SendAsync(request, cts.Token); } - catch (TaskCanceledException e) when (TimeoutFired(timeoutState, cancellationToken)) + catch (OperationCanceledException e) when (TimeoutFired(timeoutTime)) { - HandleFinishSendAsyncCleanup(cts, disposeCts, timer); + HandleFinishSendAsyncCleanup(cts, disposeCts); ThrowTimeoutException(e); throw; } catch { - HandleFinishSendAsyncCleanup(cts, disposeCts, timer); + HandleFinishSendAsyncCleanup(cts, disposeCts); throw; } return completionOption == HttpCompletionOption.ResponseContentRead && !string.Equals(request.Method.Method, "HEAD", StringComparison.OrdinalIgnoreCase) ? - FinishSendAsyncBuffered(sendTask, request, cts, disposeCts, timeoutState, cancellationToken, timer) : - FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts, timeoutState, cancellationToken, timer); + FinishSendAsyncBuffered(sendTask, request, cts, disposeCts, timeoutTime) : + FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts, timeoutTime); } private async Task FinishSendAsyncBuffered( - Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, TimeoutState timeoutState, CancellationToken callerToken, Timer timer) + Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, long timeoutTime) { HttpResponseMessage response = null; try @@ -564,7 +554,7 @@ private async Task FinishSendAsyncBuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (TaskCanceledException e) when (TimeoutFired(timeoutState, callerToken)) + catch (OperationCanceledException e) when (TimeoutFired(timeoutTime)) { response?.Dispose(); ThrowTimeoutException(e); @@ -578,12 +568,12 @@ private async Task FinishSendAsyncBuffered( } finally { - HandleFinishSendAsyncCleanup(cts, disposeCts, timer); + HandleFinishSendAsyncCleanup(cts, disposeCts); } } private async Task FinishSendAsyncUnbuffered( - Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, TimeoutState timeoutState, CancellationToken callerToken, Timer timer) + Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, long timeoutTime) { try { @@ -596,7 +586,7 @@ private async Task FinishSendAsyncUnbuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (TaskCanceledException e) when (TimeoutFired(timeoutState, callerToken)) + catch (OperationCanceledException e) when (TimeoutFired(timeoutTime)) { ThrowTimeoutException(e); throw; @@ -608,14 +598,13 @@ private async Task FinishSendAsyncUnbuffered( } finally { - HandleFinishSendAsyncCleanup(cts, disposeCts, timer); + HandleFinishSendAsyncCleanup(cts, disposeCts); } } - private bool TimeoutFired(TimeoutState timeoutState, CancellationToken callerToken) - => !callerToken.IsCancellationRequested && timeoutState != null && timeoutState.IsFired; + private bool TimeoutFired(long timeoutTime) => timeoutTime > 0 && Environment.TickCount64 >= timeoutTime; - private static void ThrowTimeoutException(TaskCanceledException originalException) + private static void ThrowTimeoutException(OperationCanceledException originalException) { var timeoutException = new TimeoutException(SR.net_http_request_timedout, originalException); var newTaskCancelledException = new TaskCanceledException(originalException.Message, timeoutException, originalException.CancellationToken); @@ -635,7 +624,7 @@ private void HandleFinishSendAsyncError(Exception e, CancellationTokenSource cts } } - private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disposeCts, Timer timer) + private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disposeCts) { // Dispose of the CancellationTokenSource if it was created specially for this request // rather than being used across multiple requests. @@ -644,8 +633,6 @@ private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disp cts.Dispose(); } - timer?.Dispose(); - // This method used to also dispose of the request content, e.g.: // request.Content?.Dispose(); // This has multiple problems: @@ -804,18 +791,6 @@ private Uri CreateUri(string uri) => private HttpRequestMessage CreateRequestMessage(HttpMethod method, Uri uri) => new HttpRequestMessage(method, uri) { Version = _defaultRequestVersion }; - - private class TimeoutState - { - public readonly CancellationTokenSource CancellationSource; - - public volatile bool IsFired; - - public TimeoutState(CancellationTokenSource cancellationSource) - { - CancellationSource = cancellationSource; - } - } #endregion Private Helpers } } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index bfc2c16712932b..653dd42705b997 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -6,7 +6,6 @@ using System.IO; using System.Linq; using System.Net.Test.Common; -using System.Reflection; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -612,29 +611,6 @@ public void Timeout_TooShort_AllPendingOperationsCanceled(HttpCompletionOption c } } - - - [Theory] - [OuterLoop("Two second delay to ensure Full GC can finish before timer fires")] - [InlineData(HttpCompletionOption.ResponseContentRead)] - [InlineData(HttpCompletionOption.ResponseHeadersRead)] - public void Timeout_TimeoutTriggeredAfterFullGC_Success(HttpCompletionOption completionOption) - { - using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) - { - client.Timeout = TimeSpan.FromSeconds(2); - Task task = Assert.ThrowsAsync(async () => await client.GetAsync(CreateFakeUri(), completionOption)); - Runtime.GCSettings.LargeObjectHeapCompactionMode = Runtime.GCLargeObjectHeapCompactionMode.CompactOnce; - GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, true, true); - GC.WaitForPendingFinalizers(); - Assert.True(task.Wait(TimeSpan.FromSeconds(4))); - TaskCanceledException e = task.Result; - TimeoutException timeoutException = (TimeoutException)e.InnerException; - Assert.NotNull(timeoutException); - Assert.NotNull(timeoutException.InnerException); - } - } - [Fact] [OuterLoop("One second delay in getting server's response")] public async Task Timeout_SetTo30AndGetResponseQuickly_Success() From 192ea1df96cbbfebd615945d0128c79e78cd2b1e Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Wed, 29 Jan 2020 16:27:38 +0100 Subject: [PATCH 09/14] TimeoutException is not thrown if the caller canceled the token --- .../src/System/Net/Http/HttpClient.cs | 16 ++++++++-------- .../tests/FunctionalTests/HttpClientTest.cs | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 8 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 a370b7aa64b9d1..021281bd25fb28 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 @@ -515,7 +515,7 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl { sendTask = base.SendAsync(request, cts.Token); } - catch (OperationCanceledException e) when (TimeoutFired(timeoutTime)) + catch (OperationCanceledException e) when (TimeoutFired(cancellationToken, timeoutTime)) { HandleFinishSendAsyncCleanup(cts, disposeCts); ThrowTimeoutException(e); @@ -528,12 +528,12 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl } return completionOption == HttpCompletionOption.ResponseContentRead && !string.Equals(request.Method.Method, "HEAD", StringComparison.OrdinalIgnoreCase) ? - FinishSendAsyncBuffered(sendTask, request, cts, disposeCts, timeoutTime) : - FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts, timeoutTime); + FinishSendAsyncBuffered(sendTask, request, cts, disposeCts, cancellationToken, timeoutTime) : + FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts, cancellationToken, timeoutTime); } private async Task FinishSendAsyncBuffered( - Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, long timeoutTime) + Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, CancellationToken callerToken, long timeoutTime) { HttpResponseMessage response = null; try @@ -554,7 +554,7 @@ private async Task FinishSendAsyncBuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (OperationCanceledException e) when (TimeoutFired(timeoutTime)) + catch (OperationCanceledException e) when (TimeoutFired(callerToken, timeoutTime)) { response?.Dispose(); ThrowTimeoutException(e); @@ -573,7 +573,7 @@ private async Task FinishSendAsyncBuffered( } private async Task FinishSendAsyncUnbuffered( - Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, long timeoutTime) + Task sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, CancellationToken callerToken, long timeoutTime) { try { @@ -586,7 +586,7 @@ private async Task FinishSendAsyncUnbuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (OperationCanceledException e) when (TimeoutFired(timeoutTime)) + catch (OperationCanceledException e) when (TimeoutFired(callerToken, timeoutTime)) { ThrowTimeoutException(e); throw; @@ -602,7 +602,7 @@ private async Task FinishSendAsyncUnbuffered( } } - private bool TimeoutFired(long timeoutTime) => timeoutTime > 0 && Environment.TickCount64 >= timeoutTime; + private bool TimeoutFired(CancellationToken callerToken, long timeoutTime) => !callerToken.IsCancellationRequested && timeoutTime > 0 && Environment.TickCount64 >= timeoutTime; private static void ThrowTimeoutException(OperationCanceledException originalException) { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index 653dd42705b997..ffa56bf5eaffd8 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -611,6 +611,24 @@ public void Timeout_TooShort_AllPendingOperationsCanceled(HttpCompletionOption c } } + [Theory] + [InlineData(HttpCompletionOption.ResponseContentRead)] + [InlineData(HttpCompletionOption.ResponseHeadersRead)] + public void Timeout_CallerCanceledToken_TimeoutIsNotDetected(HttpCompletionOption completionOption) + { + using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) + { + client.Timeout = TimeSpan.FromMilliseconds(1); + CancellationTokenSource cts = new CancellationTokenSource(); + CancellationToken token = cts.Token; + cts.Cancel(); + Task.Delay(10).Wait(); + Task task = client.GetAsync(CreateFakeUri(), completionOption, token); + TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); + Assert.Null(e.InnerException); + } + } + [Fact] [OuterLoop("One second delay in getting server's response")] public async Task Timeout_SetTo30AndGetResponseQuickly_Success() From 04fdd789794224cd6157ec1fa18506ba5d819945 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Wed, 29 Jan 2020 16:45:05 +0100 Subject: [PATCH 10/14] More unit tests --- .../tests/FunctionalTests/HttpClientTest.cs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index ffa56bf5eaffd8..b6fc13b8a48c5b 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -614,21 +614,36 @@ public void Timeout_TooShort_AllPendingOperationsCanceled(HttpCompletionOption c [Theory] [InlineData(HttpCompletionOption.ResponseContentRead)] [InlineData(HttpCompletionOption.ResponseHeadersRead)] - public void Timeout_CallerCanceledToken_TimeoutIsNotDetected(HttpCompletionOption completionOption) + public void Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(HttpCompletionOption completionOption) { using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) { - client.Timeout = TimeSpan.FromMilliseconds(1); + client.Timeout = TimeSpan.FromMilliseconds(0.01); CancellationTokenSource cts = new CancellationTokenSource(); CancellationToken token = cts.Token; cts.Cancel(); - Task.Delay(10).Wait(); Task task = client.GetAsync(CreateFakeUri(), completionOption, token); TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); Assert.Null(e.InnerException); } } + [Theory] + [InlineData(HttpCompletionOption.ResponseContentRead)] + [InlineData(HttpCompletionOption.ResponseHeadersRead)] + public void Timeout_CallerCanceledTokenBeforeTimeout_TimeoutIsNotDetected(HttpCompletionOption completionOption) + { + using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) + { + client.Timeout = TimeSpan.FromSeconds(2); + CancellationTokenSource cts = new CancellationTokenSource(); + Task task = client.GetAsync(CreateFakeUri(), completionOption, cts.Token); + cts.Cancel(); + TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); + Assert.Null(e.InnerException); + } + } + [Fact] [OuterLoop("One second delay in getting server's response")] public async Task Timeout_SetTo30AndGetResponseQuickly_Success() From 46f21919bffa6ebebe945710f3ddef039344d698 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Fri, 31 Jan 2020 15:38:29 +0100 Subject: [PATCH 11/14] Multiple small changes to address comments --- .../src/Resources/Strings.resx | 2 +- .../src/System/Net/Http/HttpClient.cs | 49 +++++++++---------- .../tests/FunctionalTests/HttpClientTest.cs | 2 +- 3 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Net.Http/src/Resources/Strings.resx b/src/libraries/System.Net.Http/src/Resources/Strings.resx index 1e2e70b45a68fb..b11b27c8d5816c 100644 --- a/src/libraries/System.Net.Http/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http/src/Resources/Strings.resx @@ -538,6 +538,6 @@ The HTTP/3 server attempted to reference a dynamic table index that does not exist. - The request timed out. + The request was canceled due to the configured HttpClient.Timeout of {0} seconds elapsing. \ No newline at end of file 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 021281bd25fb28..aae7c058e32b4e 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 @@ -492,14 +492,14 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl CancellationTokenSource cts; bool disposeCts; bool hasTimeout = _timeout != s_infiniteTimeout; - long timeoutTime = 0; + long timeoutTime = long.MaxValue; if (hasTimeout || cancellationToken.CanBeCanceled) { disposeCts = true; cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token); if (hasTimeout) { - timeoutTime = Environment.TickCount64 + (long) _timeout.TotalMilliseconds; + timeoutTime = Environment.TickCount64 + (long)_timeout.TotalMilliseconds; cts.CancelAfter(_timeout); } } @@ -515,15 +515,15 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl { sendTask = base.SendAsync(request, cts.Token); } - catch (OperationCanceledException e) when (TimeoutFired(cancellationToken, timeoutTime)) - { - HandleFinishSendAsyncCleanup(cts, disposeCts); - ThrowTimeoutException(e); - throw; - } - catch + catch (Exception e) { HandleFinishSendAsyncCleanup(cts, disposeCts); + + if (e is OperationCanceledException operationException && TimeoutFired(cancellationToken, timeoutTime)) + { + throw CreateTimeoutException(operationException); + } + throw; } @@ -554,15 +554,15 @@ private async Task FinishSendAsyncBuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (OperationCanceledException e) when (TimeoutFired(callerToken, timeoutTime)) - { - response?.Dispose(); - ThrowTimeoutException(e); - throw; - } catch (Exception e) { response?.Dispose(); + + if (e is OperationCanceledException operationException && TimeoutFired(callerToken, timeoutTime)) + { + throw CreateTimeoutException(operationException); + } + HandleFinishSendAsyncError(e, cts); throw; } @@ -586,13 +586,13 @@ private async Task FinishSendAsyncUnbuffered( if (NetEventSource.IsEnabled) NetEventSource.ClientSendCompleted(this, response, request); return response; } - catch (OperationCanceledException e) when (TimeoutFired(callerToken, timeoutTime)) - { - ThrowTimeoutException(e); - throw; - } catch (Exception e) { + if (e is OperationCanceledException operationException && TimeoutFired(callerToken, timeoutTime)) + { + throw CreateTimeoutException(operationException); + } + HandleFinishSendAsyncError(e, cts); throw; } @@ -602,13 +602,12 @@ private async Task FinishSendAsyncUnbuffered( } } - private bool TimeoutFired(CancellationToken callerToken, long timeoutTime) => !callerToken.IsCancellationRequested && timeoutTime > 0 && Environment.TickCount64 >= timeoutTime; + private bool TimeoutFired(CancellationToken callerToken, long timeoutTime) => !callerToken.IsCancellationRequested && Environment.TickCount64 >= timeoutTime; - private static void ThrowTimeoutException(OperationCanceledException originalException) + private TaskCanceledException CreateTimeoutException(OperationCanceledException originalException) { - var timeoutException = new TimeoutException(SR.net_http_request_timedout, originalException); - var newTaskCancelledException = new TaskCanceledException(originalException.Message, timeoutException, originalException.CancellationToken); - throw newTaskCancelledException; + return new TaskCanceledException(string.Format(SR.net_http_request_timedout, _timeout.TotalSeconds), + new TimeoutException(originalException.Message, originalException), originalException.CancellationToken); } private void HandleFinishSendAsyncError(Exception e, CancellationTokenSource cts) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index b6fc13b8a48c5b..7123eb51d6e858 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -635,7 +635,7 @@ public void Timeout_CallerCanceledTokenBeforeTimeout_TimeoutIsNotDetected(HttpCo { using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) { - client.Timeout = TimeSpan.FromSeconds(2); + client.Timeout = TimeSpan.FromDays(1); CancellationTokenSource cts = new CancellationTokenSource(); Task task = client.GetAsync(CreateFakeUri(), completionOption, cts.Token); cts.Cancel(); From 730b6f03ef66295dcd7860de9f9d3404744047b9 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Fri, 31 Jan 2020 16:23:35 +0100 Subject: [PATCH 12/14] Tests use ThrowsAny() --- .../System.Net.Http/tests/FunctionalTests/HttpClientTest.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index 7123eb51d6e858..b42097c02df388 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -603,7 +603,7 @@ public void Timeout_TooShort_AllPendingOperationsCanceled(HttpCompletionOption c client.Timeout = TimeSpan.FromMilliseconds(1); Task[] tasks = Enumerable.Range(0, 3).Select(_ => client.GetAsync(CreateFakeUri(), completionOption)).ToArray(); Assert.All(tasks, task => { - TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); + OperationCanceledException e = Assert.ThrowsAny(() => task.GetAwaiter().GetResult()); TimeoutException timeoutException = (TimeoutException)e.InnerException; Assert.NotNull(timeoutException); Assert.NotNull(timeoutException.InnerException); @@ -623,7 +623,7 @@ public void Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(HttpCom CancellationToken token = cts.Token; cts.Cancel(); Task task = client.GetAsync(CreateFakeUri(), completionOption, token); - TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); + OperationCanceledException e = Assert.ThrowsAny(() => task.GetAwaiter().GetResult()); Assert.Null(e.InnerException); } } @@ -639,7 +639,7 @@ public void Timeout_CallerCanceledTokenBeforeTimeout_TimeoutIsNotDetected(HttpCo CancellationTokenSource cts = new CancellationTokenSource(); Task task = client.GetAsync(CreateFakeUri(), completionOption, cts.Token); cts.Cancel(); - TaskCanceledException e = Assert.Throws(() => task.GetAwaiter().GetResult()); + OperationCanceledException e = Assert.ThrowsAny(() => task.GetAwaiter().GetResult()); Assert.Null(e.InnerException); } } From 7a01b85074ab0ad0c6ee6c35c9bb25c0f661e281 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 4 Feb 2020 13:19:04 +0100 Subject: [PATCH 13/14] A few optimizations --- .../System.Net.Http/src/System/Net/Http/HttpClient.cs | 2 +- .../System.Net.Http/tests/FunctionalTests/HttpClientTest.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 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 aae7c058e32b4e..ee846eed99c471 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 @@ -499,7 +499,7 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token); if (hasTimeout) { - timeoutTime = Environment.TickCount64 + (long)_timeout.TotalMilliseconds; + timeoutTime = Environment.TickCount64 + (_timeout.Ticks/TimeSpan.TicksPerMillisecond); cts.CancelAfter(_timeout); } } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs index b42097c02df388..ad642fc61b103a 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs @@ -614,7 +614,7 @@ public void Timeout_TooShort_AllPendingOperationsCanceled(HttpCompletionOption c [Theory] [InlineData(HttpCompletionOption.ResponseContentRead)] [InlineData(HttpCompletionOption.ResponseHeadersRead)] - public void Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(HttpCompletionOption completionOption) + public async Task Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(HttpCompletionOption completionOption) { using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled(c)))) { @@ -623,7 +623,7 @@ public void Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(HttpCom CancellationToken token = cts.Token; cts.Cancel(); Task task = client.GetAsync(CreateFakeUri(), completionOption, token); - OperationCanceledException e = Assert.ThrowsAny(() => task.GetAwaiter().GetResult()); + OperationCanceledException e = await Assert.ThrowsAnyAsync(async () => await task); Assert.Null(e.InnerException); } } From 6adf5c3c32a817672c41678251e246ccee63a8eb Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Thu, 6 Feb 2020 19:06:22 +0100 Subject: [PATCH 14/14] Code style fix Timeout cancellation is logged --- .../src/System/Net/Http/HttpClient.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 ee846eed99c471..ba716dae88c534 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 @@ -499,7 +499,7 @@ public Task SendAsync(HttpRequestMessage request, HttpCompl cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token); if (hasTimeout) { - timeoutTime = Environment.TickCount64 + (_timeout.Ticks/TimeSpan.TicksPerMillisecond); + timeoutTime = Environment.TickCount64 + (_timeout.Ticks / TimeSpan.TicksPerMillisecond); cts.CancelAfter(_timeout); } } @@ -560,6 +560,7 @@ private async Task FinishSendAsyncBuffered( if (e is OperationCanceledException operationException && TimeoutFired(callerToken, timeoutTime)) { + HandleSendAsyncTimeout(operationException); throw CreateTimeoutException(operationException); } @@ -590,6 +591,7 @@ private async Task FinishSendAsyncUnbuffered( { if (e is OperationCanceledException operationException && TimeoutFired(callerToken, timeoutTime)) { + HandleSendAsyncTimeout(operationException); throw CreateTimeoutException(operationException); } @@ -623,6 +625,15 @@ private void HandleFinishSendAsyncError(Exception e, CancellationTokenSource cts } } + private void HandleSendAsyncTimeout(OperationCanceledException e) + { + if (NetEventSource.IsEnabled) + { + NetEventSource.Error(this, e); + NetEventSource.Error(this, "Canceled due to timeout"); + } + } + private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disposeCts) { // Dispose of the CancellationTokenSource if it was created specially for this request