Remove lock when creating a SignatureProvider (#2788)

Co-authored-by: id4s <[email protected]>
AzureAD · Aug 21, 2024 · 1e23cef · 1e23cef
1 parent 7c7d1c1
commit 1e23cef
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 20 deletions.
diff --git a/build/version.props b/build/version.props
@@ -1,6 +1,6 @@
 <!-- This file may be overwritten by automation. Only values allowed here are VersionPrefix and VersionSuffix.  -->
 <Project>
-<!-- Wilson version  -->
+<!-- MicrosoftIdentityModelVersion  -->
   <PropertyGroup>
     <MicrosoftIdentityModelCurrentVersion>8.0.1</MicrosoftIdentityModelCurrentVersion>
 
@@ -14,4 +14,4 @@
     <FileVersion Condition="'$(MicrosoftIdentityModelVersion)' != '' and '$(IsCustomPreview)' != 'true' ">$(MicrosoftIdentityModelVersion).$([System.DateTime]::Now.AddYears(-2019).Year)$([System.DateTime]::Now.ToString("MMdd"))</FileVersion>
     <FileVersion Condition="'$(MicrosoftIdentityModelVersion)' == ''">$(MicrosoftIdentityModelCurrentVersion).$([System.DateTime]::Now.AddYears(-2019).Year)$([System.DateTime]::Now.ToString("MMdd"))</FileVersion>
   </PropertyGroup>
-</Project>
+</Project>
diff --git a/src/Microsoft.IdentityModel.Tokens/CryptoProviderFactory.cs b/src/Microsoft.IdentityModel.Tokens/CryptoProviderFactory.cs
@@ -16,7 +16,6 @@ public class CryptoProviderFactory
     {
         private static CryptoProviderFactory _default;
         private static readonly ConcurrentDictionary<string, string> _typeToAlgorithmMap = new ConcurrentDictionary<string, string>();
-        private static readonly object _cacheLock = new object();
         private static int _defaultSignatureProviderObjectPoolCacheSize = Environment.ProcessorCount * 4;
         private static string _typeofAsymmetricSignatureProvider = typeof(AsymmetricSignatureProvider).ToString();
         private static string _typeofSymmetricSignatureProvider = typeof(SymmetricSignatureProvider).ToString();
@@ -591,25 +590,23 @@ private SignatureProvider CreateSignatureProvider(SecurityKey key, string algori
                     return signatureProvider;
                 }
 
-                lock (_cacheLock)
-                {
-                    if (CryptoProviderCache.TryGetSignatureProvider(key, algorithm, typeofSignatureProvider, willCreateSignatures, out signatureProvider))
-                    {
-                        signatureProvider.AddRef();
-                        return signatureProvider;
-                    }
-
-                    if (!IsSupportedAlgorithm(algorithm, key))
-                        throw LogHelper.LogExceptionMessage(new NotSupportedException(LogHelper.FormatInvariant(LogMessages.IDX10634, LogHelper.MarkAsNonPII(algorithm), key)));
+                if (!IsSupportedAlgorithm(algorithm, key))
+                    throw LogHelper.LogExceptionMessage(new NotSupportedException(LogHelper.FormatInvariant(LogMessages.IDX10634, LogHelper.MarkAsNonPII(algorithm), key)));
 
-                    if (createAsymmetric)
-                        signatureProvider = new AsymmetricSignatureProvider(key, algorithm, willCreateSignatures, this);
-                    else
-                        signatureProvider = new SymmetricSignatureProvider(key, algorithm, willCreateSignatures);
+                if (createAsymmetric)
+                    signatureProvider = new AsymmetricSignatureProvider(key, algorithm, willCreateSignatures, this);
+                else
+                    signatureProvider = new SymmetricSignatureProvider(key, algorithm, willCreateSignatures);
 
-                    if (ShouldCacheSignatureProvider(signatureProvider))
-                        signatureProvider.IsCached = CryptoProviderCache.TryAdd(signatureProvider);
-                }
+                if (ShouldCacheSignatureProvider(signatureProvider))
+                    // CryptoProviderCache.TryAdd will return false if unable to add the SignatureProvider.
+                    // One possibility is the SignatureProvider was added between when we called TryGetSignatureProvider and here.
+                    // SignatureProvider.IsCached will be false and CryptoProviderFactory.Release will dispose the SignatureProvider.
+                    // Since the SignatueProvider, was never added to the cache, TryGetSignatureProvider will not return this instance, we can dispose.
+                    // This will result in sometimes (rarely) creating a SignatureProvider that is never cached.
+                    // The alternative is to use a lock after the call to TryGetSignatureProvider, and then check again: { TryGet, lock, TryGet }.
+                    // This will result in excesive locking for different keys, which is common in POP scenarios.
+                    signatureProvider.IsCached = CryptoProviderCache.TryAdd(signatureProvider);
             }
             else
             {