Connections: Move all AuthenticateAsClient to async #2878
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously when supporting earlier versions of .NET Framework, we did not have an async version of `AuthenticateAsClient` due to socket itself not having async handlers in earlier versions of `netstandard`. These days though, this can be fully async to prevent thread starvation specifically in the case where we're able to connect a socket but the server is overwhelmed and unable to finish TLS negotiations. Note that none of these take a cancellation token, so while we can avoid thread growth on the client side as a primary symptom of the case here, the server-side impact of backlogged but unconnected things may continue to grow. It's net better overall, though, and should help a bit on mass connection cases for some applications as well, in the cases TLS negotiation isn't instant.
philon-msft
approved these changes
May 3, 2025
mgravell
approved these changes
May 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously when supporting earlier versions of .NET Framework, we did not have an async version of
AuthenticateAsClientdue to socket itself not having async handlers in earlier versions ofnetstandard. These days though with the supported target framework matrix, this can be fully async to prevent thread starvation specifically in the case where we're able to connect a socket but the server is overwhelmed and unable to finish TLS negotiations.Example:
This is a case we've never seen stall before, where the server is able to accept connections but unable to complete TLS negotiation, so there's never been a reported stall raising this potential issue. Last night we saw it live on a major instance though. It's still TBD how this was manifesting server-side (could be a proxy stall in front of nodes, etc.) but we saw this against Redis Enterprise which we know a lot of folks use, so let's definitely improve anything we can as always.
Note that none of these methods take a cancellation token, so while we can avoid thread growth on the client side as a primary symptom of the case here, the server-side impact of backlogged but unconnected things may continue to grow. It's net better overall, though, and should help a bit on mass connection cases for some applications as well, in the cases TLS negotiation isn't instant.