Skip to content

Commit 28fbe78

Browse files
authored
Connections: Move all AuthenticateAsClient to async (#2878)
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 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: ![image](https://github.com/user-attachments/assets/4b82c2b0-7f54-4720-ae4e-d540b5584d49) 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.
1 parent 328b4b5 commit 28fbe78

File tree

4 files changed

+9
-13
lines changed

4 files changed

+9
-13
lines changed

docs/ReleaseNotes.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Current package versions:
1010
No pending unreleased changes
1111

1212
- Add `ConfigurationOptions.SetUserPemCertificate(...)` and `ConfigurationOptions.SetUserPfxCertificate(...)` methods to simplify using client certificates ([#2873 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2873))
13+
- Fix: Move `AuthenticateAsClient` to fully async after dropping older framework support, to help client thread starvation in cases TLS negotiation stalls server-side ([#2878 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2878))
1314

1415
## 2.8.31
1516

src/StackExchange.Redis/Configuration/LoggingTunnel.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -367,10 +367,10 @@ private async Task<Stream> TlsHandshakeAsync(Stream stream, EndPoint endpoint)
367367
}
368368
else
369369
{
370-
ssl.AuthenticateAsClient(host, _options.SslProtocols, _options.CheckCertificateRevocation);
370+
await ssl.AuthenticateAsClientAsync(host, _options.SslProtocols, _options.CheckCertificateRevocation).ForAwait();
371371
}
372372
#else
373-
ssl.AuthenticateAsClient(host, _options.SslProtocols, _options.CheckCertificateRevocation);
373+
await ssl.AuthenticateAsClientAsync(host, _options.SslProtocols, _options.CheckCertificateRevocation).ForAwait();
374374
#endif
375375
return ssl;
376376
}

src/StackExchange.Redis/ExtensionMethods.cs

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using System.Security.Authentication;
88
using System.Security.Cryptography.X509Certificates;
99
using System.Text;
10+
using System.Threading.Tasks;
1011
using Pipelines.Sockets.Unofficial.Arenas;
1112

1213
namespace StackExchange.Redis
@@ -188,22 +189,16 @@ public static class ExtensionMethods
188189
return Array.ConvertAll(values, x => (string?)x);
189190
}
190191

191-
internal static void AuthenticateAsClient(this SslStream ssl, string host, SslProtocols? allowedProtocols, bool checkCertificateRevocation)
192+
internal static Task AuthenticateAsClientAsync(this SslStream ssl, string host, SslProtocols? allowedProtocols, bool checkCertificateRevocation)
192193
{
193194
if (!allowedProtocols.HasValue)
194195
{
195196
// Default to the sslProtocols defined by the .NET Framework
196-
AuthenticateAsClientUsingDefaultProtocols(ssl, host);
197-
return;
197+
return ssl.AuthenticateAsClientAsync(host);
198198
}
199199

200200
var certificateCollection = new X509CertificateCollection();
201-
ssl.AuthenticateAsClient(host, certificateCollection, allowedProtocols.Value, checkCertificateRevocation);
202-
}
203-
204-
private static void AuthenticateAsClientUsingDefaultProtocols(SslStream ssl, string host)
205-
{
206-
ssl.AuthenticateAsClient(host);
201+
return ssl.AuthenticateAsClientAsync(host, certificateCollection, allowedProtocols.Value, checkCertificateRevocation);
207202
}
208203

209204
/// <summary>

src/StackExchange.Redis/PhysicalConnection.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1585,10 +1585,10 @@ internal async ValueTask<bool> ConnectedAsync(Socket? socket, ILogger? log, Sock
15851585
}
15861586
else
15871587
{
1588-
ssl.AuthenticateAsClient(host, config.SslProtocols, config.CheckCertificateRevocation);
1588+
await ssl.AuthenticateAsClientAsync(host, config.SslProtocols, config.CheckCertificateRevocation).ForAwait();
15891589
}
15901590
#else
1591-
ssl.AuthenticateAsClient(host, config.SslProtocols, config.CheckCertificateRevocation);
1591+
await ssl.AuthenticateAsClientAsync(host, config.SslProtocols, config.CheckCertificateRevocation).ForAwait();
15921592
#endif
15931593
}
15941594
catch (Exception ex)

0 commit comments

Comments
 (0)