Skip to content
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.

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

[WIP] Throw HttpRequestException from HttpClient instead of TaskCanceledException on Timeouts #40280

Open
wants to merge 2 commits into
base: master
from

Conversation

@thomaslevesque
Copy link
Collaborator

commented Aug 13, 2019

Fixes #20296

WIP because @karelz mentioned several possible options to solve the problem in #20296 (comment), and I don't know which one will be chosen. I implemented my favorite option, but I can change the code as requested.

@thomaslevesque
Copy link
Collaborator Author

left a comment

Added some comments on my implementation

@@ -479,19 +480,26 @@ public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, HttpCompl
CancellationTokenSource cts;
bool disposeCts;
bool hasTimeout = _timeout != s_infiniteTimeout;
CancellationTokenSource pendingRequestsCts = _pendingRequestsCts;

This comment has been minimized.

Copy link
@thomaslevesque

thomaslevesque Aug 13, 2019

Author Collaborator

Take a copy to ensure we're always using the same instance

e is OperationCanceledException
&& cts.IsCancellationRequested
&& !pendingRequestsCts.IsCancellationRequested
&& !cancellationToken.IsCancellationRequested;

This comment has been minimized.

Copy link
@thomaslevesque

thomaslevesque Aug 13, 2019

Author Collaborator

This is the ugly part. The check will actually be made in other methods (FinishSendAsyncUnbuffered or FinishSendAsyncBuffered), so I need to pass these methods what they need to check if the exception is a timeout. That means I have to pass either:

  • hasTimeout, pendingRequestsCts and cancellationToken: I dislike this approach because FinishSendAsyncUnbuffered and FinishSendAsyncBuffered aren't supposed to know the details of how we create the CTS and its relationship with _pendingRequestsCts ad cancellationToken
  • a delegate that encapsulate that logic (what I've done here). It's better, but it means more allocations (for the closure and for the delegate), which I'd rather avoid.

If requested, I can try another approach: creating a struct that encapsulates the logic and pass it to the FinishSendAsync* methods. This would avoid the allocation, but it would be a relatively big struct.

This comment has been minimized.

Copy link
@eiriktsarpalis

eiriktsarpalis Aug 21, 2019

Member

I dislike this approach because FinishSendAsyncUnbuffered and FinishSendAsyncBuffered aren't supposed to know the details of how we create the CTS and its relationship with _pendingRequestsCts ad cancellationToken

I see your point but this still concerns implementation detail, so I think we can tolerate a tiny bit of ugliness if it means saving a few redundant allocations and virtual calls. Can go a long way improving with careful choice of naming and comments explaining stuff.

@davidsh

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

We should never be throwing the TimeoutException as the top level exception.

HttpRequestException is the top level exception. A TimeoutException should be an inner exception .

@eiriktsarpalis

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I would recommend going for option #2, i.e. throwing TaskCanceledException with inner TimeoutException. We'd be breaking lots of retry/circuit breaker logics out there that expect TCE's on timeout.

@thomaslevesque

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

We should never be throwing the TimeoutException as the top level exception.

I saw your argument for this in the issue, but no confirmation that it was the way to go. I guess it would make sense, although it's also a breaking change.

The only way to avoid the breaking change would be to keep throwing a TaskCanceledException (possibly with a TimeoutException as the inner exception), but to be honest I don't really like this solution, as it doesn't really solve anything.

What I like about throwing TimeoutException is that it can be caught directly, without the need for an exception filter. But catching an HttpRequestException with a when (e.InnerException is TimeoutException) filter would work too.

So, what should I do?

@davidsh

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I would recommend going for option #2, i.e. throwing TaskCanceledException with inner TimeoutException. We'd be breaking lots of retry/circuit breaker logics out there that expect TCE's on timeout.

It's not obvious that TaskCanceledException would have inner exceptions that are interesting.

The principle design of HttpClient is to throw HttpRequestException as the top-level exception for runtime exceptions. Code is already written to assume that and to check for inner exceptions for more specifics.

But beyond the mechanics of this PR, I really want to see a detailed design of this change before a real PR.

What exactly is a "timeout"? Is is when the cancellation token expires? In the old days before async/await, a timeout was simply a network timeout. It was either socket related to TCP or perhaps a timeout waiting for HTTP response headers or response body. But now, a timeout is tied to async/await logic and cancellation token. A cancellation token can be cancelled via many ways which include calling .Cancel() on the CancellationTokenSource, calling .CancelAfter() with a TimeSpan or other ways. Are all these now becoming TimeoutException? What about when the .Timeout property of HttpClient is the one that "causes" the task to be internally cancelled since it "timed out". Is that also a TimeoutException?

@davidsh

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

We'd be breaking lots of retry/circuit breaker logics out there that expect TCE's on timeout.

And further commenting on this, we do have some cases where we already throw HttpRequestException with an inner exception of TimeoutException. Those cases occured prior to SocketsHttpHandler. We would get "timeouts" from WinHttpHandler (network timeouts) and those would throw TimeoutException from the handler. Then HttpClient wraps those as HttpRequestException with inner exception of TimeoutException. So, we already have that pattern from before.

@eiriktsarpalis

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

What exactly is a "timeout"?

It's clear that there are many different varieties of timeout. The client might even return a 504 response without throwing at all. But I'm assuming for the purposes of this conversation we're referring to the CancellationToken variety. In my experience it's been the most common type of timeout I've had to deal with in production. Wouldn't mind if we surfaced those as HttpRequestExceptions, but it would be a breaking change for existing systems.

@thomaslevesque

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

What exactly is a "timeout"? Is is when the cancellation token expires? In the old days before async/await, a timeout was simply a network timeout. It was either socket related to TCP or perhaps a timeout waiting for HTTP response headers or response body. But now, a timeout is tied to async/await logic and cancellation token. A cancellation token can be cancelled via many ways which include calling .Cancel() on the CancellationTokenSource, calling .CancelAfter() with a TimeSpan or other ways. Are all these now becoming TimeoutException? What about when the .Timeout property of HttpClient is the one that "causes" the task to be internally cancelled since it "timed out". Is that also a TimeoutException?

The fact that timeouts in HttpClient are based on cancellation is just an implementation detail. The user shouldn't have to know about it, so it makes sense to throw a TimeoutException (possibly wrapped in HttpRequestException) to hide the fact that it's using cancellation internally. Other cases of cancellation are not necessarily timeouts.

But I'm assuming for the purposes of this conversation we're referring to the CancellationToken variety. In my experience it's been the most common type of timeout I've had to deal with in production.

👍

@thomaslevesque

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

Personally, I don't mind the breaking change (admittedly, I'm not currently maintaining any legacy code, so it's easy to say for me...). I know it's something that .NET has historically avoided, but keeping backward compatibility forever hinders progress. The API would objectively be made better by throwing a more relevant exception, and that's what's important IMO. The breaking change would just have to be clearly documented.

BTW, the current documentation says that an HttpRequestException will be thrown for timeout. So the current behavior could be considered a bug, if the documentation is considered the source of truth. From that perspective, making the breaking change would technically be a bug fix 😉

@thomaslevesque

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

I pushed a new commit to wrap the TimeoutException in an HttpRequestException

@StephenCleary

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

TimeoutException should derive from OperationCanceledException. :trollface:

@eiriktsarpalis eiriktsarpalis self-assigned this Aug 21, 2019

e is OperationCanceledException
&& cts.IsCancellationRequested
&& !pendingRequestsCts.IsCancellationRequested
&& !cancellationToken.IsCancellationRequested;

This comment has been minimized.

Copy link
@eiriktsarpalis

eiriktsarpalis Aug 21, 2019

Member

I dislike this approach because FinishSendAsyncUnbuffered and FinishSendAsyncBuffered aren't supposed to know the details of how we create the CTS and its relationship with _pendingRequestsCts ad cancellationToken

I see your point but this still concerns implementation detail, so I think we can tolerate a tiny bit of ugliness if it means saving a few redundant allocations and virtual calls. Can go a long way improving with careful choice of naming and comments explaining stuff.

isTimeout = e =>
e is OperationCanceledException
&& cts.IsCancellationRequested
&& !pendingRequestsCts.IsCancellationRequested

This comment has been minimized.

Copy link
@eiriktsarpalis

eiriktsarpalis Aug 21, 2019

Member

I see that you're distinguishing between cancellations triggered by timeout and ambient cancellation tokens. While it might be the right choice, it's a design decision that needs to be stated explicitly and discussed.

@eiriktsarpalis eiriktsarpalis changed the title [WIP] Throw TimeoutException from HttpClient instead of TaskCanceledException [WIP] Throw HttpRequestException from HttpClient instead of TaskCanceledException on Timeouts Aug 21, 2019

{
return new HttpRequestException(
SR.net_http_timeout,
new TimeoutException(SR.net_http_timeout, innerException),

This comment has been minimized.

Copy link
@eiriktsarpalis

eiriktsarpalis Aug 21, 2019

Member

Wondering whether adding 3 layers of exceptions is necessary here. Could we possibly eliminate either the TimeoutException or the innermost TCO?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.