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

Revert "Set SocketsHttpHandler's default connect timeout to 15s (#66607)" #68649

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

CarnaViire
Copy link
Member

This reverts commit 436b97c.

The change proved to be problematic as-is, because

  1. There is a high chance that the user code has a retry logic that does retry when SocketException is caught but does not retry in case of OperationCanceledException. This can lead to a spike of (unretried) exceptions which weren't there before. Example:
    https://github.com/NuGet/NuGet.Client/blob/93bb3921490e9d499be1fb48d6cb9dcaff734db1/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRetryHandler.cs#L212-L263

  2. The workaround with setting the ConnectTimeout back to infinity may not be available -- if HttpClientHandler is used instead of SocketsHttpHandler (e.g. due to reusing code between .NET Framework and .NET Core/.NET 5+), or if the code that executes the requests is hidden inside a library.

I will reopen #66297 for further considerations.

@ghost
Copy link

ghost commented Apr 28, 2022

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

Issue Details

This reverts commit 436b97c.

The change proved to be problematic as-is, because

  1. There is a high chance that the user code has a retry logic that does retry when SocketException is caught but does not retry in case of OperationCanceledException. This can lead to a spike of (unretried) exceptions which weren't there before. Example:
    https://github.com/NuGet/NuGet.Client/blob/93bb3921490e9d499be1fb48d6cb9dcaff734db1/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRetryHandler.cs#L212-L263

  2. The workaround with setting the ConnectTimeout back to infinity may not be available -- if HttpClientHandler is used instead of SocketsHttpHandler (e.g. due to reusing code between .NET Framework and .NET Core/.NET 5+), or if the code that executes the requests is hidden inside a library.

I will reopen #66297 for further considerations.

Author: CarnaViire
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@stephentoub stephentoub merged commit d193abb into dotnet:main Apr 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants