Fix lock contention

src/Amazon.SecretsManager.Extensions.Caching/ISecretsManagerCache.cs

@@ -25,10 +25,13 @@ public interface ISecretsManagerCache : IDisposable
     {
 
         /// <summary>
-        /// Returns the cache entry corresponding to the specified secret if it exists in the cache.
+        /// Asynchronously returns the cache entry corresponding to the specified secret if it exists in the cache.
         /// Otherwise, the secret value is fetched from Secrets Manager and a new cache entry is created.
         /// </summary>
-        SecretCacheItem GetCachedSecret(string secretId);
+        /// <param name="secretId">The secret identifier (ARN or friendly name).</param>
+        /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous operation. The task result contains the <see cref="SecretCacheItem"/> for the specified secret.</returns>
+        Task<SecretCacheItem> GetCachedSecret(string secretId, CancellationToken cancellationToken = default);
 
         /// <summary>
         /// Asynchronously retrieves the specified SecretBinary after calling <see cref="GetCachedSecret"/>.

src/Amazon.SecretsManager.Extensions.Caching/SecretCacheItem.cs

@@ -25,11 +25,12 @@
     /// </summary>
     public class SecretCacheItem : SecretCacheObject<DescribeSecretResponse>
     {
-        /// The cached secret value versions for this cached secret. 
+        /// The cached secret value versions for this cached secret.
         private readonly MemoryCache versions = new MemoryCache(new MemoryCacheOptions());
+        private readonly SemaphoreSlim versionsLock = new SemaphoreSlim(1, 1);
         private const ushort MAX_VERSIONS_CACHE_SIZE = 10;
 
         public SecretCacheItem(String secretId, IAmazonSecretsManager client, SecretCacheConfiguration config)
             : base(secretId, client, config)
         {
         }
@@ -48,7 +49,7 @@
         /// </summary>
         protected override async Task<GetSecretValueResponse> GetSecretValueAsync(DescribeSecretResponse result, CancellationToken cancellationToken = default)
         {
-            SecretCacheVersion version = GetVersion(result);
+            SecretCacheVersion version = await GetVersion(result, cancellationToken);
             if (version == null)
             {
                 return null;
@@ -56,12 +57,12 @@
             return await version.GetSecretValue(cancellationToken);
         }
 
         public override int GetHashCode()
         {
             return (secretId ?? string.Empty).GetHashCode();
         }
 
         public override string ToString()
         {
             return $"SecretCacheItem: {secretId}";
         }
@@ -72,10 +73,13 @@
         }
 
         /// <summary>
-        /// Retrieves the SecretCacheVersion corresponding to the Version Stage
-        /// specified by the SecretCacheConfiguration.
+        /// Asynchronously retrieves the <see cref="SecretCacheVersion"/> corresponding to the version stage
+        /// specified by the <see cref="SecretCacheConfiguration"/>.
         /// </summary>
-        private SecretCacheVersion GetVersion(DescribeSecretResponse describeResult)
+        /// <param name="describeResult">The describe secret response containing version-to-stage mappings.</param>
+        /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous operation. The task result contains the matching <see cref="SecretCacheVersion"/>, or <c>null</c> if no version matches the configured stage.</returns>
+        private async Task<SecretCacheVersion> GetVersion(DescribeSecretResponse describeResult, CancellationToken cancellationToken = default)
         {
             if (null == describeResult?.VersionIdsToStages) return null;
             String currentVersionId = null;
@@ -90,13 +94,25 @@
             if (currentVersionId != null)
             {
                 SecretCacheVersion version = versions.Get<SecretCacheVersion>(currentVersionId);
-                if (null == version)
+                if (version == null)
                 {
-                    version = versions.Set(currentVersionId, new SecretCacheVersion(secretId, currentVersionId, client, config));
-                    if (versions.Count > MAX_VERSIONS_CACHE_SIZE)
+                    await this.versionsLock.WaitAsync(cancellationToken);
+                    try
                     {
-                        TrimCacheToSizeLimit();
+                        version = versions.GetOrCreate<SecretCacheVersion>(currentVersionId, entry =>
+                        {
+                            return new SecretCacheVersion(secretId, currentVersionId, client, config);
+                        });
+
+                        if (versions.Count > MAX_VERSIONS_CACHE_SIZE)
+                        {
+                            TrimCacheToSizeLimit();
+                        }
                     }
+                    finally
+                    {
+                        this.versionsLock.Release();
+                    } 
                 }
                 return version;
             }
@@ -105,7 +121,7 @@
 
         private void TrimCacheToSizeLimit()
         {
-            versions.Compact((double)(versions.Count - config.MaxCacheSize) / versions.Count);
+            versions.Compact((double)(versions.Count - MAX_VERSIONS_CACHE_SIZE) / versions.Count);
         }
     }
-}
+}

src/Amazon.SecretsManager.Extensions.Caching/SecretCacheObject.cs

@@ -25,12 +25,12 @@ public abstract class SecretCacheObject<T>
         private readonly JitteredDelay exceptionJitteredDelay;
         private readonly JitteredDelay forceRefreshJitteredDelay;
 
+        /// A private object to synchronize access to certain methods. 
+        private readonly SemaphoreSlim objLock;
+
         /// The secret identifier for this cached object. 
         protected String secretId;
 
-        /// A private object to synchronize access to certain methods. 
-        protected static readonly SemaphoreSlim Lock = new SemaphoreSlim(1,1);
-
         /// The AWS Secrets Manager client to use for requesting secrets. 
         protected IAmazonSecretsManager client;
 
@@ -57,8 +57,6 @@ public abstract class SecretCacheObject<T>
         /// The time to wait before retrying a failed AWS Secrets Manager request.
         private DateTime nextRetryTime = DateTime.MinValue;
 
-        public static readonly ThreadLocal<Random> random = new ThreadLocal<Random>(() => new Random(Environment.TickCount));
-
 
 
         /// <summary>
@@ -79,6 +77,7 @@ public SecretCacheObject(String secretId, IAmazonSecretsManager client, SecretCa
             this.forceRefreshJitteredDelay = new JitteredDelay(
                 config.ForceRefreshDelayBase,
                 config.ForceRefreshDelayJitter);
+            this.objLock = new SemaphoreSlim(1,1);
         }
 
         protected abstract Task<T> ExecuteRefreshAsync(CancellationToken cancellationToken = default);
@@ -162,7 +161,6 @@ private async Task<bool> RefreshAsync(CancellationToken cancellationToken = defa
         /// <exception cref="System.OperationCanceledException">Thrown when the <paramref name="cancellationToken"/> is cancelled during the backoff delay.</exception>
         public async Task<bool> RefreshNowAsync(CancellationToken cancellationToken = default)
         {
-            refreshNeeded = true;
             // When forcing a refresh, always sleep with a random jitter
             // to prevent coding errors that could be calling refreshNow
             // in a loop.
@@ -184,14 +182,15 @@ public async Task<bool> RefreshNowAsync(CancellationToken cancellationToken = de
 
             // Perform the requested refresh.
             bool success = false;
-            await Lock.WaitAsync(cancellationToken);
+            await objLock.WaitAsync(cancellationToken);
+            refreshNeeded = true;
             try
             {
                 success = await RefreshAsync(cancellationToken);
             }
             finally
             {
-                Lock.Release();
+                objLock.Release();
             }
             return (null == exception && success);
         }
@@ -204,14 +203,14 @@ public async Task<bool> RefreshNowAsync(CancellationToken cancellationToken = de
         public async Task<GetSecretValueResponse> GetSecretValue(CancellationToken cancellationToken)
         {
             bool success = false;
-            await Lock.WaitAsync(cancellationToken);
+            await objLock.WaitAsync(cancellationToken);
             try
             {
                 success = await RefreshAsync(cancellationToken);
             }
             finally
             {
-                Lock.Release();
+                objLock.Release();
             }
 
             if (!success && null == data && null != exception)

src/Amazon.SecretsManager.Extensions.Caching/SecretsManagerCache.cs

@@ -29,6 +29,7 @@ public class SecretsManagerCache : ISecretsManagerCache
         private readonly SecretCacheConfiguration config;
         private readonly MemoryCacheEntryOptions cacheItemPolicy;
         private readonly MemoryCache cache = new MemoryCache(new MemoryCacheOptions{ CompactionPercentage = 0 });
+        private readonly SemaphoreSlim cacheLock = new SemaphoreSlim(1,1);
 
         /// <summary>
         /// Initializes a new instance of the <see cref="SecretsManagerCache"/> class.
@@ -88,9 +89,12 @@ public void Dispose()
         /// <summary>
         /// Asynchronously retrieves the specified SecretString after calling <see cref="GetCachedSecret"/>.
         /// </summary>
+        /// <param name="secretId">The secret identifier (ARN or friendly name).</param>
+        /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous operation. The task result contains the secret string value, or <c>null</c> if not present.</returns>
         public async Task<String> GetSecretString(String secretId, CancellationToken cancellationToken = default)
         {
-            SecretCacheItem secret = GetCachedSecret(secretId);
+            SecretCacheItem secret = await GetCachedSecret(secretId, cancellationToken);
             GetSecretValueResponse response = null;
             response = await secret.GetSecretValue(cancellationToken);
             return response?.SecretString;
@@ -99,38 +103,62 @@ public async Task<String> GetSecretString(String secretId, CancellationToken can
         /// <summary>
         /// Asynchronously retrieves the specified SecretBinary after calling <see cref="GetCachedSecret"/>.
         /// </summary>
+        /// <param name="secretId">The secret identifier (ARN or friendly name).</param>
+        /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous operation. The task result contains the secret binary value, or <c>null</c> if not present.</returns>
         public async Task<byte[]> GetSecretBinary(String secretId, CancellationToken cancellationToken = default)
         {
-            SecretCacheItem secret = GetCachedSecret(secretId);
+            SecretCacheItem secret = await GetCachedSecret(secretId, cancellationToken);
             GetSecretValueResponse response = null;
             response = await secret.GetSecretValue(cancellationToken);
             return response?.SecretBinary?.ToArray();
         }
 
         /// <summary>
-        /// Requests the secret value from SecretsManager asynchronously and updates the cache entry with any changes.
+        /// Requests the secret value from Secrets Manager asynchronously and updates the cache entry with any changes.
         /// If there is no existing cache entry, a new one is created.
-        /// Returns true or false depending on if the refresh is successful.
         /// </summary>
+        /// <param name="secretId">The secret identifier (ARN or friendly name).</param>
+        /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous operation. The task result is <c>true</c> if the refresh succeeded; otherwise, <c>false</c>.</returns>
         public async Task<bool> RefreshNowAsync(String secretId, CancellationToken cancellationToken = default)
         {
-            return await GetCachedSecret(secretId).RefreshNowAsync(cancellationToken);
+            SecretCacheItem secretCacheItem = await GetCachedSecret(secretId, cancellationToken);
+            return await secretCacheItem.RefreshNowAsync(cancellationToken);
         }
 
         /// <summary>
-        /// Returns the cache entry corresponding to the specified secret if it exists in the cache.
+        /// Asynchronously returns the cache entry corresponding to the specified secret if it exists in the cache.
         /// Otherwise, the secret value is fetched from Secrets Manager and a new cache entry is created.
         /// </summary>
-        public SecretCacheItem GetCachedSecret(string secretId)
+        /// <param name="secretId">The secret identifier (ARN or friendly name).</param>
+        /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param>
+        /// <returns>A task that represents the asynchronous operation. The task result contains the <see cref="SecretCacheItem"/> for the specified secret.</returns>
+        public async Task<SecretCacheItem> GetCachedSecret(string secretId, CancellationToken cancellationToken = default)
         {
             SecretCacheItem secret = cache.Get<SecretCacheItem>(secretId);
+
             if (secret == null)
             {
-                secret = cache.Set<SecretCacheItem>(secretId, new SecretCacheItem(secretId, secretsManager, config), cacheItemPolicy);
-                if (cache.Count > config.MaxCacheSize)
+                await this.cacheLock.WaitAsync(cancellationToken);
+
+                try
+                {
+                    secret = cache.GetOrCreate<SecretCacheItem>(secretId, entry =>
+                    {
+                        entry.SetOptions(cacheItemPolicy);
+                        return new SecretCacheItem(secretId, secretsManager, config);
+                    });
+
+                    if (cache.Count > config.MaxCacheSize)
+                    {
+                        // Trim cache size to MaxCacheSize, evicting entries using LRU.
+                        cache.Compact((double)(cache.Count - config.MaxCacheSize) / cache.Count);
+                    }
+                }
+                finally
                 {
-                    // Trim cache size to MaxCacheSize, evicting entries using LRU.
-                    cache.Compact((double)(cache.Count - config.MaxCacheSize) / cache.Count);
+                    this.cacheLock.Release();
                 }
             }