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

Set internal _syncAfter using only AutomaticRefreshInterval. #2865

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Set internal _syncAfter using only AutomaticRefreshInterval. #2865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8e9ecf
Set next automatic refresh time only considering AutomaticRefreshInte…
HP712 Oct 2, 2024
6e7dbc1
Increase timeout as test is failing
HP712 Oct 3, 2024
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
2 changes: 1 addition & 1 deletion src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ private void UpdateCurrentConfiguration()
}
finally
{
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval);
Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't seem to set _configurationRetrieverState to Running anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is set to running just before this method gets started in a new task. Or does the fact that it's in a new task change the call context?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On ln 251? It's being set to ConfigurationRetrieverIdle. Regardless, I think we should address it outside of this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so the arguments are flipped in the call to Interlocked.CompareExchange.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. (The CompareExchange is not super intuitive since it compares the first and third parameters...)

if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) != ConfigurationRetrieverRunning)
                {
                    _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
                }

Logic:
If the state is idle (is not running),
then set state to running and call the update
If the state is running
then don't do anything.

at the end of Update method, we correctly reset the state to idle

azure-activedirectory-identitymodel-extensions-for-dotnet/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

Line 317 in e8e9ecf

Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);

}
#pragma warning restore CA1031 // Do not catch general exception types
Expand Down
43 changes: 43 additions & 0 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 @@ -510,6 +510,48 @@ public static TheoryData<ConfigurationManagerTheoryData<OpenIdConnectConfigurati
}
}

[Fact]
public async Task CheckSyncAfter()
{
// This test checks that the _syncAfter field is set correctly after a refresh.
var context = new CompareContext($"{this}.CheckSyncAfter");

var docRetriever = new FileDocumentRetriever();
var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever);

// This is the minimum time that should pass before an automatic refresh occurs
// stored in advance to avoid any time drift issues.
DateTimeOffset minimumRefreshInterval = DateTimeOffset.UtcNow + configManager.AutomaticRefreshInterval;

// get the first configuration, internal _syncAfter should be set to a time greater than UtcNow + AutomaticRefreshInterval.
var configuration = await configManager.GetConfigurationAsync(CancellationToken.None);

// force a refresh by setting internal field
TestUtilities.SetField(configManager, "_syncAfter", DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
configuration = await configManager.GetConfigurationAsync(CancellationToken.None);
// wait 100ms here because update of config is run as a new task.
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
Thread.Sleep(100);

// check that _syncAfter is greater than DateTimeOffset.UtcNow + AutomaticRefreshInterval
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
DateTimeOffset syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
if (syncAfter < minimumRefreshInterval)
context.Diffs.Add($"(AutomaticRefreshInterval) syncAfter '{syncAfter}' < DateTimeOffset.UtcNow + configManager.AutomaticRefreshInterval: '{minimumRefreshInterval}'.");

// make same check for RequestRefresh
// force a refresh by setting internal field
TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
configManager.RequestRefresh();
// wait 100ms here because update of config is run as a new task.
Thread.Sleep(100);

// check that _syncAfter is greater than DateTimeOffset.UtcNow + AutomaticRefreshInterval
syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
if (syncAfter < minimumRefreshInterval)
context.Diffs.Add($"(RequestRefresh) syncAfter '{syncAfter}' < DateTimeOffset.UtcNow + configManager.AutomaticRefreshInterval: '{minimumRefreshInterval}'.");

TestUtilities.AssertFailIfErrors(context);
}

[Fact]
public async Task GetConfigurationAsync()
{
Expand All @@ -520,6 +562,7 @@ public async Task GetConfigurationAsync()
// Unable to obtain a new configuration, but _currentConfiguration is not null so it should be returned.
configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), docRetriever);
var configuration = await configManager.GetConfigurationAsync(CancellationToken.None);

TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
configManager.RequestRefresh();
configManager.MetadataAddress = "http://127.0.0.1";
Expand Down
Loading