HttpClient throws TaskCanceledException on timeout

[awithy]

HttpClient is throwing a TaskCanceledException on timeout in some circumstances. This is happening for us when the server is under heavy load. We were able to work around by increasing the default timeout. The following MSDN forum thread captures the essence of the experienced issue: https://social.msdn.microsoft.com/Forums/en-US/d8d87789-0ac9-4294-84a0-91c9fa27e353/bug-in-httpclientgetasync-should-throw-webexception-not-taskcanceledexception?forum=netfxnetcom&prof=required

Thanks

[danmoseley]

@awithy what version of .NET Core are you using?

[awithy]

1.1

[stephentoub]

Whether the right design or not, by design OperationCanceledExceptions are thrown for timeouts (and TaskCanceledException is an OperationCanceledException).
cc: @davidsh

[awithy]

Our team found this unintuitive, but it does seem to be working as designed. Unfortunately, when we hit this, we wasted a bit of time trying to find a source of cancelation. Having to handle this scenario differently from task cancelation is pretty ugly (we created custom handler to manage this). This also seems to be an overuse of cancelation as a concept.

Thanks again for thee quick response.

[karelz]

Seems like by design + there were only 2 customers complains in last 6+ months.
Closing.

[jamescrowley]

@karelz adding a note as another customer impacted by this misleading exception :)

[karelz]

Thanks for info, please upvote also top post (issues can be sorted by it), thanks!

[thomaslevesque]

Whether the right design or not, by design OperationCanceledExceptions are thrown for timeouts

This is a bad design IMO. There's no way to tell if the request was actually canceled (i.e. the cancellation token passed to SendAsync was canceled) or if the timeout was elapsed. In the latter case, it would make sense to retry, whereas in the former case it wouldn't. There's a TimeoutException that would be perfect for this case, why not use it?

[csrowell]

Yeah, this also threw our team for a loop. There's an entire Stack Overflow thread dedicated to trying to deal with it: https://stackoverflow.com/questions/10547895/how-can-i-tell-when-httpclient-has-timed-out/32230327#32230327

[thomaslevesque]

I published a workaround a few days ago: https://www.thomaslevesque.com/2018/02/25/better-timeout-handling-with-httpclient/

[csrowell]

Awesome, thanks.

[karelz]

Reopening as the interest is now bigger - the StackOverflow thread has now 69 upvotes.

cc @dotnet/ncl

[Davilink]

Any news ? Still happen in dotnet core 2.0

[karelz]

It is on our list of top problems for post-2.1. It won't be fixed in 2.0/2.1 though.

[karelz]

We have several options what to do when timeout happens:

1. Throw TimeoutException instead of TaskCanceledException.
   • Pro: Easy to distinguish timeout from explicit cancellation action at runtime
   • Con: Technical breaking change - new type of exception is thrown.
2. Throw TaskCanceledException with inner exception as TimeoutException
   • Pro: Possible to distinguish timeout from explicit cancellation action at runtime. Compatible exception type.
   • Open question: Is it ok to throw away original TaskCanceledException stack and throw a new one, while preserving InnerException (as TimeoutException.InnerException)? Or should we preserve and just wrap the original TaskCanceledException?
     • Either: new TaskCanceledException -> new TimeoutException -> original TaskCanceledException (with original InnerException which may be null)
     • Or: new TaskCanceledException -> new TimeoutException -> original TaskCanceledException.InnerException (may be null)
3. Throw TaskCanceledException with message mentioning timeout as the reason
   • Pro: Possible to distinguish timeout from explicit cancellation action from the message / logs
   • Con: Cannot be distinguished at runtime, it is "debug only".
   • Open question: Same as in [2] - should we wrap or replace the original TaskCanceledException (and it stack)

I am leaning towards option [2], with discarding original stack of original TaskCanceledException.
@stephentoub @davidsh any thoughts?

BTW: The change should be fairly straightforward in HttpClient.SendAsync where we set up the timeout:
https://github.com/dotnet/corefx/blob/30fb78875148665b98748ede3013641734d9bf5c/src/System.Net.Http/src/System/Net/Http/HttpClient.cs#L433-L461

[davidsh]

The top-level exception should always be an HttpRequestException. That is the standard API model for HttpClient.

Within that, as inner exception, "timeouts" should probably be a TimeoutException. In fact, if we were to combine this with the other issues (about modifying HttpRequestion to include a new enum property similar to WebExceptionStatus), then developers would only have to check that enum and not have to inspect inner exceptions at all.

This is basically option 1) but continuing to preserve current app-compat API model that top-level exceptions from HttpClient APIs are always HttpRequestException.

Note: that any "timeouts" from directly reading an HTTP response stream from Stream APIs like:

var client = new HttpClient();
Stream responseStream = await client.GetStreamAsync(url);

int bytesRead = await responseStream.ReadAsync(buffer, 0, buffer.Length);

should continue to throw System.IO.IOException etc. as top-level exceptions (we actually have a few bugs in SocketsHttpHandler about this).

[karelz]

@davidsh are you suggesting to throw HttpRequestException even in cases when there is explicit user cancellation? Those throw TaskCanceledException today and it seems fine. Not sure if it is something we want to change ...
I'd be ok to expose client-side timeouts as HttpRequestException instead of TaskCanceledException.

[davidsh]

@davidsh are you suggesting to throw HttpRequestException even in cases when there is explicit user cancellation? Those throw TaskCanceledException today and it seems fine. Not sure if it is something we want to change …

I need to test out the different behaviors of the stacks including .NET Framework to verify the exact current behaviors before I can answer this question.

Also, in some cases, some of our stacks are throwing HttpRequestException with inner exception of OperationCanceledException. TaskCanceledException is a derived type of OperationCanceledException.

when there is explicit user cancellation

We also need clear definition of what "explicit" is. For example, is setting the HttpClient.Timeout property "explicit"? I think we're saying no. What about calling .Cancel() on the CancellationTokenSource object (which affects the passed in CancellationToken object)? Is that "explicit"? There are probably other examples we can think of.

[karelz]

.NET Framework also throws TaskCanceledException when you set HttpClient.Timeout.

[eocron]

If you can't distinguish between cancellation from client and cancellation from implementation - just check both. Before using client token create derived one:

var clientToken = ....
var innerTokenSource = CancellationTokenSource.CreateLinkedTokenSource(clientToken);

If at some point you catch OperationCancelledException:

try
{
     //invocation with innerToken
}
catch(OperationCancelledException oce)
{
      if(clientToken.IsCancellationRequested)
          throw; //as is, it is client initiated and in higher priority
      if(innerToken.IsCancellationRequested)
           //wrap it in HttpException, TimeoutException, whatever, wrap it in another linked token until you process all cases.
}

Basically you filter all possible layers from client token to deepest one, until you find token which caused cancellation.

[ericsampson]

Any updates on this making 2.2 or 3.0? @karelz @davidsh, cc @NickCraver . I kind of wish it would just do option 1) of throwing a TimeoutException, or alternatively a new HttpTimeoutException that is derived from HttpRequestException. Thanks!

[thomaslevesque]

Any updates on this making 2.2 or 3.0?

Too late for 2.2, it was released last week ;)

[ericsampson]

Too late for 2.2, it was released last week ;)

Argh, that's what happens when I take a break ;)

[Hobray]

Albeit a little goofy, the most simplistic workaround I've found is to nest a catch handler for OperationCanceledException and then evaluate the value of IsCancellationRequested to determine whether or not the cancellation token was the result of a timeout or an actual session abortion. Clearly the logic performing the API call would likely be in a business logic or service layer but I've consolidated the example for simplicity:

[HttpGet]
public async Task<IActionResult> Index(CancellationToken cancellationToken)
{
	try
	{
		//This would normally be done in a business logic or service layer rather than in the controller itself but I'm illustrating the point here for simplicity sake
		try
		{
			//Using HttpClientFactory for HttpClient instances      
			var httpClient = _httpClientFactory.CreateClient();

			//Set an absurd timeout to illustrate the point
			httpClient.Timeout = new TimeSpan(0, 0, 0, 0, 1);

			//Perform call that requires special timeout logic                
			var httpResponse = await httpClient.GetAsync("https://someurl.com/api/long/running");

			//... (if GetAsync doesn't fail, handle the response as desired)
		}
		catch (OperationCanceledException innerOperationCanceled)
		{
			//If a canceled token exception occurs due to a timeout, "IsCancellationRequested" should be false
			if (cancellationToken.IsCancellationRequested)
			{
				//Bubble exception to global handler
				throw innerOperationCanceled;
			}
			else
			{
				//... (perform timeout logic here)
			}                    
		}                

	}
	catch (OperationCanceledException operationCanceledEx)
	{
		_logger.LogWarning(operationCanceledEx, "Request was aborted by the end user.");
		return new StatusCodeResult(499);
	}
	catch (Exception ex)
	{
		_logger.LogError(ex, "An unexepcted error occured.");
		return new StatusCodeResult(500);
	}
}

I've seen other articles about how to handle this scenario more elegantly but they all require digging into the pipeline etc. I realize that this approach isn't perfect by my hope is that there will be better support for actual timeout exceptions with a future release.

[dotnetchris]

Working with http client is utterly abysmal. HttpClient needs deleted and started over from scratch.

[dotnetchris]

Albeit a little goofy, the most simplistic workaround I've found is to nest a catch handler for OperationCanceledException and then evaluate the value of IsCancellationRequested to determine whether or not the cancellation token was the result of a timeout or an actual session abortion. Clearly the logic performing the API call would likely be in a business logic or service layer but I've consolidated the example for simplicity:

[HttpGet]
public async Task<IActionResult> Index(CancellationToken cancellationToken)
{
	try
	{
		//This would normally be done in a business logic or service layer rather than in the controller itself but I'm illustrating the point here for simplicity sake
		try
		{
			//Using HttpClientFactory for HttpClient instances      
			var httpClient = _httpClientFactory.CreateClient();

			//Set an absurd timeout to illustrate the point
			httpClient.Timeout = new TimeSpan(0, 0, 0, 0, 1);

			//Perform call that requires special timeout logic                
			var httpResponse = await httpClient.GetAsync("https://someurl.com/api/long/running");

			//... (if GetAsync doesn't fail, handle the response as desired)
		}
		catch (OperationCanceledException innerOperationCanceled)
		{
			//If a canceled token exception occurs due to a timeout, "IsCancellationRequested" should be false
			if (cancellationToken.IsCancellationRequested)
			{
				//Bubble exception to global handler
				throw innerOperationCanceled;
			}
			else
			{
				//... (perform timeout logic here)
			}                    
		}                

	}
	catch (OperationCanceledException operationCanceledEx)
	{
		_logger.LogWarning(operationCanceledEx, "Request was aborted by the end user.");
		return new StatusCodeResult(499);
	}
	catch (Exception ex)
	{
		_logger.LogError(ex, "An unexepcted error occured.");
		return new StatusCodeResult(500);
	}
}

I've seen other articles about how to handle this scenario more elegantly but they all require digging into the pipeline etc. I realize that this approach isn't perfect by my hope is that there will be better support for actual timeout exceptions with a future release.

It's absolutely bonkers we need to write this atrocious code. HttpClient is the leakiest abstraction ever created by Microsoft

[Hobray]

I agree that it seems rather short sighted to not include more specific exception information around timeouts. Seems like a pretty basic thing for people to want to handle actual timeouts differently than cancelled token exceptions.

[ericsampson]

Working with http client is utterly abysmal. HttpClient needs deleted and started over from scratch.\

That's not very helpful or constructive feedback @dotnetchris . Customers and MSFT employees alike are all here to try and make things better, and that's why folks like @karelz are still here listening to customer feedback and trying to improve. There is no requirement for them to do development in the open, and let's not burn their good will with negativity or they could decide to take their ball and go home, and that would be worse for the community. Thanks! :)

[andre-ss6]

We agreed on that point @ericsampson but that would be breaking change for many existing users. As @scalablecory mentioned we compromised to make improvement while limiting compatibility damage.

Although I don't necessarily disagree with the wrapping option, I think it's worth pointing out that - as far as I've understood - the issue is not that the derived option is a breaking change, but that the wrapping one is simpler. As @stephentoub mentioned earlier in the discussion, throwing a derived exception is not considered a breaking change by the corefx guidelines.

[sksk571]

Deriving HttpTimeoutException from TaskCanceledException violates “is a” relationship between these two exceptions. It will still make calling code to think that cancellation happened, unless it specifically handles HttpTimeoutException. This is basically the same handling that we have to do at the moment when we check for cancellation token being canceled to determine whether it’s a timeout or not. Besides, having two timeout exceptions in core library is confusing.
Adding TimeoutException as an inner exception to TaskCanceledException will not provide any additional information in the most cases, as we can already check cancellation token and determine whether it’s timeout or not.

I think the only sensible solution is to change exception thrown on timeout to TimeoutException. Yes, it will be a breaking change, but the issue already spans several major releases and breaking changes is what major releases are for.

[qidydl]

Can the documentation for existing versions be updated to indicate that methods could throw TaskCanceledException and/or OperationCanceledException (both concrete types appear to be a possibility) when a timeout occurs? Currently the documentation indicates HttpRequestException deals with timeouts, which is incorrect and misleading.

[scalablecory]

@qidydl that's the plan... will fully document any ways timeouts can manifest and how to detail with them.

[ericsampson]

Thanks @alnikola and @scalablecory and @davidsh and @karelz and everyone else involved in this discussion/implementation, I really appreciate it.

[adydeshmukh]

Don't see a Timeout exception wrapped inside TaskCanceledException on PostAsync() call, it is returning under aggregate exception - .net version 4.5
System.AggregateException: One or more errors occurred. (A task was canceled.)
---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
at System.Threading.Tasks.Task1.GetResultCore(Boolean waitCompletionNotification) at System.Threading.Tasks.Task1.get_Result()

[wfurt]

This discussion his not relevant to .NET Framework @adydeshmukh. That is completely different implementation.