From 8b817958ba3754349021d0e278a68040713df641 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 6 Mar 2024 19:09:50 +0100 Subject: [PATCH 01/11] Implement MsQuicConfiguration cache --- .../Internal/MsQuicConfiguration.Cache.cs | 182 ++++++++++++++++++ .../Net/Quic/Internal/MsQuicConfiguration.cs | 32 ++- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 4 +- .../src/System/Net/Quic/QuicConnection.cs | 2 +- 4 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs new file mode 100644 index 00000000000000..377fed0b0895d2 --- /dev/null +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -0,0 +1,182 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Collections.Concurrent; +using System.Collections.ObjectModel; +using System.Security.Authentication; +using System.Net.Security; +using System.Security.Cryptography.X509Certificates; +using System.Threading; +using Microsoft.Quic; + +namespace System.Net.Quic; + +internal static partial class MsQuicConfiguration +{ + private static readonly ConcurrentDictionary s_configurationCache = new(); + + private readonly struct CacheKey : IEquatable + { + public readonly List CertificateThumbprints; + public readonly QUIC_CREDENTIAL_FLAGS Flags; + public readonly QUIC_SETTINGS Settings; + public readonly List ApplicationProtocols; + + public CacheKey(List certificateThumbprints, QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, List applicationProtocols) + { + CertificateThumbprints = certificateThumbprints; + Flags = flags; + Settings = settings; + ApplicationProtocols = applicationProtocols; + } + + public override bool Equals(object? obj) => obj is CacheKey key && Equals(key); + + public bool Equals(CacheKey other) + { + if (CertificateThumbprints.Count != other.CertificateThumbprints.Count) + { + return false; + } + + for (int i = 0; i < CertificateThumbprints.Count; i++) + { + if (!CertificateThumbprints[i].AsSpan().SequenceEqual(other.CertificateThumbprints[i])) + { + return false; + } + } + + if (ApplicationProtocols.Count != other.ApplicationProtocols.Count) + { + return false; + } + + for (int i = 0; i < ApplicationProtocols.Count; i++) + { + if (ApplicationProtocols[i] != other.ApplicationProtocols[i]) + { + return false; + } + } + + return + Flags == other.Flags && + Settings.Equals(other.Settings); + } + + public override int GetHashCode() + { + HashCode hash = default; + + foreach (var thumbprint in CertificateThumbprints) + { + hash.Add(thumbprint); + } + + hash.Add(Flags); + hash.Add(Settings); + + foreach (var protocol in ApplicationProtocols) + { + hash.Add(protocol); + } + + return hash.ToHashCode(); + } + } + + private static MsQuicSafeHandle? TryGetCachedConfigurationHandle(CacheKey key) + { + if (s_configurationCache.TryGetValue(key, out MsQuicSafeHandle? handle)) + { + try + { + // + // This races with potential cache cleanup, which may close the + // handle before we claim ownership. + // + bool ignore = false; + handle.DangerousAddRef(ref ignore); + return handle; + } + catch (ObjectDisposedException) + { + // we lost the race, behave as if the handle was not in the cache in the first place + } + } + + return null; + } + + private static void CacheConfigurationHandle(CacheKey key, MsQuicSafeHandle handle) + { + s_configurationCache.AddOrUpdate( + key, + (k, h) => + { + // add-ref the handle to signify that it is statically cached + bool ignore = false; + h.DangerousAddRef(ref ignore); + return h; + }, + (k, existing, h) => + { + // there already is an existing handle for this key, make this a no-op + return existing; + }, + handle); + + if (s_configurationCache.Count % 32 == 0) + { + // let only one thread perform cleanup at a time + lock (s_configurationCache) + { + if (s_configurationCache.Count % 32 == 0) + { + CleanupCachedCredentials(); + } + } + } + } + + private static void CleanupCachedCredentials() + { + KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); + + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}"); + + foreach (KeyValuePair kvp in toRemoveAttempt) + { + var handle = kvp.Value; + + // + // Unfortunately, we can't get the current refcount of the handle, + // so we try to release and see if the handle closes. + // + handle.DangerousRelease(); + bool inUse = false; + try + { + if (!handle.IsClosed) + { + // This add-ref races with QuicConnection.Dispose(); + handle.DangerousAddRef(ref inUse); + } + } + catch (ObjectDisposedException) + { + // we lost the race, the handle is closed, we can proceed to remove it from + // the cache. + } + + if (!inUse) + { + s_configurationCache.TryRemove(kvp.Key, out _); + } + } + + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}"); + } +} diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index 7e527e41bf9564..d87dfa2759611e 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -11,7 +11,7 @@ namespace System.Net.Quic; -internal static class MsQuicConfiguration +internal static partial class MsQuicConfiguration { private static bool HasPrivateKey(this X509Certificate certificate) => certificate is X509Certificate2 certificate2 && certificate2.Handle != IntPtr.Zero && certificate2.HasPrivateKey; @@ -119,6 +119,11 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) { + if (cipherSuitesPolicy != null) + { + flags |= QUIC_CREDENTIAL_FLAGS.SET_ALLOWED_CIPHER_SUITES; + } + // Validate options and SSL parameters. if (alpnProtocols is null || alpnProtocols.Count <= 0) { @@ -176,6 +181,26 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI : 0; // 0 disables the timeout } + CacheKey cacheKey = new CacheKey( + certificate == null ? new List() : new List { certificate.GetCertHash() }, + flags, + settings, + alpnProtocols); // TODO: defensive copy + + if (intermediates != null) + { + foreach (X509Certificate2 intermediate in intermediates) + { + cacheKey.CertificateThumbprints.Add(intermediate.GetCertHash()); + } + } + + MsQuicSafeHandle? configurationHandle = TryGetCachedConfigurationHandle(cacheKey); + if (configurationHandle != null) + { + return configurationHandle; + } + QUIC_HANDLE* handle; using MsQuicBuffers msquicBuffers = new MsQuicBuffers(); @@ -189,7 +214,7 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI (void*)IntPtr.Zero, &handle), "ConfigurationOpen failed"); - MsQuicSafeHandle configurationHandle = new MsQuicSafeHandle(handle, SafeHandleType.Configuration); + configurationHandle = new MsQuicSafeHandle(handle, SafeHandleType.Configuration); try { @@ -198,7 +223,6 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI if (cipherSuitesPolicy != null) { - config.Flags |= QUIC_CREDENTIAL_FLAGS.SET_ALLOWED_CIPHER_SUITES; config.AllowedCipherSuites = CipherSuitePolicyToFlags(cipherSuitesPolicy); } @@ -273,6 +297,8 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI throw; } + CacheConfigurationHandle(cacheKey, configurationHandle); + return configurationHandle; } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index 38a099ed9e49f3..e14a16579c4ada 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -52,7 +52,8 @@ public MsQuicSafeHandle(QUIC_HANDLE* handle, SafeHandleType safeHandleType) SafeHandleType.Stream => MsQuicApi.Api.ApiTable->StreamClose, _ => throw new ArgumentException($"Unexpected value: {safeHandleType}", nameof(safeHandleType)) }, - safeHandleType) { } + safeHandleType) + { } protected override bool ReleaseHandle() { @@ -65,6 +66,7 @@ protected override bool ReleaseHandle() NetEventSource.Info(this, $"{this} MsQuicSafeHandle released"); } + GC.SuppressFinalize(this); return true; } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 5a4f626e2f5465..2c728dac96e3b1 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -677,7 +677,7 @@ public async ValueTask DisposeAsync() _handle.Dispose(); _shutdownTokenSource.Dispose(); - _configuration?.Dispose(); + _configuration?.DangerousRelease(); // Dispose remote certificate only if it hasn't been accessed via getter, in which case the accessing code becomes the owner of the certificate lifetime. if (!_remoteCertificateExposed) From 6f94f382b885134b03d93abc8707a64b3cd879c3 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 7 Mar 2024 12:19:03 +0100 Subject: [PATCH 02/11] Fix creds with custom cipher suites --- .../Net/Quic/Internal/MsQuicConfiguration.Cache.cs | 12 +++++++++--- .../System/Net/Quic/Internal/MsQuicConfiguration.cs | 8 ++++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 377fed0b0895d2..54a950e6c9a4b6 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -22,13 +22,15 @@ internal static partial class MsQuicConfiguration public readonly QUIC_CREDENTIAL_FLAGS Flags; public readonly QUIC_SETTINGS Settings; public readonly List ApplicationProtocols; + public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - public CacheKey(List certificateThumbprints, QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, List applicationProtocols) + public CacheKey(List certificateThumbprints, QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, List applicationProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { CertificateThumbprints = certificateThumbprints; Flags = flags; Settings = settings; ApplicationProtocols = applicationProtocols; + AllowedCipherSuites = allowedCipherSuites; } public override bool Equals(object? obj) => obj is CacheKey key && Equals(key); @@ -63,7 +65,8 @@ public bool Equals(CacheKey other) return Flags == other.Flags && - Settings.Equals(other.Settings); + Settings.Equals(other.Settings) && + AllowedCipherSuites == other.AllowedCipherSuites; } public override int GetHashCode() @@ -83,6 +86,8 @@ public override int GetHashCode() hash.Add(protocol); } + hash.Add(AllowedCipherSuites); + return hash.ToHashCode(); } } @@ -123,7 +128,8 @@ private static void CacheConfigurationHandle(CacheKey key, MsQuicSafeHandle hand }, (k, existing, h) => { - // there already is an existing handle for this key, make this a no-op + // there already is an existing handle for this key, perhaps we lost a race, + // make this a no-op return existing; }, handle); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index d87dfa2759611e..c53aeff860dc3a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -119,9 +119,12 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) { + QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites = QUIC_ALLOWED_CIPHER_SUITE_FLAGS.NONE; + if (cipherSuitesPolicy != null) { flags |= QUIC_CREDENTIAL_FLAGS.SET_ALLOWED_CIPHER_SUITES; + allowedCipherSuites = CipherSuitePolicyToFlags(cipherSuitesPolicy); } // Validate options and SSL parameters. @@ -185,7 +188,8 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI certificate == null ? new List() : new List { certificate.GetCertHash() }, flags, settings, - alpnProtocols); // TODO: defensive copy + alpnProtocols, + allowedCipherSuites); // TODO: defensive copy if (intermediates != null) { @@ -223,7 +227,7 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI if (cipherSuitesPolicy != null) { - config.AllowedCipherSuites = CipherSuitePolicyToFlags(cipherSuitesPolicy); + config.AllowedCipherSuites = allowedCipherSuites; } int status; From 34c314d86e87bfba33100b9be87e2e0b8c3654f2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 7 Mar 2024 13:00:36 +0100 Subject: [PATCH 03/11] Polishing --- .../Internal/MsQuicConfiguration.Cache.cs | 14 ++++++++--- .../Net/Quic/Internal/MsQuicConfiguration.cs | 24 +++++++++---------- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 4 +--- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 54a950e6c9a4b6..f1543080c213af 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -14,6 +14,8 @@ namespace System.Net.Quic; internal static partial class MsQuicConfiguration { + private const int CheckExpiredModulo = 32; + private static readonly ConcurrentDictionary s_configurationCache = new(); private readonly struct CacheKey : IEquatable @@ -104,6 +106,7 @@ public override int GetHashCode() // bool ignore = false; handle.DangerousAddRef(ref ignore); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Using cached MsQuicConfiguration {key}"); return handle; } catch (ObjectDisposedException) @@ -124,6 +127,7 @@ private static void CacheConfigurationHandle(CacheKey key, MsQuicSafeHandle hand // add-ref the handle to signify that it is statically cached bool ignore = false; h.DangerousAddRef(ref ignore); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching MsQuicConfiguration {key}"); return h; }, (k, existing, h) => @@ -134,12 +138,12 @@ private static void CacheConfigurationHandle(CacheKey key, MsQuicSafeHandle hand }, handle); - if (s_configurationCache.Count % 32 == 0) + if (s_configurationCache.Count % CheckExpiredModulo == 0) { // let only one thread perform cleanup at a time lock (s_configurationCache) { - if (s_configurationCache.Count % 32 == 0) + if (s_configurationCache.Count % CheckExpiredModulo == 0) { CleanupCachedCredentials(); } @@ -158,7 +162,7 @@ private static void CleanupCachedCredentials() var handle = kvp.Value; // - // Unfortunately, we can't get the current refcount of the handle, + // Unfortunately, we can't directly get the current refcount of the handle, // so we try to release and see if the handle closes. // handle.DangerousRelease(); @@ -179,7 +183,11 @@ private static void CleanupCachedCredentials() if (!inUse) { + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}"); s_configurationCache.TryRemove(kvp.Key, out _); + // The handle is closed, but we did not call Dispose on it. Doing so would throw ODE, + // suppress finalization to prevent Dispose from being called in Finalizer threads. + GC.SuppressFinalize(handle); } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index c53aeff860dc3a..34cb524d4537ac 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -119,14 +119,6 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) { - QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites = QUIC_ALLOWED_CIPHER_SUITE_FLAGS.NONE; - - if (cipherSuitesPolicy != null) - { - flags |= QUIC_CREDENTIAL_FLAGS.SET_ALLOWED_CIPHER_SUITES; - allowedCipherSuites = CipherSuitePolicyToFlags(cipherSuitesPolicy); - } - // Validate options and SSL parameters. if (alpnProtocols is null || alpnProtocols.Count <= 0) { @@ -184,12 +176,20 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI : 0; // 0 disables the timeout } + QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites = QUIC_ALLOWED_CIPHER_SUITE_FLAGS.NONE; + + if (cipherSuitesPolicy != null) + { + flags |= QUIC_CREDENTIAL_FLAGS.SET_ALLOWED_CIPHER_SUITES; + allowedCipherSuites = CipherSuitePolicyToFlags(cipherSuitesPolicy); + } + CacheKey cacheKey = new CacheKey( certificate == null ? new List() : new List { certificate.GetCertHash() }, flags, settings, - alpnProtocols, - allowedCipherSuites); // TODO: defensive copy + new List(alpnProtocols), // make defensive copy to prevent modification + allowedCipherSuites); if (intermediates != null) { @@ -208,11 +208,11 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI QUIC_HANDLE* handle; using MsQuicBuffers msquicBuffers = new MsQuicBuffers(); - msquicBuffers.Initialize(alpnProtocols, alpnProtocol => alpnProtocol.Protocol); + msquicBuffers.Initialize(cacheKey.ApplicationProtocols, alpnProtocol => alpnProtocol.Protocol); ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ConfigurationOpen( MsQuicApi.Api.Registration, msquicBuffers.Buffers, - (uint)alpnProtocols.Count, + (uint)msquicBuffers.Count, &settings, (uint)sizeof(QUIC_SETTINGS), (void*)IntPtr.Zero, diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index e14a16579c4ada..38a099ed9e49f3 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -52,8 +52,7 @@ public MsQuicSafeHandle(QUIC_HANDLE* handle, SafeHandleType safeHandleType) SafeHandleType.Stream => MsQuicApi.Api.ApiTable->StreamClose, _ => throw new ArgumentException($"Unexpected value: {safeHandleType}", nameof(safeHandleType)) }, - safeHandleType) - { } + safeHandleType) { } protected override bool ReleaseHandle() { @@ -66,7 +65,6 @@ protected override bool ReleaseHandle() NetEventSource.Info(this, $"{this} MsQuicSafeHandle released"); } - GC.SuppressFinalize(this); return true; } From 6825c3a08dfc26d37463bbd98d8f97c5730df44e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 7 Mar 2024 15:10:20 +0100 Subject: [PATCH 04/11] Dispose discarded handle when racing to add into cache --- .../Internal/MsQuicConfiguration.Cache.cs | 75 +++++++++++++------ .../Net/Quic/Internal/MsQuicConfiguration.cs | 2 +- .../src/System/Net/Quic/QuicConnection.cs | 1 + 3 files changed, 54 insertions(+), 24 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index f1543080c213af..f080d8b051a656 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -77,7 +77,7 @@ public override int GetHashCode() foreach (var thumbprint in CertificateThumbprints) { - hash.Add(thumbprint); + hash.AddBytes(thumbprint); } hash.Add(Flags); @@ -85,7 +85,7 @@ public override int GetHashCode() foreach (var protocol in ApplicationProtocols) { - hash.Add(protocol); + hash.AddBytes(protocol.Protocol.Span); } hash.Add(AllowedCipherSuites); @@ -101,43 +101,69 @@ public override int GetHashCode() try { // - // This races with potential cache cleanup, which may close the - // handle before we claim ownership. + // This races with a potential cache cleanup, which may close the + // handle before we claim it. // bool ignore = false; handle.DangerousAddRef(ref ignore); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Using cached MsQuicConfiguration {key}"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Using cached MsQuicConfiguration {handle}."); return handle; } catch (ObjectDisposedException) { - // we lost the race, behave as if the handle was not in the cache in the first place + // we lost the race, behave as if the handle was not in the cache. } } return null; } - private static void CacheConfigurationHandle(CacheKey key, MsQuicSafeHandle handle) + private static void CacheConfigurationHandle(CacheKey key, ref MsQuicSafeHandle handle) { - s_configurationCache.AddOrUpdate( + var cached = s_configurationCache.AddOrUpdate( key, - (k, h) => + (_, newHandle) => { - // add-ref the handle to signify that it is statically cached + // The cache now holds the ownership of the handle. bool ignore = false; - h.DangerousAddRef(ref ignore); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching MsQuicConfiguration {key}"); - return h; + newHandle.DangerousAddRef(ref ignore); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching MsQuicConfiguration {newHandle}."); + return newHandle; }, - (k, existing, h) => + (_, existingHandle, newHandle) => { - // there already is an existing handle for this key, perhaps we lost a race, - // make this a no-op - return existing; + // another thread was faster in creating the configuration, check if we can + // use the cached one + bool ignore = false; + try + { + // + // This also races with the cache cleanup but should be rare since the + // configuration was just added to the cache and is likely still being used. + // + existingHandle.DangerousAddRef(ref ignore); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Found existing MsQuicConfiguration {existingHandle} in cache."); + return existingHandle; + } + catch (ObjectDisposedException) + { + // we lost the race with cleanup, the existing configuration handle is closed, + // keep the one we created. + newHandle.DangerousAddRef(ref ignore); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching MsQuicConfiguration {newHandle}."); + return newHandle; + } }, handle); + if (cached != handle) + { + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); + handle.Dispose(); + handle = cached; + return; + } + if (s_configurationCache.Count % CheckExpiredModulo == 0) { // let only one thread perform cleanup at a time @@ -155,22 +181,25 @@ private static void CleanupCachedCredentials() { KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}."); foreach (KeyValuePair kvp in toRemoveAttempt) { var handle = kvp.Value; // - // Unfortunately, we can't directly get the current refcount of the handle, - // so we try to release and see if the handle closes. + // We can't directly get the current refcount of the handle, we know it's at least 1, + // so we decrement it and if it does not close, then it must be in use, so we increment + // it back. // + handle.DangerousRelease(); bool inUse = false; try { if (!handle.IsClosed) { + // handle is in use, add the ref back. // This add-ref races with QuicConnection.Dispose(); handle.DangerousAddRef(ref inUse); } @@ -183,14 +212,14 @@ private static void CleanupCachedCredentials() if (!inUse) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); s_configurationCache.TryRemove(kvp.Key, out _); // The handle is closed, but we did not call Dispose on it. Doing so would throw ODE, - // suppress finalization to prevent Dispose from being called in Finalizer threads. + // suppress finalization to prevent Dispose from being called in a Finalizer thread. GC.SuppressFinalize(handle); } } - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}."); } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index 34cb524d4537ac..f21225f421663b 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -301,7 +301,7 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI throw; } - CacheConfigurationHandle(cacheKey, configurationHandle); + CacheConfigurationHandle(cacheKey, ref configurationHandle); return configurationHandle; } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 2c728dac96e3b1..ce512091f61522 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -677,6 +677,7 @@ public async ValueTask DisposeAsync() _handle.Dispose(); _shutdownTokenSource.Dispose(); + // don't dispose the handle, just release refcount because it may be cached _configuration?.DangerousRelease(); // Dispose remote certificate only if it hasn't been accessed via getter, in which case the accessing code becomes the owner of the certificate lifetime. From b5689a5f6c00d0225e8d3f1eccfb9057e48f1f74 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 11 Mar 2024 13:26:20 +0100 Subject: [PATCH 05/11] Shuffle code around, add AppCtx switch for disabling --- .../Internal/MsQuicConfiguration.Cache.cs | 60 +++++++++++++++++-- .../Net/Quic/Internal/MsQuicConfiguration.cs | 42 +++++-------- 2 files changed, 71 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index f080d8b051a656..562372f188c29e 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -16,6 +16,35 @@ internal static partial class MsQuicConfiguration { private const int CheckExpiredModulo = 32; + private const string DisableCacheEnvironmentVariable = "DOTNET_SYSTEM_NET_QUIC_DISABLE_CONFIGURATION_CACHE"; + private const string DisableCacheCtxSwitch = "System.Net.Quic.DisableConfigurationCache"; + + private static volatile int s_configurationCacheEnabled = -1; + private static bool ConfigurationCacheEnabled + { + get + { + int enabled = s_configurationCacheEnabled; + if (enabled != -1) + { + return enabled != 0; + } + + // AppContext switch takes precedence + if (AppContext.TryGetSwitch(DisableCacheCtxSwitch, out bool value)) + { + s_configurationCacheEnabled = value ? 0 : 1; + } + else + { + // check environment variable + s_configurationCacheEnabled = Environment.GetEnvironmentVariable(DisableCacheEnvironmentVariable) is string envVar && (envVar == "1" || envVar.Equals("true", StringComparison.OrdinalIgnoreCase)) ? 0 : 1; + } + + return s_configurationCacheEnabled != 0; + } + } + private static readonly ConcurrentDictionary s_configurationCache = new(); private readonly struct CacheKey : IEquatable @@ -26,12 +55,22 @@ internal static partial class MsQuicConfiguration public readonly List ApplicationProtocols; public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - public CacheKey(List certificateThumbprints, QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, List applicationProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + public CacheKey(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { - CertificateThumbprints = certificateThumbprints; + CertificateThumbprints = certificate == null ? new List() : new List { certificate.GetCertHash() }; + + if (intermediates != null) + { + foreach (X509Certificate2 intermediate in intermediates) + { + CertificateThumbprints.Add(intermediate.GetCertHash()); + } + } + Flags = flags; Settings = settings; - ApplicationProtocols = applicationProtocols; + // make defensive copy to prevent modification (the list comes from user code) + ApplicationProtocols = new List(alpnProtocols); AllowedCipherSuites = allowedCipherSuites; } @@ -94,8 +133,16 @@ public override int GetHashCode() } } - private static MsQuicSafeHandle? TryGetCachedConfigurationHandle(CacheKey key) + private static MsQuicSafeHandle GetCachedCredentialOrCreate(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { + CacheKey key = new CacheKey( + flags, + settings, + certificate, + intermediates, + alpnProtocols, + allowedCipherSuites); + if (s_configurationCache.TryGetValue(key, out MsQuicSafeHandle? handle)) { try @@ -115,7 +162,10 @@ public override int GetHashCode() } } - return null; + handle = CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + CacheConfigurationHandle(key, handle); + + return handle; } private static void CacheConfigurationHandle(CacheKey key, ref MsQuicSafeHandle handle) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index f21225f421663b..b3d8f82a9bd2b3 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -117,7 +117,7 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin return Create(options, flags, certificate, intermediates, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy); } - private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) + private static MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) { // Validate options and SSL parameters. if (alpnProtocols is null || alpnProtocols.Count <= 0) @@ -184,31 +184,25 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI allowedCipherSuites = CipherSuitePolicyToFlags(cipherSuitesPolicy); } - CacheKey cacheKey = new CacheKey( - certificate == null ? new List() : new List { certificate.GetCertHash() }, - flags, - settings, - new List(alpnProtocols), // make defensive copy to prevent modification - allowedCipherSuites); - - if (intermediates != null) + if (!MsQuicApi.UsesSChannelBackend) { - foreach (X509Certificate2 intermediate in intermediates) - { - cacheKey.CertificateThumbprints.Add(intermediate.GetCertHash()); - } + flags |= QUIC_CREDENTIAL_FLAGS.USE_PORTABLE_CERTIFICATES; } - MsQuicSafeHandle? configurationHandle = TryGetCachedConfigurationHandle(cacheKey); - if (configurationHandle != null) + if (ConfigurationCacheEnabled) { - return configurationHandle; + return GetCachedCredentialOrCreate(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); } + return CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + } + + private static unsafe MsQuicSafeHandle CreateInternal(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + { QUIC_HANDLE* handle; using MsQuicBuffers msquicBuffers = new MsQuicBuffers(); - msquicBuffers.Initialize(cacheKey.ApplicationProtocols, alpnProtocol => alpnProtocol.Protocol); + msquicBuffers.Initialize(alpnProtocols, alpnProtocol => alpnProtocol.Protocol); ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ConfigurationOpen( MsQuicApi.Api.Registration, msquicBuffers.Buffers, @@ -218,17 +212,15 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI (void*)IntPtr.Zero, &handle), "ConfigurationOpen failed"); - configurationHandle = new MsQuicSafeHandle(handle, SafeHandleType.Configuration); + MsQuicSafeHandle configurationHandle = new MsQuicSafeHandle(handle, SafeHandleType.Configuration); try { - QUIC_CREDENTIAL_CONFIG config = new QUIC_CREDENTIAL_CONFIG { Flags = flags }; - config.Flags |= (MsQuicApi.UsesSChannelBackend ? QUIC_CREDENTIAL_FLAGS.NONE : QUIC_CREDENTIAL_FLAGS.USE_PORTABLE_CERTIFICATES); - - if (cipherSuitesPolicy != null) + QUIC_CREDENTIAL_CONFIG config = new QUIC_CREDENTIAL_CONFIG { - config.AllowedCipherSuites = allowedCipherSuites; - } + Flags = flags, + AllowedCipherSuites = allowedCipherSuites + }; int status; if (certificate is null) @@ -301,8 +293,6 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI throw; } - CacheConfigurationHandle(cacheKey, ref configurationHandle); - return configurationHandle; } From 40c4bc23fd9361e6a5fffdd37cda0bf5d55e6ca9 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 11 Mar 2024 15:12:44 +0100 Subject: [PATCH 06/11] Code review feedback --- .../Internal/MsQuicConfiguration.Cache.cs | 136 +++++------------- .../Net/Quic/Internal/MsQuicConfiguration.cs | 10 +- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 77 +++++++++- .../src/System/Net/Quic/QuicConnection.cs | 4 +- 4 files changed, 116 insertions(+), 111 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 562372f188c29e..7eee854e414823 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Collections.Generic; using System.Collections.Concurrent; using System.Collections.ObjectModel; @@ -20,7 +21,7 @@ internal static partial class MsQuicConfiguration private const string DisableCacheCtxSwitch = "System.Net.Quic.DisableConfigurationCache"; private static volatile int s_configurationCacheEnabled = -1; - private static bool ConfigurationCacheEnabled + internal static bool ConfigurationCacheEnabled { get { @@ -45,7 +46,7 @@ private static bool ConfigurationCacheEnabled } } - private static readonly ConcurrentDictionary s_configurationCache = new(); + private static readonly ConcurrentDictionary s_configurationCache = new(); private readonly struct CacheKey : IEquatable { @@ -133,87 +134,38 @@ public override int GetHashCode() } } - private static MsQuicSafeHandle GetCachedCredentialOrCreate(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + private static SafeMsQuicConfigurationHandle GetCachedCredentialOrCreate(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { - CacheKey key = new CacheKey( - flags, - settings, - certificate, - intermediates, - alpnProtocols, - allowedCipherSuites); - - if (s_configurationCache.TryGetValue(key, out MsQuicSafeHandle? handle)) + CacheKey key = new CacheKey(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + + SafeMsQuicConfigurationHandle? handle; + + if (s_configurationCache.TryGetValue(key, out handle) && handle.TryAddRentCount()) { - try - { - // - // This races with a potential cache cleanup, which may close the - // handle before we claim it. - // - bool ignore = false; - handle.DangerousAddRef(ref ignore); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Using cached MsQuicConfiguration {handle}."); - return handle; - } - catch (ObjectDisposedException) - { - // we lost the race, behave as if the handle was not in the cache. - } + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Found cached MsQuicConfiguration: {handle}."); + return handle; } - handle = CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); - CacheConfigurationHandle(key, handle); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"MsQuicConfiguration not found in cache, creating new."); - return handle; - } + handle = CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); - private static void CacheConfigurationHandle(CacheKey key, ref MsQuicSafeHandle handle) - { - var cached = s_configurationCache.AddOrUpdate( - key, - (_, newHandle) => - { - // The cache now holds the ownership of the handle. - bool ignore = false; - newHandle.DangerousAddRef(ref ignore); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching MsQuicConfiguration {newHandle}."); - return newHandle; - }, - (_, existingHandle, newHandle) => - { - // another thread was faster in creating the configuration, check if we can - // use the cached one - bool ignore = false; - try - { - // - // This also races with the cache cleanup but should be rare since the - // configuration was just added to the cache and is likely still being used. - // - existingHandle.DangerousAddRef(ref ignore); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Found existing MsQuicConfiguration {existingHandle} in cache."); - return existingHandle; - } - catch (ObjectDisposedException) - { - // we lost the race with cleanup, the existing configuration handle is closed, - // keep the one we created. - newHandle.DangerousAddRef(ref ignore); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching MsQuicConfiguration {newHandle}."); - return newHandle; - } - }, - handle); + SafeMsQuicConfigurationHandle cached; + do + { + cached = s_configurationCache.GetOrAdd(key, handle); + } + while (!cached.TryAddRentCount()); if (cached != handle) { + // we lost a race with another thread to insert new handle into the cache if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); - handle.Dispose(); + handle.ReleaseHandleFromCache(); handle = cached; - return; } + // we added a new handle, check if we need to cleanup if (s_configurationCache.Count % CheckExpiredModulo == 0) { // let only one thread perform cleanup at a time @@ -225,49 +177,29 @@ private static void CacheConfigurationHandle(CacheKey key, ref MsQuicSafeHandle } } } + + return handle; } private static void CleanupCachedCredentials() { - KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); + KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}."); - foreach (KeyValuePair kvp in toRemoveAttempt) + foreach ((CacheKey key, SafeMsQuicConfigurationHandle handle) in toRemoveAttempt) { - var handle = kvp.Value; - - // - // We can't directly get the current refcount of the handle, we know it's at least 1, - // so we decrement it and if it does not close, then it must be in use, so we increment - // it back. - // - - handle.DangerousRelease(); - bool inUse = false; - try + if (!handle.TryMarkForDispose()) { - if (!handle.IsClosed) - { - // handle is in use, add the ref back. - // This add-ref races with QuicConnection.Dispose(); - handle.DangerousAddRef(ref inUse); - } - } - catch (ObjectDisposedException) - { - // we lost the race, the handle is closed, we can proceed to remove it from - // the cache. + // handle in use + continue; } - if (!inUse) - { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); - s_configurationCache.TryRemove(kvp.Key, out _); - // The handle is closed, but we did not call Dispose on it. Doing so would throw ODE, - // suppress finalization to prevent Dispose from being called in a Finalizer thread. - GC.SuppressFinalize(handle); - } + // the handle is not in use and has been marked such that no new rents can be added. + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); + bool removed = s_configurationCache.TryRemove(key, out _); + Debug.Assert(removed, "Handle was marked for disposal, but was not found in the cache."); + handle.ReleaseHandleFromCache(); } if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}."); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index b3d8f82a9bd2b3..ad10b71541ef69 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -16,7 +16,7 @@ internal static partial class MsQuicConfiguration private static bool HasPrivateKey(this X509Certificate certificate) => certificate is X509Certificate2 certificate2 && certificate2.Handle != IntPtr.Zero && certificate2.HasPrivateKey; - public static MsQuicSafeHandle Create(QuicClientConnectionOptions options) + public static SafeMsQuicConfigurationHandle Create(QuicClientConnectionOptions options) { SslClientAuthenticationOptions authenticationOptions = options.ClientAuthenticationOptions; @@ -79,7 +79,7 @@ public static MsQuicSafeHandle Create(QuicClientConnectionOptions options) return Create(options, flags, certificate, intermediates, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy); } - public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, string? targetHost) + public static SafeMsQuicConfigurationHandle Create(QuicServerConnectionOptions options, string? targetHost) { SslServerAuthenticationOptions authenticationOptions = options.ServerAuthenticationOptions; @@ -117,7 +117,7 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin return Create(options, flags, certificate, intermediates, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy); } - private static MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) + private static SafeMsQuicConfigurationHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) { // Validate options and SSL parameters. if (alpnProtocols is null || alpnProtocols.Count <= 0) @@ -197,7 +197,7 @@ private static MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDE return CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); } - private static unsafe MsQuicSafeHandle CreateInternal(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + private static unsafe SafeMsQuicConfigurationHandle CreateInternal(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { QUIC_HANDLE* handle; @@ -212,7 +212,7 @@ private static unsafe MsQuicSafeHandle CreateInternal(QUIC_CREDENTIAL_FLAGS flag (void*)IntPtr.Zero, &handle), "ConfigurationOpen failed"); - MsQuicSafeHandle configurationHandle = new MsQuicSafeHandle(handle, SafeHandleType.Configuration); + SafeMsQuicConfigurationHandle configurationHandle = new SafeMsQuicConfigurationHandle(handle); try { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index 38a099ed9e49f3..a02650e02af238 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Runtime.InteropServices; +using System.Threading; using Microsoft.Quic; namespace System.Net.Quic; @@ -52,7 +53,8 @@ public MsQuicSafeHandle(QUIC_HANDLE* handle, SafeHandleType safeHandleType) SafeHandleType.Stream => MsQuicApi.Api.ApiTable->StreamClose, _ => throw new ArgumentException($"Unexpected value: {safeHandleType}", nameof(safeHandleType)) }, - safeHandleType) { } + safeHandleType) + { } protected override bool ReleaseHandle() { @@ -142,3 +144,76 @@ protected override unsafe bool ReleaseHandle() return true; } } + +internal sealed class SafeMsQuicConfigurationHandle : MsQuicSafeHandle +{ + // MsQuicConfiguration handles are cached, so we need to keep track of the + // number of times a handle is rented. Once we decide to dispose the handle, + // we set the _rentCount to -1. + private volatile int _rentCount; + + public unsafe SafeMsQuicConfigurationHandle(QUIC_HANDLE* handle) + : base(handle, SafeHandleType.Configuration) { } + + public bool TryAddRentCount() + { + int oldCount; + + do + { + oldCount = _rentCount; + if (oldCount < 0) + { + // The handle is already disposed. + return false; + } + } while (Interlocked.CompareExchange(ref _rentCount, oldCount + 1, oldCount) != oldCount); + + return true; + } + + public bool TryMarkForDispose() + { + int oldCount; + + do + { + oldCount = _rentCount; + if (oldCount > 0) + { + // The handle is being used + return false; + } + } while (Interlocked.CompareExchange(ref _rentCount, -1, oldCount) != oldCount); + + return true; + } + + public void ReleaseHandleFromCache() + { + Debug.Assert(_rentCount == -1); + + // calls SafeHandle.Dispose + base.Dispose(true); + GC.SuppressFinalize(this); + + // There should not be more outstanding refs to the native handle. + Debug.Assert(IsClosed); + } + + protected override void Dispose(bool disposing) + { + if (MsQuicConfiguration.ConfigurationCacheEnabled) + { + // only decrement the rent count + Debug.Assert(_rentCount > 0); + Interlocked.Decrement(ref _rentCount); + } + else + { + // dispose as usual + Debug.Assert(_rentCount == 0); + base.Dispose(disposing); + } + } +} diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index ce512091f61522..0846543a6aee66 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -676,9 +676,7 @@ public async ValueTask DisposeAsync() Debug.Assert(_connectedTcs.IsCompleted); _handle.Dispose(); _shutdownTokenSource.Dispose(); - - // don't dispose the handle, just release refcount because it may be cached - _configuration?.DangerousRelease(); + _configuration?.Dispose(); // Dispose remote certificate only if it hasn't been accessed via getter, in which case the accessing code becomes the owner of the certificate lifetime. if (!_remoteCertificateExposed) From 82965d24b5cbb10763e3363cdf88ae8644ad616f Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 11 Mar 2024 15:37:13 +0100 Subject: [PATCH 07/11] Add comments, minor fixes --- .../Internal/MsQuicConfiguration.Cache.cs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 7eee854e414823..17bff54af5bf31 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -146,23 +146,42 @@ private static SafeMsQuicConfigurationHandle GetCachedCredentialOrCreate(QUIC_CR return handle; } + // + // if we get here, the handle is either not in the cache, or we lost the race between + // TryAddRentCount on this thread and MarkForDispose on another thread doing cache cleanup. + // In either case, we need to create a new handle. + // + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"MsQuicConfiguration not found in cache, creating new."); handle = CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + handle.TryAddRentCount(); // we are the first renter SafeMsQuicConfigurationHandle cached; do { cached = s_configurationCache.GetOrAdd(key, handle); } - while (!cached.TryAddRentCount()); + // + // If we get the same handle back, we successfully added it to the cache and we are done. + // If we get a different handle back, we need to increase the rent count. + // If we fail to add the rent count, then the existing/cached handle is in process of + // being removed from the and we can try again, eventually either succeeding to add our + // new handle or getting a fresh handle inserted by another thread meanwhile. + // + while (cached != handle && !cached.TryAddRentCount()); if (cached != handle) { // we lost a race with another thread to insert new handle into the cache if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); + + // Decrement the rent count we added before attempting the cache insertion + // and close the handle + handle.Dispose(); handle.ReleaseHandleFromCache(); - handle = cached; + + return cached; } // we added a new handle, check if we need to cleanup From 0578632be3f4b9a2331ff7d7551123087d4dfd81 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 11 Mar 2024 16:33:38 +0100 Subject: [PATCH 08/11] Fix failing test on Windows --- .../Internal/MsQuicConfiguration.Cache.cs | 10 +++--- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 35 +++++++------------ 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 17bff54af5bf31..5390913ef2e4eb 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -176,10 +176,11 @@ private static SafeMsQuicConfigurationHandle GetCachedCredentialOrCreate(QUIC_CR // we lost a race with another thread to insert new handle into the cache if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); - // Decrement the rent count we added before attempting the cache insertion - // and close the handle + // First dispose decrements the rent count we added before attempting the cache insertion + // and second closes the handle handle.Dispose(); - handle.ReleaseHandleFromCache(); + handle.Dispose(); + Debug.Assert(handle.IsClosed, "Handle should be closed."); return cached; } @@ -218,7 +219,8 @@ private static void CleanupCachedCredentials() if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); bool removed = s_configurationCache.TryRemove(key, out _); Debug.Assert(removed, "Handle was marked for disposal, but was not found in the cache."); - handle.ReleaseHandleFromCache(); + handle.Dispose(); + Debug.Assert(handle.IsClosed, "Handle should be closed."); } if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}."); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index a02650e02af238..6f84c4ccee838a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -189,31 +189,20 @@ public bool TryMarkForDispose() return true; } - public void ReleaseHandleFromCache() - { - Debug.Assert(_rentCount == -1); - - // calls SafeHandle.Dispose - base.Dispose(true); - GC.SuppressFinalize(this); - - // There should not be more outstanding refs to the native handle. - Debug.Assert(IsClosed); - } - protected override void Dispose(bool disposing) { - if (MsQuicConfiguration.ConfigurationCacheEnabled) - { - // only decrement the rent count - Debug.Assert(_rentCount > 0); - Interlocked.Decrement(ref _rentCount); - } - else + int oldCount; + do { - // dispose as usual - Debug.Assert(_rentCount == 0); - base.Dispose(disposing); - } + oldCount = _rentCount; + if (oldCount <= 0) + { + // _rentCount is 0 if the handle was never rented (e.g. failure during creation), + // and is -1 when evicted from cache. + base.Dispose(disposing); + return; + } + + } while (Interlocked.CompareExchange(ref _rentCount, oldCount - 1, oldCount) != oldCount); } } From 8e26582978ee191d4e539c2c9da7f0f0db82c447 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 15 Mar 2024 13:25:22 +0100 Subject: [PATCH 09/11] Code review feedback --- .../Internal/MsQuicConfiguration.Cache.cs | 64 +++++++++++-------- .../Net/Quic/Internal/MsQuicConfiguration.cs | 14 ++-- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 4 +- 3 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 5390913ef2e4eb..945bf1ea804838 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -46,7 +46,7 @@ internal static bool ConfigurationCacheEnabled } } - private static readonly ConcurrentDictionary s_configurationCache = new(); + private static readonly ConcurrentDictionary s_configurationCache = new(); private readonly struct CacheKey : IEquatable { @@ -56,7 +56,7 @@ internal static bool ConfigurationCacheEnabled public readonly List ApplicationProtocols; public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - public CacheKey(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { CertificateThumbprints = certificate == null ? new List() : new List { certificate.GetCertHash() }; @@ -134,53 +134,58 @@ public override int GetHashCode() } } - private static SafeMsQuicConfigurationHandle GetCachedCredentialOrCreate(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + private static MsQuicConfigurationSafeHandle GetCachedCredentialOrCreate(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { - CacheKey key = new CacheKey(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + CacheKey key = new CacheKey(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); - SafeMsQuicConfigurationHandle? handle; + MsQuicConfigurationSafeHandle? handle; if (s_configurationCache.TryGetValue(key, out handle) && handle.TryAddRentCount()) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Found cached MsQuicConfiguration: {handle}."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(null, $"Found cached MsQuicConfiguration: {handle}."); + } return handle; } - // // if we get here, the handle is either not in the cache, or we lost the race between // TryAddRentCount on this thread and MarkForDispose on another thread doing cache cleanup. // In either case, we need to create a new handle. - // - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"MsQuicConfiguration not found in cache, creating new."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(null, $"MsQuicConfiguration not found in cache, creating new."); + } - handle = CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + handle = CreateInternal(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); handle.TryAddRentCount(); // we are the first renter - SafeMsQuicConfigurationHandle cached; + MsQuicConfigurationSafeHandle cached; do { cached = s_configurationCache.GetOrAdd(key, handle); } - // // If we get the same handle back, we successfully added it to the cache and we are done. // If we get a different handle back, we need to increase the rent count. // If we fail to add the rent count, then the existing/cached handle is in process of - // being removed from the and we can try again, eventually either succeeding to add our + // being removed from the cache and we can try again, eventually either succeeding to add our // new handle or getting a fresh handle inserted by another thread meanwhile. - // while (cached != handle && !cached.TryAddRentCount()); if (cached != handle) { // we lost a race with another thread to insert new handle into the cache - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); + } // First dispose decrements the rent count we added before attempting the cache insertion // and second closes the handle handle.Dispose(); handle.Dispose(); - Debug.Assert(handle.IsClosed, "Handle should be closed."); + Debug.Assert(handle.IsClosed); return cached; } @@ -193,7 +198,7 @@ private static SafeMsQuicConfigurationHandle GetCachedCredentialOrCreate(QUIC_CR { if (s_configurationCache.Count % CheckExpiredModulo == 0) { - CleanupCachedCredentials(); + CleanupCache(); } } } @@ -201,13 +206,16 @@ private static SafeMsQuicConfigurationHandle GetCachedCredentialOrCreate(QUIC_CR return handle; } - private static void CleanupCachedCredentials() + private static void CleanupCache() { - KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); + KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}."); + } - foreach ((CacheKey key, SafeMsQuicConfigurationHandle handle) in toRemoveAttempt) + foreach ((CacheKey key, MsQuicConfigurationSafeHandle handle) in toRemoveAttempt) { if (!handle.TryMarkForDispose()) { @@ -216,13 +224,19 @@ private static void CleanupCachedCredentials() } // the handle is not in use and has been marked such that no new rents can be added. - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); + } bool removed = s_configurationCache.TryRemove(key, out _); - Debug.Assert(removed, "Handle was marked for disposal, but was not found in the cache."); + Debug.Assert(removed); handle.Dispose(); - Debug.Assert(handle.IsClosed, "Handle should be closed."); + Debug.Assert(handle.IsClosed); } - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}."); + } } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs index ad10b71541ef69..e99f1a68ae9ec5 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs @@ -16,7 +16,7 @@ internal static partial class MsQuicConfiguration private static bool HasPrivateKey(this X509Certificate certificate) => certificate is X509Certificate2 certificate2 && certificate2.Handle != IntPtr.Zero && certificate2.HasPrivateKey; - public static SafeMsQuicConfigurationHandle Create(QuicClientConnectionOptions options) + public static MsQuicConfigurationSafeHandle Create(QuicClientConnectionOptions options) { SslClientAuthenticationOptions authenticationOptions = options.ClientAuthenticationOptions; @@ -79,7 +79,7 @@ public static SafeMsQuicConfigurationHandle Create(QuicClientConnectionOptions o return Create(options, flags, certificate, intermediates, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy); } - public static SafeMsQuicConfigurationHandle Create(QuicServerConnectionOptions options, string? targetHost) + public static MsQuicConfigurationSafeHandle Create(QuicServerConnectionOptions options, string? targetHost) { SslServerAuthenticationOptions authenticationOptions = options.ServerAuthenticationOptions; @@ -117,7 +117,7 @@ public static SafeMsQuicConfigurationHandle Create(QuicServerConnectionOptions o return Create(options, flags, certificate, intermediates, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy); } - private static SafeMsQuicConfigurationHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) + private static MsQuicConfigurationSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy) { // Validate options and SSL parameters. if (alpnProtocols is null || alpnProtocols.Count <= 0) @@ -191,13 +191,13 @@ private static SafeMsQuicConfigurationHandle Create(QuicConnectionOptions option if (ConfigurationCacheEnabled) { - return GetCachedCredentialOrCreate(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + return GetCachedCredentialOrCreate(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); } - return CreateInternal(flags, settings, certificate, intermediates, alpnProtocols, allowedCipherSuites); + return CreateInternal(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); } - private static unsafe SafeMsQuicConfigurationHandle CreateInternal(QUIC_CREDENTIAL_FLAGS flags, QUIC_SETTINGS settings, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) + private static unsafe MsQuicConfigurationSafeHandle CreateInternal(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { QUIC_HANDLE* handle; @@ -212,7 +212,7 @@ private static unsafe SafeMsQuicConfigurationHandle CreateInternal(QUIC_CREDENTI (void*)IntPtr.Zero, &handle), "ConfigurationOpen failed"); - SafeMsQuicConfigurationHandle configurationHandle = new SafeMsQuicConfigurationHandle(handle); + MsQuicConfigurationSafeHandle configurationHandle = new MsQuicConfigurationSafeHandle(handle); try { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index 6f84c4ccee838a..aad4fb1efac024 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -145,14 +145,14 @@ protected override unsafe bool ReleaseHandle() } } -internal sealed class SafeMsQuicConfigurationHandle : MsQuicSafeHandle +internal sealed class MsQuicConfigurationSafeHandle : MsQuicSafeHandle { // MsQuicConfiguration handles are cached, so we need to keep track of the // number of times a handle is rented. Once we decide to dispose the handle, // we set the _rentCount to -1. private volatile int _rentCount; - public unsafe SafeMsQuicConfigurationHandle(QUIC_HANDLE* handle) + public unsafe MsQuicConfigurationSafeHandle(QUIC_HANDLE* handle) : base(handle, SafeHandleType.Configuration) { } public bool TryAddRentCount() From c1ddffb0e1d23d0b4353e1e19bb0d236dc812dcf Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Mon, 18 Mar 2024 10:37:11 +0100 Subject: [PATCH 10/11] Apply suggestions from code review Co-authored-by: Miha Zupan --- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 31 ++++--------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index aad4fb1efac024..cf7d70a18e08d1 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -174,35 +174,16 @@ public bool TryAddRentCount() public bool TryMarkForDispose() { - int oldCount; - - do - { - oldCount = _rentCount; - if (oldCount > 0) - { - // The handle is being used - return false; - } - } while (Interlocked.CompareExchange(ref _rentCount, -1, oldCount) != oldCount); - - return true; + return Interlocked.CompareExchange(ref _rentCount, -1, 0) == 0; } protected override void Dispose(bool disposing) { - int oldCount; - do + if (Interlocked.Decrement(ref _rentCount) < 0) { - oldCount = _rentCount; - if (oldCount <= 0) - { - // _rentCount is 0 if the handle was never rented (e.g. failure during creation), - // and is -1 when evicted from cache. - base.Dispose(disposing); - return; - } - - } while (Interlocked.CompareExchange(ref _rentCount, oldCount - 1, oldCount) != oldCount); + // _rentCount is 0 if the handle was never rented (e.g. failure during creation), + // and is -1 when evicted from cache. + base.Dispose(disposing); + } } } From ebab536d626ccf4e19d8ddae72b7889f0982dea1 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 18 Mar 2024 11:13:20 +0100 Subject: [PATCH 11/11] Code review changes --- .../Internal/MsQuicConfiguration.Cache.cs | 47 ++++++++----------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 945bf1ea804838..4fc86adfbd1565 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -20,29 +20,20 @@ internal static partial class MsQuicConfiguration private const string DisableCacheEnvironmentVariable = "DOTNET_SYSTEM_NET_QUIC_DISABLE_CONFIGURATION_CACHE"; private const string DisableCacheCtxSwitch = "System.Net.Quic.DisableConfigurationCache"; - private static volatile int s_configurationCacheEnabled = -1; - internal static bool ConfigurationCacheEnabled + internal static bool ConfigurationCacheEnabled { get; } = GetConfigurationCacheEnabled(); + private static bool GetConfigurationCacheEnabled() { - get + // AppContext switch takes precedence + if (AppContext.TryGetSwitch(DisableCacheCtxSwitch, out bool value)) { - int enabled = s_configurationCacheEnabled; - if (enabled != -1) - { - return enabled != 0; - } - - // AppContext switch takes precedence - if (AppContext.TryGetSwitch(DisableCacheCtxSwitch, out bool value)) - { - s_configurationCacheEnabled = value ? 0 : 1; - } - else - { - // check environment variable - s_configurationCacheEnabled = Environment.GetEnvironmentVariable(DisableCacheEnvironmentVariable) is string envVar && (envVar == "1" || envVar.Equals("true", StringComparison.OrdinalIgnoreCase)) ? 0 : 1; - } - - return s_configurationCacheEnabled != 0; + return !value; + } + else + { + // check environment variable + return + Environment.GetEnvironmentVariable(DisableCacheEnvironmentVariable) is string envVar && + !(envVar == "1" || envVar.Equals("true", StringComparison.OrdinalIgnoreCase)); } } @@ -191,12 +182,15 @@ private static MsQuicConfigurationSafeHandle GetCachedCredentialOrCreate(QUIC_SE } // we added a new handle, check if we need to cleanup - if (s_configurationCache.Count % CheckExpiredModulo == 0) + var count = s_configurationCache.Count; + if (count % CheckExpiredModulo == 0) { // let only one thread perform cleanup at a time lock (s_configurationCache) { - if (s_configurationCache.Count % CheckExpiredModulo == 0) + // check again, if another thread just cleaned up (and cached count went down) we are unlikely + // to clean anything + if (s_configurationCache.Count >= count) { CleanupCache(); } @@ -208,14 +202,12 @@ private static MsQuicConfigurationSafeHandle GetCachedCredentialOrCreate(QUIC_SE private static void CleanupCache() { - KeyValuePair[] toRemoveAttempt = s_configurationCache.ToArray(); - if (NetEventSource.Log.IsEnabled()) { - NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {toRemoveAttempt.Length}."); + NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {s_configurationCache.Count}."); } - foreach ((CacheKey key, MsQuicConfigurationSafeHandle handle) in toRemoveAttempt) + foreach ((CacheKey key, MsQuicConfigurationSafeHandle handle) in s_configurationCache) { if (!handle.TryMarkForDispose()) { @@ -228,6 +220,7 @@ private static void CleanupCache() { NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); } + bool removed = s_configurationCache.TryRemove(key, out _); Debug.Assert(removed); handle.Dispose();