Skip to content

HttpClient throws TimeoutException wrapped by TaskCancellationException when request times out #2281

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 16 commits into from
Feb 19, 2020
Merged

HttpClient throws TimeoutException wrapped by TaskCancellationException when request times out #2281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
93bc5b2
HttpClient throws TimeoutException wrapped by TaskCancelledException …
alnikola Jan 27, 2020
70a0389
Timer is disposed on all exceptions
alnikola Jan 28, 2020
935d893
Timeout's conversion to long is removed as unnecessary
alnikola Jan 28, 2020
355cd80
ExceptionDispatchInfo.Capture removed
alnikola Jan 28, 2020
ad0cad8
Update src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
alnikola Jan 28, 2020
8347c67
Redundant using removed
alnikola Jan 28, 2020
e1a84bf
Merge branch 'alnikola/20296-httpclient-timeout-exception' of https:/…
alnikola Jan 28, 2020
29aaa4a
- _pendingRequestCts check removed
alnikola Jan 28, 2020
aa543d0
- CancelAfter is brought back
alnikola Jan 29, 2020
192ea1d
TimeoutException is not thrown if the caller canceled the token
alnikola Jan 29, 2020
04fdd78
More unit tests
alnikola Jan 29, 2020
46f2191
Multiple small changes to address comments
alnikola Jan 31, 2020
730b6f0
Tests use ThrowsAny<OperationCancelledException>()
alnikola Jan 31, 2020
7a01b85
A few optimizations
alnikola Feb 4, 2020
6adf5c3
Code style fix
alnikola Feb 6, 2020
7ea3e19
Merge branch 'master' into alnikola/20296-httpclient-timeout-exception
alnikola Feb 17, 2020
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
5 changes: 4 additions & 1 deletion src/libraries/System.Net.Http/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,4 +537,7 @@
<data name="net_http_qpack_no_dynamic_table" xml:space="preserve">
<value>The HTTP/3 server attempted to reference a dynamic table index that does not exist.</value>
</data>
</root>
<data name="net_http_request_timedout" xml:space="preserve">
<value>The request was canceled due to the configured HttpClient.Timeout of {0} seconds elapsing.</value>
</data>
</root>
49 changes: 44 additions & 5 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -491,12 +492,14 @@ public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, HttpCompl
CancellationTokenSource cts;
bool disposeCts;
bool hasTimeout = _timeout != s_infiniteTimeout;
long timeoutTime = long.MaxValue;
if (hasTimeout || cancellationToken.CanBeCanceled)
{
disposeCts = true;
cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, _pendingRequestsCts.Token);
if (hasTimeout)
{
timeoutTime = Environment.TickCount64 + (_timeout.Ticks / TimeSpan.TicksPerMillisecond);
cts.CancelAfter(_timeout);
}
}
Expand All @@ -512,19 +515,25 @@ public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, HttpCompl
{
sendTask = base.SendAsync(request, cts.Token);
}
catch
catch (Exception e)
{
HandleFinishSendAsyncCleanup(cts, disposeCts);

if (e is OperationCanceledException operationException && TimeoutFired(cancellationToken, timeoutTime))
{
throw CreateTimeoutException(operationException);
}

throw;
}

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, cancellationToken, timeoutTime) :
FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts, cancellationToken, timeoutTime);
}

private async Task<HttpResponseMessage> FinishSendAsyncBuffered(
Task<HttpResponseMessage> sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts)
Task<HttpResponseMessage> sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, CancellationToken callerToken, long timeoutTime)
{
HttpResponseMessage response = null;
try
Expand All @@ -548,6 +557,13 @@ private async Task<HttpResponseMessage> FinishSendAsyncBuffered(
catch (Exception e)
{
response?.Dispose();

if (e is OperationCanceledException operationException && TimeoutFired(callerToken, timeoutTime))
{
HandleSendAsyncTimeout(operationException);
throw CreateTimeoutException(operationException);
}

HandleFinishSendAsyncError(e, cts);
throw;
}
Expand All @@ -558,7 +574,7 @@ private async Task<HttpResponseMessage> FinishSendAsyncBuffered(
}

private async Task<HttpResponseMessage> FinishSendAsyncUnbuffered(
Task<HttpResponseMessage> sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts)
Task<HttpResponseMessage> sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts, CancellationToken callerToken, long timeoutTime)
{
try
{
Expand All @@ -573,6 +589,12 @@ private async Task<HttpResponseMessage> FinishSendAsyncUnbuffered(
}
catch (Exception e)
{
if (e is OperationCanceledException operationException && TimeoutFired(callerToken, timeoutTime))
Copy link
Contributor

@scalablecory scalablecory Feb 14, 2020

Choose a reason for hiding this comment

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

It looks like this could be an exception filter.

Copy link
Member

@stephentoub stephentoub Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If it's an exception filter, then you need two catch blocks; doesn't really help anything, e.g.

catch (OperationCanceledException oce) when TimeoutFired(callerToken, timeoutTime)
{
    HandleSendAsyncTimeout(oce);
    throw CreateTimeoutException(oce);
}
catch (Exception e)
{
    HandleFinishSendAsyncError(e, cts);
    throw;
}

But either way is fine.

Copy link
Contributor

@scalablecory scalablecory Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I find the two catch blocks cleaner; is there a reason to avoid multiple catch blocks when they have entirely exclusive bodies? perf concerns?

Copy link
Member

@stephentoub stephentoub Feb 14, 2020

Choose a reason for hiding this comment

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

Previously they weren't entirely discrete:
#2281 (comment)
As they are now, it's really in the eye of the beholder. The two blocks with an exception filter will add some expense when the exception occurs, but given that it's only in the exceptional case, it's not prohibitive. Whichever you both decide is fine.

Copy link
Contributor Author

@alnikola alnikola Feb 17, 2020

Choose a reason for hiding this comment

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

@scalablecory Do you want me to change it back to the exception filters? I'd rather not do this since it will require rerunning all perf tests for Linux and Windows which takes a lot of time.

Copy link
Contributor

@davidsh davidsh Feb 17, 2020

Choose a reason for hiding this comment

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

My opinion is that this is fine as-is. If we think this is a problem later, you can change it.

{
HandleSendAsyncTimeout(operationException);
throw CreateTimeoutException(operationException);
}

HandleFinishSendAsyncError(e, cts);
throw;
}
Expand All @@ -582,6 +604,14 @@ private async Task<HttpResponseMessage> FinishSendAsyncUnbuffered(
}
}

private bool TimeoutFired(CancellationToken callerToken, long timeoutTime) => !callerToken.IsCancellationRequested && Environment.TickCount64 >= timeoutTime;

private TaskCanceledException CreateTimeoutException(OperationCanceledException originalException)
{
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)
{
if (NetEventSource.IsEnabled) NetEventSource.Error(this, e);
Expand All @@ -595,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
Expand Down
48 changes: 44 additions & 4 deletions src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,14 +593,54 @@ 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<HttpResponseMessage>(c))))
{
client.Timeout = TimeSpan.FromMilliseconds(1);
Task<HttpResponseMessage>[] tasks = Enumerable.Range(0, 3).Select(_ => client.GetAsync(CreateFakeUri())).ToArray();
Assert.All(tasks, task => Assert.Throws<TaskCanceledException>(() => task.GetAwaiter().GetResult()));
Task<HttpResponseMessage>[] tasks = Enumerable.Range(0, 3).Select(_ => client.GetAsync(CreateFakeUri(), completionOption)).ToArray();
Assert.All(tasks, task => {
OperationCanceledException e = Assert.ThrowsAny<OperationCanceledException>(() => task.GetAwaiter().GetResult());
TimeoutException timeoutException = (TimeoutException)e.InnerException;
Assert.NotNull(timeoutException);
Assert.NotNull(timeoutException.InnerException);
});
}
}

[Theory]
[InlineData(HttpCompletionOption.ResponseContentRead)]
[InlineData(HttpCompletionOption.ResponseHeadersRead)]
public async Task Timeout_CallerCanceledTokenAfterTimeout_TimeoutIsNotDetected(HttpCompletionOption completionOption)
{
using (var client = new HttpClient(new CustomResponseHandler((r, c) => WhenCanceled<HttpResponseMessage>(c))))
{
client.Timeout = TimeSpan.FromMilliseconds(0.01);
CancellationTokenSource cts = new CancellationTokenSource();
CancellationToken token = cts.Token;
cts.Cancel();
Task<HttpResponseMessage> task = client.GetAsync(CreateFakeUri(), completionOption, token);
OperationCanceledException e = await Assert.ThrowsAnyAsync<OperationCanceledException>(async () => await task);
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<HttpResponseMessage>(c))))
{
client.Timeout = TimeSpan.FromDays(1);
CancellationTokenSource cts = new CancellationTokenSource();
Task<HttpResponseMessage> task = client.GetAsync(CreateFakeUri(), completionOption, cts.Token);
cts.Cancel();
OperationCanceledException e = Assert.ThrowsAny<OperationCanceledException>(() => task.GetAwaiter().GetResult());
Assert.Null(e.InnerException);
}
}

Expand Down