Revert "Remove SlimLock when updating metadata. (#2751)"

This reverts commit bbc09a4.

Author: Keegan Caruso

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/Configuration/OpenIdConnectConfigurationValidator.cs

@@ -39,20 +39,11 @@ public ConfigurationValidationResult Validate(OpenIdConnectConfiguration openIdC
                     Succeeded = false
                 };
             }
-
-            int numberOfValidKeys = 0;
-            for (int i = 0;  i < openIdConnectConfiguration.JsonWebKeySet.Keys.Count; i++)
-                if (openIdConnectConfiguration.JsonWebKeySet.Keys[i].ConvertedSecurityKey != null)
-                    numberOfValidKeys++;
+            var numberOfValidKeys = openIdConnectConfiguration.JsonWebKeySet.Keys.Where(key => key.ConvertedSecurityKey != null).Count();
 
             if (numberOfValidKeys < MinimumNumberOfKeys)
             {
-                string convertKeyInfos = string.Join(
-                    "\n",
-                    openIdConnectConfiguration.JsonWebKeySet.Keys.Where(
-                        key => !string.IsNullOrEmpty(key.ConvertKeyInfo))
-                    .Select(key => key.Kid.ToString() + ": " + key.ConvertKeyInfo));
-
+                var convertKeyInfos = string.Join("\n", openIdConnectConfiguration.JsonWebKeySet.Keys.Where(key => !string.IsNullOrEmpty(key.ConvertKeyInfo)).Select(key => key.Kid.ToString() + ": " + key.ConvertKeyInfo));
                 return new ConfigurationValidationResult
                 {
                     ErrorMessage = LogHelper.FormatInvariant(

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Diagnostics.Contracts;
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
@@ -19,22 +20,17 @@ namespace Microsoft.IdentityModel.Protocols
     public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationManager<T> where T : class
     {
         private DateTimeOffset _syncAfter = DateTimeOffset.MinValue;
-        private DateTimeOffset _lastRequestRefresh = DateTimeOffset.MinValue;
+        private DateTimeOffset _lastRefresh = DateTimeOffset.MinValue;
         private bool _isFirstRefreshRequest = true;
 
+        private readonly SemaphoreSlim _refreshLock;
         private readonly IDocumentRetriever _docRetriever;
         private readonly IConfigurationRetriever<T> _configRetriever;
         private readonly IConfigurationValidator<T> _configValidator;
         private T _currentConfiguration;
+        private Exception _fetchMetadataFailure;
         private TimeSpan _bootstrapRefreshInterval = TimeSpan.FromSeconds(1);
 
-        // task states are used to ensure the call to 'update config' (UpdateCurrentConfiguration) is a singleton. Uses Interlocked.CompareExchange.
-        // metadata is not being obtained
-        private const int ConfigurationRetrieverIdle = 0;
-        // metadata is being retrieved
-        private const int ConfigurationRetrieverRunning = 1;
-        private int _configurationRetrieverState = ConfigurationRetrieverIdle;
-
         /// <summary>
         /// Instantiates a new <see cref="ConfigurationManager{T}"/> that manages automatic and controls refreshing on configuration data.
         /// </summary>
@@ -96,6 +92,7 @@ public ConfigurationManager(string metadataAddress, IConfigurationRetriever<T> c
             MetadataAddress = metadataAddress;
             _docRetriever = docRetriever;
             _configRetriever = configRetriever;
+            _refreshLock = new SemaphoreSlim(1);
         }
 
         /// <summary>
@@ -148,149 +145,83 @@ public async Task<T> GetConfigurationAsync()
         public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
         {
             if (_currentConfiguration != null && _syncAfter > DateTimeOffset.UtcNow)
+            {
                 return _currentConfiguration;
+            }
 
-            Exception fetchMetadataFailure = null;
-
-            // LOGIC
-            // if configuration != null => configuration has been retrieved before
-            // reach out to the metadata endpoint
-            // else
-            //   if task is running, return the current configuration
-            //   else kick off task to update current configuration
-            if (_currentConfiguration == null)
+            await _refreshLock.WaitAsync(cancel).ConfigureAwait(false);
+            try
             {
-                try
+                if (_syncAfter <= DateTimeOffset.UtcNow)
                 {
-                    // Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
-                    // The transport should have it's own timeouts, etc..
-                    var configuration = await _configRetriever.GetConfigurationAsync(MetadataAddress, _docRetriever, CancellationToken.None).ConfigureAwait(false);
-                    if (_configValidator != null)
+                    try
                     {
-                        ConfigurationValidationResult result = _configValidator.Validate(configuration);
-                        // in this case we have never had a valid configuration, so we will throw an exception if the validation fails
-                        if (!result.Succeeded)
-                            throw LogHelper.LogExceptionMessage(new InvalidConfigurationException(LogHelper.FormatInvariant(LogMessages.IDX20810, result.ErrorMessage)));
-                    }
-
-                    // Add a random amount between 0 and 5% of AutomaticRefreshInterval jitter to avoid spike traffic to IdentityProvider.
-                    _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
-                        TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
-
-                    _currentConfiguration = configuration;
-                }
-                catch (Exception ex)
-                {
-                    fetchMetadataFailure = ex;
+                        // Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
+                        // The transport should have it's own timeouts, etc..
+                        var configuration = await _configRetriever.GetConfigurationAsync(MetadataAddress, _docRetriever, CancellationToken.None).ConfigureAwait(false);
+                        if (_configValidator != null)
+                        {
+                            ConfigurationValidationResult result = _configValidator.Validate(configuration);
+                            if (!result.Succeeded)
+                                throw LogHelper.LogExceptionMessage(new InvalidConfigurationException(LogHelper.FormatInvariant(LogMessages.IDX20810, result.ErrorMessage)));
+                        }
 
-                    // In this case configuration was never obtained.
-                    if (_currentConfiguration == null)
+                        _lastRefresh = DateTimeOffset.UtcNow;
+                        // Add a random amount between 0 and 5% of AutomaticRefreshInterval jitter to avoid spike traffic to IdentityProvider.
+                        _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
+                            TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
+                        _currentConfiguration = configuration;
+                    }
+                    catch (Exception ex)
                     {
-                        if (_bootstrapRefreshInterval < RefreshInterval)
+                        _fetchMetadataFailure = ex;
+
+                        if (_currentConfiguration == null) // Throw an exception if there's no configuration to return.
                         {
-                            // Adopt exponential backoff for bootstrap refresh interval with a decorrelated jitter if it is not longer than the refresh interval.
-                            TimeSpan _bootstrapRefreshIntervalWithJitter = TimeSpan.FromSeconds(new Random().Next((int)_bootstrapRefreshInterval.TotalSeconds));
-                            _bootstrapRefreshInterval += _bootstrapRefreshInterval;
-                            _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, _bootstrapRefreshIntervalWithJitter);
-                        }
+                            if (_bootstrapRefreshInterval < RefreshInterval)
+                            {
+                                // Adopt exponential backoff for bootstrap refresh interval with a decorrelated jitter if it is not longer than the refresh interval.
+                                TimeSpan _bootstrapRefreshIntervalWithJitter = TimeSpan.FromSeconds(new Random().Next((int)_bootstrapRefreshInterval.TotalSeconds));
+                                _bootstrapRefreshInterval += _bootstrapRefreshInterval;
+                                _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, _bootstrapRefreshIntervalWithJitter);
+                            }
+                            else
+                            {
+                                _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
+                            }
+
+                            throw LogHelper.LogExceptionMessage(
+                                new InvalidOperationException(
+                                    LogHelper.FormatInvariant(LogMessages.IDX20803, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), LogHelper.MarkAsNonPII(_syncAfter), LogHelper.MarkAsNonPII(ex)), ex));
+                        } 
                         else
                         {
                             _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
-                        }
-
-                        throw LogHelper.LogExceptionMessage(
-                            new InvalidOperationException(
-                                LogHelper.FormatInvariant(
-                                    LogMessages.IDX20803,
-                                    LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                                    LogHelper.MarkAsNonPII(_syncAfter),
-                                    LogHelper.MarkAsNonPII(ex)),
-                                ex));
-                    }
-                    else
-                    {
-                        _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
 
-                        LogHelper.LogExceptionMessage(
-                            new InvalidOperationException(
-                                LogHelper.FormatInvariant(
-                                    LogMessages.IDX20806,
-                                    LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                                    LogHelper.MarkAsNonPII(ex)),
-                                ex));
+                            LogHelper.LogExceptionMessage(
+                                new InvalidOperationException(
+                                    LogHelper.FormatInvariant(LogMessages.IDX20806, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), LogHelper.MarkAsNonPII(ex)), ex));
+                        }
                     }
                 }
-            }
-            else
-            {
-                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
-                {
-                    _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
-                }
-            }
-
-            // If metadata exists return it.
-            if (_currentConfiguration != null)
-                return _currentConfiguration;
-
-            throw LogHelper.LogExceptionMessage(
-                new InvalidOperationException(
-                    LogHelper.FormatInvariant(
-                        LogMessages.IDX20803,
-                        LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                        LogHelper.MarkAsNonPII(_syncAfter),
-                        LogHelper.MarkAsNonPII(fetchMetadataFailure)),
-                    fetchMetadataFailure));
-        }
-
-        /// <summary>
-        /// This should be called when the configuration needs to be updated either from RequestRefresh or AutomaticRefresh, first checking the state checking state using:
-        /// if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning).
-        /// </summary>
-        private void UpdateCurrentConfiguration()
-        {
-#pragma warning disable CA1031 // Do not catch general exception types
-            try
-            {
-                T configuration = _configRetriever.GetConfigurationAsync(
-                    MetadataAddress,
-                    _docRetriever,
-                    CancellationToken.None).ConfigureAwait(false).GetAwaiter().GetResult();
 
-                if (_configValidator == null)
-                {
-                    _currentConfiguration = configuration;
-                }
+                // Stale metadata is better than no metadata
+                if (_currentConfiguration != null)
+                    return _currentConfiguration;
                 else
-                {
-                    ConfigurationValidationResult result = _configValidator.Validate(configuration);
-
-                    if (!result.Succeeded)
-                        LogHelper.LogExceptionMessage(
-                            new InvalidConfigurationException(
-                                LogHelper.FormatInvariant(
-                                    LogMessages.IDX20810,
-                                    result.ErrorMessage)));
-                    else
-                        _currentConfiguration = configuration;
-                }
-            }
-            catch (Exception ex)
-            {
-                LogHelper.LogExceptionMessage(
-                    new InvalidOperationException(
-                        LogHelper.FormatInvariant(
-                            LogMessages.IDX20806,
-                            LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                            ex),
-                        ex));
+                    throw LogHelper.LogExceptionMessage(
+                        new InvalidOperationException(
+                            LogHelper.FormatInvariant(
+                                LogMessages.IDX20803,
+                                LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
+                                LogHelper.MarkAsNonPII(_syncAfter),
+                                LogHelper.MarkAsNonPII(_fetchMetadataFailure)),
+                            _fetchMetadataFailure));
             }
             finally
             {
-                _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
-                Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);
+                _refreshLock.Release();
             }
-#pragma warning restore CA1031 // Do not catch general exception types
         }
 
         /// <summary>
@@ -301,8 +232,10 @@ private void UpdateCurrentConfiguration()
         /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
         public override async Task<BaseConfiguration> GetBaseConfigurationAsync(CancellationToken cancel)
         {
-            T obj = await GetConfigurationAsync(cancel).ConfigureAwait(false);
-            return obj as BaseConfiguration;
+            var obj = await GetConfigurationAsync(cancel).ConfigureAwait(false);
+            if (obj is BaseConfiguration)
+                return obj as BaseConfiguration;
+            return null;
         }
 
         /// <summary>
@@ -313,15 +246,14 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
         public override void RequestRefresh()
         {
             DateTimeOffset now = DateTimeOffset.UtcNow;
-
-            if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest )
+            if (_isFirstRefreshRequest)
             {
+                _syncAfter = now;
                 _isFirstRefreshRequest = false;
-                _lastRequestRefresh = now;
-                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
-                {
-                    _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
-                }
+            }
+            else if (now >= DateTimeUtil.Add(_lastRefresh.UtcDateTime, RefreshInterval))
+            {
+                _syncAfter = now;
             }
         }
 

src/Microsoft.IdentityModel.Protocols/Configuration/HttpDocumentRetriever.cs

@@ -84,22 +84,17 @@ public HttpDocumentRetriever(HttpClient httpClient)
         public async Task<string> GetDocumentAsync(string address, CancellationToken cancel)
         {
             if (string.IsNullOrWhiteSpace(address))
-                throw LogHelper.LogArgumentNullException(nameof(address));
+                throw LogHelper.LogArgumentNullException("address");
 
             if (!Utility.IsHttps(address) && RequireHttps)
-                throw LogHelper.LogExceptionMessage(
-                    new ArgumentException(
-                        LogHelper.FormatInvariant(
-                            LogMessages.IDX20108,
-                            LogHelper.MarkAsNonPII(address)),
-                        nameof(address)));
+                throw LogHelper.LogExceptionMessage(new ArgumentException(LogHelper.FormatInvariant(LogMessages.IDX20108, address), nameof(address)));
 
             Exception unsuccessfulHttpResponseException;
             HttpResponseMessage response;
             try
             {
                 if (LogHelper.IsEnabled(EventLogLevel.Verbose))
-                    LogHelper.LogVerbose(LogMessages.IDX20805, LogHelper.MarkAsNonPII(address));
+                    LogHelper.LogVerbose(LogMessages.IDX20805, address);
 
                 var httpClient = _httpClient ?? _defaultHttpClient;
                 var uri = new Uri(address, UriKind.RelativeOrAbsolute);
@@ -109,24 +104,13 @@ public async Task<string> GetDocumentAsync(string address, CancellationToken can
                 if (response.IsSuccessStatusCode)
                     return responseContent;
 
-                unsuccessfulHttpResponseException = new IOException(
-                    LogHelper.FormatInvariant(
-                        LogMessages.IDX20807,
-                        LogHelper.MarkAsNonPII(address),
-                        response,
-                        responseContent));
-
+                unsuccessfulHttpResponseException = new IOException(LogHelper.FormatInvariant(LogMessages.IDX20807, address, response, responseContent));
                 unsuccessfulHttpResponseException.Data.Add(StatusCode, response.StatusCode);
                 unsuccessfulHttpResponseException.Data.Add(ResponseContent, responseContent);
             }
             catch (Exception ex)
             {
-                throw LogHelper.LogExceptionMessage(
-                    new IOException(
-                        LogHelper.FormatInvariant(
-                            LogMessages.IDX20804,
-                            LogHelper.MarkAsNonPII(address)),
-                        ex));
+                throw LogHelper.LogExceptionMessage(new IOException(LogHelper.FormatInvariant(LogMessages.IDX20804, address), ex));
             }
 
             throw LogHelper.LogExceptionMessage(unsuccessfulHttpResponseException);