Skip to content

Commit

Permalink
RequestRefresh back to a signal
Browse files Browse the repository at this point in the history 
Further reverting the change to RequestRefresh from #2780
This is still too much of a behavioral change. RequestRefresh goes back
to its historical function, signalling that the next call to
GetConfigurationAsync should request data. This is incorporated with
the background thread change to instead do the request as a blocking
operation if from a RequestRefresh

Part of #3082
  • Loading branch information
Keegan Caruso committed Jan 10, 2025
1 parent 6146f1f commit 931a3bf
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 57 deletions.
20 changes: 13 additions & 7 deletions src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationM
private const int ConfigurationRetrieverRunning = 1;
private int _configurationRetrieverState = ConfigurationRetrieverIdle;

// 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>
Expand Down Expand Up @@ -214,7 +218,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);
}
}

Expand Down Expand Up @@ -310,15 +320,11 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
public override void RequestRefresh()
{
DateTimeOffset now = DateTimeOffset.UtcNow;

if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest)
{
_isFirstRefreshRequest = false;
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
{
UpdateCurrentConfiguration();
_lastRequestRefresh = now;
}
_syncAfter = now;
_refreshRequested = true;
}
}

Expand Down
70 changes: 20 additions & 50 deletions test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,48 +209,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()
{
Expand Down Expand Up @@ -320,13 +278,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
{
Expand All @@ -339,9 +299,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);
}
Expand Down Expand Up @@ -605,7 +566,7 @@ 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");
Expand Down Expand Up @@ -634,9 +595,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");
Expand Down

0 comments on commit 931a3bf

Please sign in to comment.