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

Set SocketsHttpHandler.ConnectTimeout to 15 seconds by default #1833

Closed
wants to merge 1 commit into from

Conversation

mahpate
Copy link

@mahpate mahpate commented Aug 17, 2022

Fixes #1678

Setting SocketsHttpHandler.ConnectTimeout to 15 seconds as default value in .Net 7.
See dotnet/runtime#66673 (comment)

@Tratcher
Copy link
Member

@miha, the change to SocketsHttpHandler's ConnectTimeout default was reverted, correct?

@MihaZupan
Copy link
Member

Yes, that change was reverted. The default ConnectTimeout stayed the same (infinite).

We went a different route in runtime with dotnet/runtime#71785. A connection attempt will now be canceled 5 seconds after the request that started it is completed/canceled.
In practice, this should have a similar effect to ConnectTimeout, but without impacting the exception type surfaced to requests (a stalled connection attempt won't show up as a ConnectTimeout by default).

The issue mentioned in the linked comment about wasting connection attempts on canceled requests is also no longer a problem as we backported mitigations for that to 6.0 (dotnet/runtime#66990).


That said, the reason the change to 15 seconds was reverted is not that the change is bad in itself.
We reverted it because users were expecting a different kind of exception (from the Sockets layer) and started getting ConnectTimeout exceptions instead. This broke code that was doing retries based on the kind of exception.

YARP is in a different situation here given that we are the ones calling into HttpClient to make requests. While someone could have implemented retry logic in a similar way, it's much less likely.

I would be okay with changing the default to 15s in YARP, but I don't think we should be linking to that comment as justification. The reason for doing so is not to avoid "connection leaks", but to avoid potential memory leaks/higher request failure rates.

@MihaZupan
Copy link
Member

MihaZupan commented Aug 17, 2022

For reference, this is the issue tracking making such changes in YARP: #1678

@MihaZupan MihaZupan changed the title Bug fix for connection leak in SocketsHttpHandler Set SocketsHttpHandler.ConnectTimeout to 15 seconds by default Aug 17, 2022
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

@mahpate can you please also update this property in other places where we specify these defaults:

@@ -48,7 +48,7 @@ public HttpMessageInvoker CreateClient(ForwarderHttpClientContext context)
AutomaticDecompression = DecompressionMethods.None,
UseCookies = false,
ActivityHeadersPropagator = new ReverseProxyPropagator(DistributedContextPropagator.Current),

ConnectTimeout = TimeSpan.FromSeconds(15), // See: https://github.com/dotnet/runtime/issues/66673#issuecomment-1073811798
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ConnectTimeout = TimeSpan.FromSeconds(15), // See: https://github.com/dotnet/runtime/issues/66673#issuecomment-1073811798
ConnectTimeout = TimeSpan.FromSeconds(15),

@MihaZupan MihaZupan added this to the YARP 2.0.0 milestone Aug 19, 2022
@MihaZupan
Copy link
Member

@mahpate are you still interested in making this change?

@MihaZupan
Copy link
Member

MihaZupan commented Sep 8, 2022

@mahpate are you able to apply the recommended fixes here?

Feel free to reopen the PR if you wish to continue working on this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider setting a finite ConnectTimeout by default
3 participants