Skip to content

Commit e9f4fc3

Browse files
HP712HP712
committed
Fixed Interlock ordering
1 parent 9a502c9 commit e9f4fc3

File tree

4 files changed

+130
-13
lines changed

4 files changed

+130
-13
lines changed

‎src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
212212
}
213213
else
214214
{
215-
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
215+
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
216216
{
217217
_ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
218218
}
@@ -302,8 +302,9 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
302302
}
303303

304304
/// <summary>
305-
/// Requests that then next call to <see cref="GetConfigurationAsync()"/> obtain new configuration.
306-
/// <para>If it is a first force refresh or the last refresh was greater than <see cref="BaseConfigurationManager.RefreshInterval"/> then the next call to <see cref="GetConfigurationAsync()"/> will retrieve new configuration.</para>
305+
/// Triggers updating metadata when:
306+
/// <para>1. Called the first time.</para>
307+
/// <para>2. The time between when this method was called and DateTimeOffset.Now is greater than <see cref="BaseConfigurationManager.RefreshInterval"/>.</para>
307308
/// <para>If <see cref="BaseConfigurationManager.RefreshInterval"/> == <see cref="TimeSpan.MaxValue"/> then this method does nothing.</para>
308309
/// </summary>
309310
public override void RequestRefresh()
@@ -313,10 +314,10 @@ public override void RequestRefresh()
313314
if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest)
314315
{
315316
_isFirstRefreshRequest = false;
316-
_lastRequestRefresh = now;
317-
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
317+
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
318318
{
319319
_ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
320+
_lastRequestRefresh = now;
320321
}
321322
}
322323
}

‎test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public class ConfigurationManagerTests
2626
{
2727
/// <summary>
2828
/// This test reaches out the the internet to fetch the OpenIdConnectConfiguration from the specified metadata address.
29-
/// There is no validaiton of the configuration. The validation is done in the OpenIdConnectConfigurationSerializationTests.Deserialize
29+
/// There is no validation of the configuration. The validation is done in the OpenIdConnectConfigurationSerializationTests.Deserialize
3030
/// against values obtained 2/2/2024
3131
/// </summary>
3232
/// <param name="theoryData"></param>
@@ -209,6 +209,94 @@ public async Task FetchMetadataFailureTest()
209209
TestUtilities.AssertFailIfErrors(context);
210210
}
211211

212+
[Fact]
213+
public async Task VerifyInterlockGuardForRequestRefresh()
214+
{
215+
ManualResetEvent waitEvent = new ManualResetEvent(false);
216+
ManualResetEvent signalEvent = new ManualResetEvent(false);
217+
InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent);
218+
219+
var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(
220+
"AADCommonV1Json",
221+
new OpenIdConnectConfigurationRetriever(),
222+
inMemoryDocumentRetriever);
223+
224+
// populate the configurationManager with AADCommonV1Config
225+
TestUtilities.SetField(configurationManager, "_currentConfiguration", OpenIdConfigData.AADCommonV1Config);
226+
227+
// InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called.
228+
// The first RequestRefresh will not have finished before the next RequestRefresh() is called.
229+
// The guard '_lastRequestRefresh' will not block as we set it to DateTimeOffset.MinValue.
230+
// Interlocked guard will block.
231+
// Configuration should be AADCommonV1Config
232+
signalEvent.Reset();
233+
configurationManager.RequestRefresh();
234+
235+
// InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress
236+
// otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished.
237+
signalEvent.WaitOne();
238+
239+
// AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event.
240+
configurationManager.MetadataAddress = "AADCommonV2Json";
241+
TestUtilities.SetField(configurationManager, "_lastRequestRefresh", DateTimeOffset.MinValue);
242+
configurationManager.RequestRefresh();
243+
244+
// Set the event to release the lock and let the previous retriever finish.
245+
waitEvent.Set();
246+
247+
// Configuration should be AADCommonV1Config
248+
var configuration = await configurationManager.GetConfigurationAsync();
249+
Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer),
250+
$"configuration.Issuer from configurationManager was not as expected," +
251+
$"configuration.Issuer: '{configuration.Issuer}' != expected '{OpenIdConfigData.AADCommonV1Config.Issuer}'.");
252+
}
253+
254+
[Fact]
255+
public async Task VerifyInterlockGuardForGetConfigurationAsync()
256+
{
257+
ManualResetEvent waitEvent = new ManualResetEvent(false);
258+
ManualResetEvent signalEvent = new ManualResetEvent(false);
259+
260+
InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent);
261+
waitEvent.Set();
262+
263+
var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(
264+
"AADCommonV1Json",
265+
new OpenIdConnectConfigurationRetriever(),
266+
inMemoryDocumentRetriever);
267+
268+
OpenIdConnectConfiguration configuration = await configurationManager.GetConfigurationAsync();
269+
270+
// InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called.
271+
// The GetConfigurationAsync to update config will not have finished before the next GetConfigurationAsync() is called.
272+
// The guard '_syncAfter' will not block as we set it to DateTimeOffset.MinValue.
273+
// Interlocked guard should block.
274+
// Configuration should be AADCommonV1Config
275+
276+
waitEvent.Reset();
277+
signalEvent.Reset();
278+
279+
TestUtilities.SetField(configurationManager, "_syncAfter", DateTimeOffset.MinValue);
280+
await configurationManager.GetConfigurationAsync(CancellationToken.None);
281+
282+
// InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress
283+
// otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished.
284+
signalEvent.WaitOne();
285+
286+
// AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event.
287+
configurationManager.MetadataAddress = "AADCommonV2Json";
288+
await configurationManager.GetConfigurationAsync(CancellationToken.None);
289+
290+
// Set the event to release the lock and let the previous retriever finish.
291+
waitEvent.Set();
292+
293+
// Configuration should be AADCommonV1Config
294+
configuration = await configurationManager.GetConfigurationAsync();
295+
Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer),
296+
$"configuration.Issuer from configurationManager was not as expected," +
297+
$" configuration.Issuer: '{configuration.Issuer}' != expected: '{OpenIdConfigData.AADCommonV1Config.Issuer}'.");
298+
}
299+
212300
[Fact]
213301
public async Task BootstrapRefreshIntervalTest()
214302
{
@@ -814,6 +902,20 @@ public static TheoryData<ConfigurationManagerTheoryData<OpenIdConnectConfigurati
814902
{ "https://login.microsoftonline.com/common/discovery/v2.0/keys", OpenIdConfigData.AADCommonV2JwksString }
815903
});
816904

905+
private static InMemoryDocumentRetriever InMemoryDocumentRetrieverWithEvents(ManualResetEvent waitEvent, ManualResetEvent signalEvent)
906+
{
907+
return new InMemoryDocumentRetriever(
908+
new Dictionary<string, string>
909+
{
910+
{ "AADCommonV1Json", OpenIdConfigData.AADCommonV1Json },
911+
{ "https://login.microsoftonline.com/common/discovery/keys", OpenIdConfigData.AADCommonV1JwksString },
912+
{ "AADCommonV2Json", OpenIdConfigData.AADCommonV2Json },
913+
{ "https://login.microsoftonline.com/common/discovery/v2.0/keys", OpenIdConfigData.AADCommonV2JwksString }
914+
},
915+
waitEvent,
916+
signalEvent);
917+
}
918+
817919
public class ConfigurationManagerTheoryData<T> : TheoryDataBase where T : class
818920
{
819921
public ConfigurationManager<T> ConfigurationManager { get; set; }

‎test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,8 @@
1212
using System.Threading.Tasks;
1313
using Microsoft.IdentityModel.TestUtils;
1414
using Microsoft.IdentityModel.Tokens;
15-
using Newtonsoft.Json;
1615
using Xunit;
1716

18-
#pragma warning disable CS3016 // Arrays as attribute arguments is not CLS-compliant
19-
2017
namespace Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests
2118
{
2219
/// <summary>
@@ -1813,5 +1810,3 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData)
18131810
}
18141811
}
18151812
}
1816-
1817-
#pragma warning restore CS3016 // Arrays as attribute arguments is not CLS-compliant

‎test/Microsoft.IdentityModel.TestUtils/InMemoryDocumentRetriever.cs‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,25 @@ namespace Microsoft.IdentityModel.TestUtils
1414
/// </summary>
1515
public class InMemoryDocumentRetriever : IDocumentRetriever
1616
{
17-
private readonly IDictionary<string, string> _configurations;
17+
private readonly Dictionary<string, string> _configurations;
18+
private ManualResetEvent _waitEvent;
19+
private ManualResetEvent _signalEvent;
1820

1921
/// <summary>
2022
/// Initializes a new instance of the <see cref="FileDocumentRetriever"/> class.
2123
/// </summary>
22-
public InMemoryDocumentRetriever(IDictionary<string, string> configuration)
24+
public InMemoryDocumentRetriever(Dictionary<string, string> configuration)
2325
{
2426
_configurations = configuration;
2527
}
2628

29+
public InMemoryDocumentRetriever(Dictionary<string, string> configuration, ManualResetEvent waitEvent, ManualResetEvent signalEvent)
30+
{
31+
_configurations = configuration;
32+
_waitEvent = waitEvent;
33+
_signalEvent = signalEvent;
34+
}
35+
2736
/// <summary>
2837
/// Returns the document passed in constructor in dictionary./>
2938
/// </summary>
@@ -32,6 +41,16 @@ public InMemoryDocumentRetriever(IDictionary<string, string> configuration)
3241
/// <returns>UTF8 decoding of bytes in the file.</returns>
3342
public async Task<string> GetDocumentAsync(string address, CancellationToken cancel)
3443
{
44+
// Some tests change the Metadata address on ConfigurationManger to test different scenarios.
45+
// This event is used to let the test know that the GetDocumentAsync method has been called, and the test can now change the Metadata address.
46+
if (_signalEvent != null)
47+
_signalEvent.Set();
48+
49+
// This event lets the caller control when metadata can be returned.
50+
// Useful when testing delays.
51+
if (_waitEvent != null)
52+
_waitEvent.WaitOne();
53+
3554
return await Task.FromResult(_configurations[address]).ConfigureAwait(false);
3655
}
3756
}

0 commit comments

Comments
 (0)