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

Confusing message on Http timeout #67505

Closed
danmoseley opened this issue Apr 3, 2022 · 4 comments · Fixed by #72274
Closed

Confusing message on Http timeout #67505

danmoseley opened this issue Apr 3, 2022 · 4 comments · Fixed by #72274
Assignees
Milestone

Comments

@danmoseley
Copy link
Member

From some test failures I noticed:

NuGet.Protocol.Core.Types.FatalProtocolException : An error occurred while retrieving package metadata for 'Microsoft.NETFramework.ReferenceAssemblies.net48.1.0.2' from source 'dotnet-public'.
---- System.Threading.Tasks.TaskCanceledException : The request was canceled due to the configured HttpClient.Timeout of -0.001 seconds elapsing.
-------- System.TimeoutException : The operation was canceled.
------------ System.Threading.Tasks.TaskCanceledException : The operation was canceled.
---------------- System.TimeoutException : A connection could not be established within the configured ConnectTimeout.

Apparently the timeout was set to Timeout.InfiniteTimeSpan, which is -1 ms. Presumably this couldn't have expired, so the message is confusing. I don't have a repro, so feel free to close if it's not important.

@ghost
Copy link

ghost commented Apr 3, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

From some test failures I noticed:

NuGet.Protocol.Core.Types.FatalProtocolException : An error occurred while retrieving package metadata for 'Microsoft.NETFramework.ReferenceAssemblies.net48.1.0.2' from source 'dotnet-public'.
---- System.Threading.Tasks.TaskCanceledException : The request was canceled due to the configured HttpClient.Timeout of -0.001 seconds elapsing.
-------- System.TimeoutException : The operation was canceled.
------------ System.Threading.Tasks.TaskCanceledException : The operation was canceled.
---------------- System.TimeoutException : A connection could not be established within the configured ConnectTimeout.

Apparently the timeout was set to Timeout.InfiniteTimeSpan, which is -1 ms. Presumably this couldn't have expired, so the message is confusing. I don't have a repro, so feel free to close if it's not important.

Author: danmoseley
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 3, 2022
@MihaZupan MihaZupan added the bug label Apr 3, 2022
@danmoseley
Copy link
Member Author

#67508

@MihaZupan
Copy link
Member

The relevant code path that determines the exception type:

// 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);

We should differentiate between a regular timeout and the ConnectTimeout.

Ideally, we would also expose such info in a programmatical way to the user. This recently came up in YARP as well (microsoft/reverse-proxy#1628).

@karelz
Copy link
Member

karelz commented Apr 7, 2022

Triage: It seems we are doing the right thing in the ConnectionPool, but HttpClient overrides it (and wraps with TImeoutException even if the timeout is not set) -- we should fix that.

@karelz karelz added this to the 7.0.0 milestone Apr 7, 2022
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Apr 7, 2022
@MihaZupan MihaZupan self-assigned this Jul 15, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 15, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants