RequestRefresh back to a signal for GetConfigurationAsync

build/dependenciesTest.props

@@ -19,4 +19,13 @@
   <PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))">
     <XunitRunnerVisualStudioVersion>3.0.0-pre.49</XunitRunnerVisualStudioVersion>
   </PropertyGroup>
+
+  <!-- MicrosoftExtensionsTimeProviderTesting 8.x has a 6.0.0 target, 9.x does not. -->
+  <PropertyGroup Condition="'$(TargetFramework)' != 'net6.0'">
+    <MicrosoftExtensionsTimeProviderTestingVersion>9.0.0</MicrosoftExtensionsTimeProviderTestingVersion>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(TargetFramework)' == 'net6.0'">
+    <MicrosoftExtensionsTimeProviderTestingVersion>8.10.0</MicrosoftExtensionsTimeProviderTestingVersion>
+  </PropertyGroup>
+
 </Project>

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

@@ -35,6 +35,12 @@ public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationM
         private const int ConfigurationRetrieverRunning = 1;
         private int _configurationRetrieverState = ConfigurationRetrieverIdle;
 
+        private readonly TimeProvider _timeProvider = TimeProvider.System;
+
+        // If a refresh is requested, then do the refresh as a blocking operation
+        // not on a background thread.
+        bool _refreshRequested;
+
         /// <summary>
         /// Instantiates a new <see cref="ConfigurationManager{T}"/> that manages automatic and controls refreshing on configuration data.
         /// </summary>
@@ -147,7 +153,7 @@ public async Task<T> GetConfigurationAsync()
         /// <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 virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
         {
-            if (_currentConfiguration != null && _syncAfter > DateTimeOffset.UtcNow)
+            if (_currentConfiguration != null && _syncAfter > _timeProvider.GetUtcNow())
                 return _currentConfiguration;
 
             Exception fetchMetadataFailure = null;
@@ -214,7 +220,13 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
             {
                 if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
                 {
-                    _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
+                    if (_refreshRequested)
+                    {
+                        UpdateCurrentConfiguration();
+                        _refreshRequested = false;
+                    }
+                    else
+                        _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
                 }
             }
 
@@ -285,7 +297,7 @@ private void UpdateCurrentConfiguration()
         private void UpdateConfiguration(T configuration)
         {
             _currentConfiguration = configuration;
-            _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
+            _syncAfter = DateTimeUtil.Add(_timeProvider.GetUtcNow().UtcDateTime, AutomaticRefreshInterval +
                 TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
         }
 
@@ -309,16 +321,13 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
         /// </summary>
         public override void RequestRefresh()
         {
-            DateTimeOffset now = DateTimeOffset.UtcNow;
-
+            DateTimeOffset now = _timeProvider.GetUtcNow();
             if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest)
             {
                 _isFirstRefreshRequest = false;
-                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
-                {
-                    UpdateCurrentConfiguration();
-                    _lastRequestRefresh = now;
-                }
+                _syncAfter = now;
+                _lastRequestRefresh = now;
+                _refreshRequested = true;
             }
         }
 

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs

@@ -11,6 +11,7 @@
 using System.Reflection;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Extensions.Time.Testing;
 using Microsoft.IdentityModel.Protocols.Configuration;
 using Microsoft.IdentityModel.Protocols.OpenIdConnect.Configuration;
 using Microsoft.IdentityModel.TestUtils;
@@ -19,9 +20,6 @@
 
 namespace Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests
 {
-    /// <summary>
-    /// 
-    /// </summary>
     public class ConfigurationManagerTests
     {
         /// <summary>
@@ -209,48 +207,6 @@ public async Task FetchMetadataFailureTest()
             TestUtilities.AssertFailIfErrors(context);
         }
 
-        [Fact]
-        public async Task VerifyInterlockGuardForRequestRefresh()
-        {
-            ManualResetEvent waitEvent = new ManualResetEvent(false);
-            ManualResetEvent signalEvent = new ManualResetEvent(false);
-            InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent);
-
-            var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(
-                    "AADCommonV1Json",
-                    new OpenIdConnectConfigurationRetriever(),
-                    inMemoryDocumentRetriever);
-
-            // populate the configurationManager with AADCommonV1Config
-            TestUtilities.SetField(configurationManager, "_currentConfiguration", OpenIdConfigData.AADCommonV1Config);
-
-            // InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called.
-            // The first RequestRefresh will not have finished before the next RequestRefresh() is called.
-            // The guard '_lastRequestRefresh' will not block as we set it to DateTimeOffset.MinValue.
-            // Interlocked guard will block.
-            // Configuration should be AADCommonV1Config
-            signalEvent.Reset();
-            _ = Task.Run(() => configurationManager.RequestRefresh());
-
-            // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress
-            // otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished.
-            signalEvent.WaitOne();
-
-            // AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event.
-            configurationManager.MetadataAddress = "AADCommonV2Json";
-            TestUtilities.SetField(configurationManager, "_lastRequestRefresh", DateTimeOffset.MinValue);
-            _ = Task.Run(() => configurationManager.RequestRefresh());
-
-            // Set the event to release the lock and let the previous retriever finish.
-            waitEvent.Set();
-
-            // Configuration should be AADCommonV1Config
-            var configuration = await configurationManager.GetConfigurationAsync();
-            Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer),
-                    $"configuration.Issuer from configurationManager was not as expected," +
-                    $"configuration.Issuer: '{configuration.Issuer}' != expected '{OpenIdConfigData.AADCommonV1Config.Issuer}'.");
-        }
-
         [Fact]
         public async Task VerifyInterlockGuardForGetConfigurationAsync()
         {
@@ -320,13 +276,15 @@ public async Task BootstrapRefreshIntervalTest()
             catch (Exception firstFetchMetadataFailure)
             {
                 // _syncAfter should not have been changed, because the fetch failed.
-                var syncAfter = TestUtilities.GetField(configManager, "_syncAfter");
-                if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue)
+                DateTimeOffset syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
+                if (syncAfter != DateTimeOffset.MinValue)
                     context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'.");
 
                 if (firstFetchMetadataFailure.InnerException == null)
                     context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure.");
 
+                DateTime requestTime = DateTime.UtcNow;
+
                 // Fetch metadata again during refresh interval, the exception should be same from above.
                 try
                 {
@@ -339,9 +297,10 @@ public async Task BootstrapRefreshIntervalTest()
                         context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure.");
 
                     // _syncAfter should not have been changed, because the fetch failed.
-                    syncAfter = TestUtilities.GetField(configManager, "_syncAfter");
-                    if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue)
-                        context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'.");
+                    syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
+
+                    if (!IdentityComparer.AreDatesEqualWithEpsilon(requestTime, syncAfter.UtcDateTime, 1))
+                        context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter.UtcDateTime}' should equal be within 1 second of '{requestTime}'.");
 
                     IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context);
                 }
@@ -605,10 +564,10 @@ public static TheoryData<ConfigurationManagerTheoryData<OpenIdConnectConfigurati
         }
 
         [Fact]
-        public async Task CheckSyncAfter()
+        public async Task CheckSyncAfterAndRefreshRequested()
         {
             // This test checks that the _syncAfter field is set correctly after a refresh.
-            var context = new CompareContext($"{this}.CheckSyncAfter");
+            var context = new CompareContext($"{this}.CheckSyncAfterAndRefreshRequested");
 
             var docRetriever = new FileDocumentRetriever();
             var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever);
@@ -634,9 +593,18 @@ public async Task CheckSyncAfter()
             // make same check for RequestRefresh
             // force a refresh by setting internal field
             TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
+
             configManager.RequestRefresh();
-            // wait 1000ms here because update of config is run as a new task.
-            Thread.Sleep(1000);
+
+            bool refreshRequested = (bool)TestUtilities.GetField(configManager, "_refreshRequested");
+            if (!refreshRequested)
+                context.Diffs.Add("Refresh is expected to be requested after RequestRefresh is called");
+
+            await configManager.GetConfigurationAsync();
+
+            refreshRequested = (bool)TestUtilities.GetField(configManager, "_refreshRequested");
+            if (refreshRequested)
+                context.Diffs.Add("Refresh is not expected to be requested after GetConfigurationAsync is called");
 
             // check that _syncAfter is greater than DateTimeOffset.UtcNow + AutomaticRefreshInterval
             syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
@@ -750,6 +718,96 @@ public void TestConfigurationComparer()
             TestUtilities.AssertFailIfErrors(context);
         }
 
+        [Fact]
+        public async Task RequestRefresh_RespectsRefreshInterval()
+        {
+            // This test checks that the _syncAfter field is set correctly after a refresh.
+            var context = new CompareContext($"{this}.RequestRefresh_RespectsRefreshInterval");
+
+            var timeProvider = new FakeTimeProvider();
+
+            var docRetriever = new FileDocumentRetriever();
+            var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever);
+            TestUtilities.SetField(configManager, "_timeProvider", timeProvider);
+
+            // Get the first configuration.
+            var configuration = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            configManager.RequestRefresh();
+
+            var configAfterFirstRefresh = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            // First RequestRefresh triggers a refresh.
+            if (object.ReferenceEquals(configuration, configAfterFirstRefresh))
+                context.Diffs.Add("object.ReferenceEquals(configuration, configAfterFirstRefresh)");
+
+            configManager.RequestRefresh();
+
+            var configAfterSecondRefresh = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            // Second RequestRefresh should not trigger a refresh because the refresh interval has not passed.
+            if (!object.ReferenceEquals(configAfterFirstRefresh, configAfterSecondRefresh))
+                context.Diffs.Add("!object.ReferenceEquals(configAfterFirstRefresh, configAfterSecondRefresh)");
+
+            // Advance time to trigger a refresh.
+            timeProvider.Advance(configManager.RefreshInterval);
+
+            configManager.RequestRefresh();
+
+            var configAfterThirdRefresh = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            // Third RequestRefresh should trigger a refresh because the refresh interval has passed.
+            if (object.ReferenceEquals(configAfterSecondRefresh, configAfterThirdRefresh))
+                context.Diffs.Add("object.ReferenceEquals(configAfterSecondRefresh, configAfterThirdRefresh)");
+
+            TestUtilities.AssertFailIfErrors(context);
+        }
+
+        [Fact]
+        public async Task GetConfigurationAsync_RespectsRefreshInterval()
+        {
+            var context = new CompareContext($"{this}.GetConfigurationAsync_RespectsRefreshInterval");
+
+            var timeProvider = new FakeTimeProvider();
+
+            var docRetriever = new FileDocumentRetriever();
+            var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever);
+            TestUtilities.SetField(configManager, "_timeProvider", timeProvider);
+
+            TimeSpan advanceInterval = BaseConfigurationManager.DefaultAutomaticRefreshInterval.Add(TimeSpan.FromSeconds(configManager.AutomaticRefreshInterval.TotalSeconds / 20));
+
+            TestUtilities.SetField(configManager, "_timeProvider", timeProvider);
+
+            // Get the first configuration.
+            var configuration = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            var configNoAdvanceInTime = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            // First GetConfigurationAsync should not trigger a refresh because the refresh interval has not passed.
+            if (!object.ReferenceEquals(configuration, configNoAdvanceInTime))
+                context.Diffs.Add("!object.ReferenceEquals(configuration, configNoAdvanceInTime)");
+
+            // Advance time to trigger a refresh.
+            timeProvider.Advance(advanceInterval);
+
+            var configAfterTimeIsAdvanced = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            // Same config, but a task is queued to update the configuration.
+            if (!object.ReferenceEquals(configNoAdvanceInTime, configAfterTimeIsAdvanced))
+                context.Diffs.Add("!object.ReferenceEquals(configuration, configAfterTimeIsAdvanced)");
+
+            // Need to wait for background task to finish.
+            Thread.Sleep(250);
+
+            var configAfterBackgroundTask = await configManager.GetConfigurationAsync(CancellationToken.None);
+
+            // Configuration should be updated after the background task finishes.
+            if (object.ReferenceEquals(configAfterTimeIsAdvanced, configAfterBackgroundTask))
+                context.Diffs.Add("object.ReferenceEquals(configuration, configAfterBackgroundTask)");
+
+            TestUtilities.AssertFailIfErrors(context);
+        }
+
         [Theory, MemberData(nameof(ValidateOpenIdConnectConfigurationTestCases), DisableDiscoveryEnumeration = true)]
         public async Task ValidateOpenIdConnectConfigurationTests(ConfigurationManagerTheoryData<OpenIdConnectConfiguration> theoryData)
         {

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests.csproj

@@ -11,6 +11,12 @@
     <DelaySign>true</DelaySign>
   </PropertyGroup>
 
+  <PropertyGroup Condition="'$(TargetFramework)' == 'net462' or '$(TargetFramework)' == 'net472'">
+    <!-- This warning comes from Microsoft.Extensions.TimeProvider.Testing
+    see here for details: https://github.com/dotnet/extensions/issues/5162#issuecomment-2316266601-->
+    <SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings>
+  </PropertyGroup>
+
   <ItemGroup>
     <None Update="JsonWebKeySet.json;JsonWebKeySetBadBase64Data.json;JsonWebKeySetBadX509Data.json;JsonWebKeySetEnd2End.json;JsonWebKeySetSingleX509Data.json;OpenIdConnectMetadata.json;OpenIdConnectMetadata2.json;JsonWebKeySetUnrecognizedKty.json;JsonWebKeySetBadRsaDataMissingComponent.json;OpenIdConnectMetadataBadBase64Data.json;OpenIdConnectMetadataBadX509Data.json;OpenIdConnectMetadataEnd2End.json;OpenIdConnectMetadataJsonWebKeySetBadUri.json;PingLabsJWKS.json;PingLabs-openid-configuration.json;;JsonWebKeySetEnd2EndEC.json;OpenIdConnectMetadataEnd2EndEC.json;OpenIdConnectMetadataUnrecognizedKty.json;OpenIdConnectMetadataBadRsaDataMissingComponent.json;OpenIdConnectMetadataEnd2EndAcrValuesLast.json">
       <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
@@ -27,6 +33,7 @@
   <ItemGroup>
     <PackageReference Include="System.Text.RegularExpressions" Version="$(SystemTextRegularExpressions)"/>
     <PackageReference Include="System.Net.Http" Version="$(SystemNetHttp)"/>
+    <PackageReference Include="Microsoft.Extensions.TimeProvider.Testing" Version="$(MicrosoftExtensionsTimeProviderTestingVersion)" />
   </ItemGroup>
 
   <ItemGroup>