Skip to content

Commit

Permalink
Make updates to syncAfter and lastRequestRefresh atomic
Browse files Browse the repository at this point in the history
  • Loading branch information
Keegan Caruso committed Jan 11, 2025
1 parent 42b0c2c commit 5e794ce
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 23 deletions.
46 changes: 37 additions & 9 deletions src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Net.Http;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.IdentityModel.Logging;
Expand All @@ -18,8 +19,16 @@ namespace Microsoft.IdentityModel.Protocols
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable")]
public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationManager<T> where T : class
{
private DateTimeOffset _syncAfter = DateTimeOffset.MinValue;
private DateTimeOffset _lastRequestRefresh = DateTimeOffset.MinValue;
// To prevent tearing, this needs to be only updated through AtomicUpdateSyncAfter.
// Reads should be done through the property SyncAfter.
private DateTime _syncAfter = DateTime.MinValue;
private DateTime SyncAfter => _syncAfter;

// See comment above, this should only be updated through AtomicUpdateLastRequestRefresh,
// read through LastRequestRefresh.
private DateTime _lastRequestRefresh = DateTime.MinValue;
private DateTime LastRequestRefresh => _lastRequestRefresh;

private bool _isFirstRefreshRequest = true;
private readonly SemaphoreSlim _configurationNullLock = new SemaphoreSlim(1);

Expand Down Expand Up @@ -156,7 +165,7 @@ public async Task<T> GetConfigurationAsync()
/// <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 && _syncAfter > _timeProvider.GetUtcNow())
if (_currentConfiguration != null && SyncAfter > _timeProvider.GetUtcNow())
return _currentConfiguration;

Exception fetchMetadataFailure = null;
Expand Down Expand Up @@ -242,7 +251,7 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
LogHelper.FormatInvariant(
LogMessages.IDX20803,
LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
LogHelper.MarkAsNonPII(_syncAfter),
LogHelper.MarkAsNonPII(SyncAfter),
LogHelper.MarkAsNonPII(fetchMetadataFailure)),
fetchMetadataFailure));
}
Expand Down Expand Up @@ -300,8 +309,27 @@ private void UpdateCurrentConfiguration()
private void UpdateConfiguration(T configuration)
{
_currentConfiguration = configuration;
_syncAfter = DateTimeUtil.Add(_timeProvider.GetUtcNow().UtcDateTime, AutomaticRefreshInterval +
var newSyncTime = DateTimeUtil.Add(_timeProvider.GetUtcNow().UtcDateTime, AutomaticRefreshInterval +
TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
AtomicUpdateSyncAfter(newSyncTime);
}

private void AtomicUpdateSyncAfter(DateTime syncAfter)
{
// DateTime's backing data is safe to treat as a long if the Kind is not local.
// _syncAfter will always be updated to a UTC time.
// See the implementation of ToBinary on DateTime.
Interlocked.Exchange(
ref Unsafe.As<DateTime, long>(ref _syncAfter),
Unsafe.As<DateTime, long>(ref syncAfter));
}

private void AtomicUpdateLastRequestRefresh(DateTime lastRequestRefresh)
{
// See the comment in AtomicUpdateSyncAfter.
Interlocked.Exchange(
ref Unsafe.As<DateTime, long>(ref _lastRequestRefresh),
Unsafe.As<DateTime, long>(ref lastRequestRefresh));
}

/// <summary>
Expand All @@ -324,12 +352,12 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
/// </summary>
public override void RequestRefresh()
{
DateTimeOffset now = _timeProvider.GetUtcNow();
if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest)
DateTime now = _timeProvider.GetUtcNow().UtcDateTime;
if (now >= DateTimeUtil.Add(LastRequestRefresh, RefreshInterval) || _isFirstRefreshRequest)
{
_isFirstRefreshRequest = false;
_syncAfter = now;
_lastRequestRefresh = now;
AtomicUpdateSyncAfter(now);
AtomicUpdateLastRequestRefresh(now);
_refreshRequested = true;
}
}
Expand Down
29 changes: 15 additions & 14 deletions test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.IO;
using System.Net;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Time.Testing;
Expand Down Expand Up @@ -232,7 +233,7 @@ public async Task VerifyInterlockGuardForGetConfigurationAsync()
waitEvent.Reset();
signalEvent.Reset();

TestUtilities.SetField(configurationManager, "_syncAfter", DateTimeOffset.MinValue);
TestUtilities.SetField(configurationManager, "_syncAfter", DateTime.MinValue);
await configurationManager.GetConfigurationAsync(CancellationToken.None);

// InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress
Expand Down Expand Up @@ -276,8 +277,8 @@ public async Task BootstrapRefreshIntervalTest()
catch (Exception firstFetchMetadataFailure)
{
// _syncAfter should not have been changed, because the fetch failed.
DateTimeOffset syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
if (syncAfter != DateTimeOffset.MinValue)
DateTime syncAfter = (DateTime)TestUtilities.GetField(configManager, "_syncAfter");
if (syncAfter != DateTime.MinValue)
context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'.");

if (firstFetchMetadataFailure.InnerException == null)
Expand All @@ -297,10 +298,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 = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
syncAfter = (DateTime)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}'.");
if (!IdentityComparer.AreDatesEqualWithEpsilon(requestTime, syncAfter, 1))
context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal be within 1 second of '{requestTime}'.");

IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context);
}
Expand Down Expand Up @@ -355,10 +356,10 @@ public async Task AutomaticRefreshInterval(ConfigurationManagerTheoryData<OpenId
IdentityComparer.AreEqual(configuration, theoryData.ExpectedConfiguration, context);

theoryData.ConfigurationManager.MetadataAddress = theoryData.UpdatedMetadataAddress;
TestUtilities.SetField(theoryData.ConfigurationManager, "_syncAfter", theoryData.SyncAfter);
TestUtilities.SetField(theoryData.ConfigurationManager, "_syncAfter", theoryData.SyncAfter.UtcDateTime);
var updatedConfiguration = await theoryData.ConfigurationManager.GetConfigurationAsync(CancellationToken.None);
// we wait 50 ms here to make the task is finished.
Thread.Sleep(50);
// we wait 100 ms here to make the task is finished.
Thread.Sleep(100);
updatedConfiguration = await theoryData.ConfigurationManager.GetConfigurationAsync(CancellationToken.None);
IdentityComparer.AreEqual(updatedConfiguration, theoryData.ExpectedUpdatedConfiguration, context);

Expand Down Expand Up @@ -580,19 +581,19 @@ public async Task CheckSyncAfterAndRefreshRequested()
var configuration = await configManager.GetConfigurationAsync(CancellationToken.None);

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

// check that _syncAfter is greater than DateTimeOffset.UtcNow + AutomaticRefreshInterval
DateTimeOffset syncAfter = (DateTimeOffset)TestUtilities.GetField(configManager, "_syncAfter");
DateTime syncAfter = (DateTime)TestUtilities.GetField(configManager, "_syncAfter");
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));
TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTime.UtcNow.Subtract(TimeSpan.FromHours(1)));

configManager.RequestRefresh();

Expand All @@ -607,7 +608,7 @@ public async Task CheckSyncAfterAndRefreshRequested()
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");
syncAfter = (DateTime)TestUtilities.GetField(configManager, "_syncAfter");
if (syncAfter < minimumRefreshInterval)
context.Diffs.Add($"(RequestRefresh) syncAfter '{syncAfter}' < DateTimeOffset.UtcNow + configManager.AutomaticRefreshInterval: '{minimumRefreshInterval}'.");

Expand All @@ -625,7 +626,7 @@ public async Task GetConfigurationAsync()
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));
TestUtilities.SetField(configManager, "_lastRequestRefresh", DateTime.UtcNow.Subtract(TimeSpan.FromHours(1)));
configManager.MetadataAddress = "http://127.0.0.1";
configManager.RequestRefresh();
var configuration2 = await configManager.GetConfigurationAsync(CancellationToken.None);
Expand Down

0 comments on commit 5e794ce

Please sign in to comment.