Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequestRefresh back to a signal for GetConfigurationAsync #3087

Merged
merged 3 commits into from
Jan 10, 2025
Merged

RequestRefresh back to a signal for GetConfigurationAsync #3087

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
931a3bf
RequestRefresh back to a signal
Jan 10, 2025
c44ef1a
Add time based testing to ConfigurationManagerTests
Jan 10, 2025
d3c4dbf
Review comments
Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Open in desktop
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.
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
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()
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
{
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;
keegan-caruso marked this conversation as resolved.
Show resolved Hide resolved
}
_syncAfter = now;
_refreshRequested = true;
}
}

Expand Down
70 changes: 20 additions & 50 deletions test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
View file
Open in desktop
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
Loading