From 7e58f8e9d63bf6bfd4e64b95bef00d1726a18207 Mon Sep 17 00:00:00 2001 From: id4s Date: Fri, 8 Nov 2024 08:53:51 -0800 Subject: [PATCH] address PR comments --- .../Configuration/ConfigurationManager.cs | 35 ++++++++++++------- .../BaseConfigurationManager.cs | 11 ++++-- .../InternalAPI.Unshipped.txt | 2 ++ .../ConfigurationManagerTests.cs | 15 ++++++-- .../EventDrivenConfigurationManager.cs | 22 ++++++------ 5 files changed, 58 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index bd31bb4666..12c3fa2ab5 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -131,7 +131,8 @@ public ConfigurationManager(string metadataAddress, IConfigurationRetriever c /// Obtains an updated version of Configuration. /// /// Configuration of type T. - /// If the time since the last call is less than then is not called and the current Configuration is returned. + /// If the time since the last call is less than then + /// is not called and the current Configuration is returned. public async Task GetConfigurationAsync() { return await GetConfigurationAsync(CancellationToken.None).ConfigureAwait(false); @@ -142,14 +143,15 @@ public async Task GetConfigurationAsync() /// /// CancellationToken /// Configuration of type T. - /// If the time since the last call is less than then is not called and the current Configuration is returned. + /// If the time since the last call is less than then + /// is not called and the current Configuration is returned. public virtual async Task 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; } @@ -170,7 +172,7 @@ public virtual async Task 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. @@ -210,8 +212,8 @@ public virtual async Task GetConfigurationAsync(CancellationToken cancel) } finally { + _configurationRetrieverState = ConfigurationRetrieverIdle; _configurationNullLock.Release(); - Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle); } } else @@ -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); } /// @@ -329,13 +331,13 @@ 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); @@ -343,6 +345,15 @@ public override void RequestRefresh() } } + /// + /// SecondsSinceInstanceWasCreated is the number of seconds since ConfigurationManager was instantiated. + /// + /// double + private double SecondsSinceInstanceWasCreated() + { + return (DateTimeOffset.UtcNow - StartupTime).TotalSeconds; + } + /// /// 12 hours is the default time interval that afterwards, will obtain new configuration. /// diff --git a/src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs b/src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs index 36da3dedcf..be636ee662 100644 --- a/src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs @@ -17,11 +17,13 @@ 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; @@ -29,6 +31,7 @@ public abstract class BaseConfigurationManager internal EventBasedLRUCache _lastKnownGoodConfigurationCache; + /// /// Gets or sets the that controls how often an automatic metadata refresh should occur. /// @@ -42,23 +45,27 @@ public TimeSpan AutomaticRefreshInterval _automaticRefreshInterval = value; Interlocked.Exchange(ref _automaticRefreshIntervalInSeconds, value.TotalSeconds); + Interlocked.Exchange(ref _maxJitter, value.TotalSeconds / 20); } } /// /// 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. /// internal DateTimeOffset StartupTime { get; set; } = DateTimeOffset.UtcNow; /// - /// Each time GetConfigurationAsync() results in a http request, this value is incremented. + /// Each call to results in a http request, this value is incremented. /// internal long NumberOfTimesAutomaticRefreshRequested { get; set; } /// - /// Each time RequestRefresh() results in a http request, this value is incremented. + /// Each call to results in a http request, this value is incremented. /// internal long NumberOfTimesRequestRefreshRequested { get; set; } diff --git a/src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt b/src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt index 1819dabca0..e21891df87 100644 --- a/src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt +++ b/src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt @@ -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 diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index 8bdc7ffe5d..8de03f99ef 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -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); @@ -829,6 +829,17 @@ public void TestConfigurationComparer() TestUtilities.AssertFailIfErrors(context); } + [Fact] + public void TestMaxJitter() + { + var configurationManager = new ConfigurationManager("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 theoryData) { diff --git a/test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs b/test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs index 7d1c7f5093..aed68f5b54 100644 --- a/test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs +++ b/test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs @@ -15,11 +15,11 @@ public class EventDrivenConfigurationRetriever : IConfigurationRetriever private T _configuration; /// - /// Initializes an new instance of with a Configuration instance. + /// Initializes an new instance of with a configuration instance. /// - /// - /// - /// + /// The Configuration that will be returned + /// A that is be signaled when inside GetConfigurationAsync. + /// A that waits inside GetConfigurationAsync. public EventDrivenConfigurationRetriever( T configuration, ManualResetEvent signalEvent, @@ -53,13 +53,13 @@ public class EventControlledConfigurationManger : ConfigurationManager, IC /// /// Initializes an new instance of with a Configuration instance. /// - /// - /// - /// - /// - /// - /// - /// + /// The metadata address to obtain configuration. + /// The that reads the metadata. + /// The that obtains the metadata. + /// A that is signaled when GetConfigurationAsync is exiting. + /// A that waits in GetConfigurationAsync after calling base.GetConfigurationAsync. + /// A that is signaled when RequestRefresh is exiting. + /// A that waits after base.RequestRefresh is called. public EventControlledConfigurationManger( string metadataAddress, IConfigurationRetriever configurationRetriever,