Skip to content

Commit

Permalink
address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@HP712
HP712 committed Nov 8, 2024
1 parent 467eec9 commit 7e58f8e
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 27 deletions.
35 changes: 23 additions & 12 deletions src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ public ConfigurationManager(string metadataAddress, IConfigurationRetriever<T> c
/// Obtains an updated version of Configuration.
/// </summary>
/// <returns>Configuration of type T.</returns>
/// <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>
/// <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 async Task<T> GetConfigurationAsync()
{
return await GetConfigurationAsync(CancellationToken.None).ConfigureAwait(false);
Expand All @@ -142,14 +143,15 @@ public async Task<T> GetConfigurationAsync()
/// </summary>
/// <param name="cancel">CancellationToken</param>
/// <returns>Configuration of type T.</returns>
/// <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>
/// <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)
{
// StartupTime is the time when ConfigurationManager was instantiated.
double nextRefresh = _automaticRefreshIntervalInSeconds + _timeInSecondsWhenLastAutomaticRefreshOccurred;
if (nextRefresh > (DateTimeOffset.UtcNow - StartupTime).TotalSeconds)
if (nextRefresh > SecondsSinceInstanceWasCreated())
return _currentConfiguration;
}

Expand All @@ -170,7 +172,7 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
if (_currentConfiguration != null)
return _currentConfiguration;

Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning);
_configurationRetrieverState = ConfigurationRetrieverRunning;
NumberOfTimesAutomaticRefreshRequested++;

// Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
Expand Down Expand Up @@ -210,8 +212,8 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
}
finally
{
_configurationRetrieverState = ConfigurationRetrieverIdle;
_configurationNullLock.Release();
Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);
}
}
else
Expand Down Expand Up @@ -295,14 +297,14 @@ private void UpdateConfiguration(T configuration)
{
_currentConfiguration = configuration;
// StartupTime is the time when ConfigurationManager was instantiated.
// (DateTimeOffset.UtcNow - StartupTime).TotalSeconds is the number of seconds since ConfigurationManager was instantiated.
// For automatic refresh, we add a 5% jit.
// SecondsSinceInstanceWasCreated is the number of seconds since ConfigurationManager was instantiated.
// For automatic refresh, we add a 5% jitter.
// Record in seconds when the last time configuration was obtained.
double secondsWhenRefreshOccurred = (DateTimeOffset.UtcNow - StartupTime).TotalSeconds +
((_automaticRefreshIntervalInSeconds >= int.MaxValue) ? 0 : (new Random().Next((int)_automaticRefreshIntervalInSeconds / 20)));
double timeInSecondsWhenLastAutomaticRefreshOccurred = SecondsSinceInstanceWasCreated() +
((_automaticRefreshIntervalInSeconds >= int.MaxValue) ? 0 : (_random.Next((int)_maxJitter)));

// transfer to int in single operation.
_timeInSecondsWhenLastAutomaticRefreshOccurred = (int)((secondsWhenRefreshOccurred <= int.MaxValue) ? (int)secondsWhenRefreshOccurred : int.MaxValue);
_timeInSecondsWhenLastAutomaticRefreshOccurred = (int)((timeInSecondsWhenLastAutomaticRefreshOccurred <= int.MaxValue) ? (int)timeInSecondsWhenLastAutomaticRefreshOccurred : int.MaxValue);
}

/// <summary>
Expand All @@ -329,20 +331,29 @@ public override void RequestRefresh()
return;

double nextRefresh = _requestRefreshIntervalInSeconds + _timeInSecondsWhenLastRequestRefreshOccurred;
if (nextRefresh < (DateTimeOffset.UtcNow - StartupTime).TotalSeconds || _isFirstRefreshRequest)
if (nextRefresh < SecondsSinceInstanceWasCreated() || _isFirstRefreshRequest)
{
_isFirstRefreshRequest = false;
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
{
NumberOfTimesRequestRefreshRequested++;
double recordWhenRefreshOccurred = (DateTimeOffset.UtcNow - StartupTime).TotalSeconds;
double recordWhenRefreshOccurred = SecondsSinceInstanceWasCreated();
// transfer to int in single operation.
_timeInSecondsWhenLastRequestRefreshOccurred = (int)((recordWhenRefreshOccurred <= int.MaxValue) ? (int)recordWhenRefreshOccurred : int.MaxValue);
_ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
}
}
}

/// <summary>
/// SecondsSinceInstanceWasCreated is the number of seconds since ConfigurationManager was instantiated.
/// </summary>
/// <returns>double</returns>
private double SecondsSinceInstanceWasCreated()
{
return (DateTimeOffset.UtcNow - StartupTime).TotalSeconds;
}

/// <summary>
/// 12 hours is the default time interval that afterwards, <see cref="GetBaseConfigurationAsync(CancellationToken)"/> will obtain new configuration.
/// </summary>
Expand Down
11 changes: 9 additions & 2 deletions src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,21 @@ public abstract class BaseConfigurationManager
{
private TimeSpan _automaticRefreshInterval = DefaultAutomaticRefreshInterval;
internal double _automaticRefreshIntervalInSeconds = DefaultAutomaticRefreshInterval.TotalSeconds;
internal double _maxJitter = DefaultAutomaticRefreshInterval.TotalSeconds / 20;
private TimeSpan _refreshInterval = DefaultRefreshInterval;
internal double _requestRefreshIntervalInSeconds = DefaultRefreshInterval.TotalSeconds;
private TimeSpan _lastKnownGoodLifetime = DefaultLastKnownGoodConfigurationLifetime;
private BaseConfiguration _lastKnownGoodConfiguration;
private DateTime? _lastKnownGoodConfigFirstUse;
internal Random _random = new();

// Seconds since the BaseConfigurationManager was created when the last refresh occurred.
internal int _timeInSecondsWhenLastAutomaticRefreshOccurred;
internal int _timeInSecondsWhenLastRequestRefreshOccurred;

internal EventBasedLRUCache<BaseConfiguration, DateTime> _lastKnownGoodConfigurationCache;


/// <summary>
/// Gets or sets the <see cref="TimeSpan"/> that controls how often an automatic metadata refresh should occur.
/// </summary>
Expand All @@ -42,23 +45,27 @@ public TimeSpan AutomaticRefreshInterval

_automaticRefreshInterval = value;
Interlocked.Exchange(ref _automaticRefreshIntervalInSeconds, value.TotalSeconds);
Interlocked.Exchange(ref _maxJitter, value.TotalSeconds / 20);
}
}

/// <summary>
/// Records the time this instance was created.
/// Used to determine if the automatic refresh or request refresh interval has passed.
/// The logic is to remember the number of seconds since startup that the last refresh occurred.
/// Store that value in _timeInSecondsWhenLastAutomaticRefreshOccurred or _timeInSecondsWhenLastRequestRefreshOccurred.
/// Then compare to (UtcNow - Startup).TotalSeconds.
/// The set is used for testing purposes.
/// </summary>
internal DateTimeOffset StartupTime { get; set; } = DateTimeOffset.UtcNow;

/// <summary>
/// Each time GetConfigurationAsync() results in a http request, this value is incremented.
/// Each call to <see cref="GetBaseConfigurationAsync(CancellationToken)"/> results in a http request, this value is incremented.
/// </summary>
internal long NumberOfTimesAutomaticRefreshRequested { get; set; }

/// <summary>
/// Each time RequestRefresh() results in a http request, this value is incremented.
/// Each call to <see cref="RequestRefresh"/> results in a http request, this value is incremented.
/// </summary>
internal long NumberOfTimesRequestRefreshRequested { get; set; }

Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ Microsoft.IdentityModel.Tokens.BaseConfigurationManager.NumberOfTimesRequestRefr
Microsoft.IdentityModel.Tokens.BaseConfigurationManager.StartupTime.get -> System.DateTimeOffset
Microsoft.IdentityModel.Tokens.BaseConfigurationManager.StartupTime.set -> void
Microsoft.IdentityModel.Tokens.BaseConfigurationManager._automaticRefreshIntervalInSeconds -> double
Microsoft.IdentityModel.Tokens.BaseConfigurationManager._maxJitter -> double
Microsoft.IdentityModel.Tokens.BaseConfigurationManager._random -> System.Random
Microsoft.IdentityModel.Tokens.BaseConfigurationManager._requestRefreshIntervalInSeconds -> double
Microsoft.IdentityModel.Tokens.BaseConfigurationManager._timeInSecondsWhenLastAutomaticRefreshOccurred -> int
Microsoft.IdentityModel.Tokens.BaseConfigurationManager._timeInSecondsWhenLastRequestRefreshOccurred -> int
Expand Down
15 changes: 13 additions & 2 deletions test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,9 +513,9 @@ public async Task CheckThatAutomaticRefreshIntervalIsSetCorrectly()

// _timeInSecondsWhenLastAutomaticRefreshOccurred should not be greater than (totalSeconds + _automaticRefreshIntervalInSeconds / 20)
// where we added 5% of a random amount between 0 and _automaticRefreshIntervalInSeconds.
if (configurationManager._timeInSecondsWhenLastAutomaticRefreshOccurred > (int)(totalSeconds + configurationManager._automaticRefreshIntervalInSeconds / 20))
if (configurationManager._timeInSecondsWhenLastAutomaticRefreshOccurred > (int)(totalSeconds + configurationManager._maxJitter))
context.Diffs.Add($"_timeInSecondsWhenLastAutomaticRefreshOccurred '{configurationManager._timeInSecondsWhenLastAutomaticRefreshOccurred}' > " +
$"(int)(totalSeconds + configurationManager._automaticRefreshIntervalInSeconds / 20) '{(int)(totalSeconds + configurationManager._automaticRefreshIntervalInSeconds / 20)}'," +
$"(int)(totalSeconds + configurationManager._automaticRefreshIntervalInSeconds / 20) '{(int)(totalSeconds + configurationManager._maxJitter)}'," +
$" _automaticRefreshIntervalInSeconds: '{configurationManager._automaticRefreshIntervalInSeconds}'.");

TestUtilities.AssertFailIfErrors(context);
Expand Down Expand Up @@ -829,6 +829,17 @@ public void TestConfigurationComparer()
TestUtilities.AssertFailIfErrors(context);
}

[Fact]
public void TestMaxJitter()
{
var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), new FileDocumentRetriever());
Assert.Equal(configurationManager._automaticRefreshIntervalInSeconds / 20, configurationManager._maxJitter);
configurationManager.AutomaticRefreshInterval = TimeSpan.FromMinutes(5);
Assert.Equal(configurationManager._automaticRefreshIntervalInSeconds / 20, configurationManager._maxJitter);
configurationManager.AutomaticRefreshInterval = TimeSpan.MaxValue;
Assert.Equal(configurationManager._automaticRefreshIntervalInSeconds / 20, configurationManager._maxJitter);
}

[Theory, MemberData(nameof(ValidateOpenIdConnectConfigurationTestCases), DisableDiscoveryEnumeration = true)]
public async Task ValidateOpenIdConnectConfigurationTests(ConfigurationManagerTheoryData<OpenIdConnectConfiguration> theoryData)
{
Expand Down
22 changes: 11 additions & 11 deletions test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ public class EventDrivenConfigurationRetriever<T> : IConfigurationRetriever<T>
private T _configuration;

/// <summary>
/// Initializes an new instance of <see cref="MockConfigurationManager{T}"/> with a Configuration instance.
/// Initializes an new instance of <see cref="EventDrivenConfigurationRetriever{T}"/> with a configuration instance.
/// </summary>
/// <param name="configuration"></param>
/// <param name="signalEvent"></param>
/// <param name="waitEvent"></param>
/// <param name="configuration">The Configuration that will be returned</param>
/// <param name="signalEvent">A <see cref="ManualResetEvent"/>that is be signaled when inside GetConfigurationAsync.</param>
/// <param name="waitEvent">A <see cref="ManualResetEvent"/>that waits inside GetConfigurationAsync.</param>
public EventDrivenConfigurationRetriever(
T configuration,
ManualResetEvent signalEvent,
Expand Down Expand Up @@ -53,13 +53,13 @@ public class EventControlledConfigurationManger<T> : ConfigurationManager<T>, IC
/// <summary>
/// Initializes an new instance of <see cref="EventControlledConfigurationManger{T}"/> with a Configuration instance.
/// </summary>
/// <param name="metadataAddress"></param>
/// <param name="configurationRetriever"></param>
/// <param name="documentRetriever"></param>
/// <param name="configSignalEvent"></param>
/// <param name="configWaitEvent"></param>
/// <param name="refreshSignalEvent"></param>
/// <param name="refreshWaitEvent"></param>
/// <param name="metadataAddress">The metadata address to obtain configuration.</param>
/// <param name="configurationRetriever">The <see cref="IConfigurationRetriever{T}"/>that reads the metadata.</param>
/// <param name="documentRetriever">The <see cref="IDocumentRetriever"/>that obtains the metadata.</param>
/// <param name="configSignalEvent">A <see cref="ManualResetEvent"/>that is signaled when GetConfigurationAsync is exiting.</param>
/// <param name="configWaitEvent">A <see cref="ManualResetEvent"/>that waits in GetConfigurationAsync after calling base.GetConfigurationAsync. </param>
/// <param name="refreshSignalEvent">A <see cref="ManualResetEvent"/> that is signaled when RequestRefresh is exiting.</param>
/// <param name="refreshWaitEvent">A <see cref="ManualResetEvent"/>that waits after base.RequestRefresh is called.</param>
public EventControlledConfigurationManger(
string metadataAddress,
IConfigurationRetriever<T> configurationRetriever,
Expand Down

0 comments on commit 7e58f8e

Please sign in to comment.