Skip to content

Commit bbc09a4

Browse files
brentschmaltzHP712
brentschmaltz
and
HP712
authored
Remove SlimLock when updating metadata. (#2751)
* Remove SlimLock when updating metadata. Update metadata on new Task. * addressed PR comments. --------- Co-authored-by: id4s <[email protected]>
1 parent 1491481 commit bbc09a4

File tree

9 files changed

+684
-225
lines changed

9 files changed

+684
-225
lines changed

‎src/Microsoft.IdentityModel.Protocols.OpenIdConnect/Configuration/OpenIdConnectConfigurationValidator.cs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,20 @@ public ConfigurationValidationResult Validate(OpenIdConnectConfiguration openIdC
3939
Succeeded = false
4040
};
4141
}
42-
var numberOfValidKeys = openIdConnectConfiguration.JsonWebKeySet.Keys.Where(key => key.ConvertedSecurityKey != null).Count();
42+
43+
int numberOfValidKeys = 0;
44+
for (int i = 0; i < openIdConnectConfiguration.JsonWebKeySet.Keys.Count; i++)
45+
if (openIdConnectConfiguration.JsonWebKeySet.Keys[i].ConvertedSecurityKey != null)
46+
numberOfValidKeys++;
4347

4448
if (numberOfValidKeys < MinimumNumberOfKeys)
4549
{
46-
var convertKeyInfos = string.Join("\n", openIdConnectConfiguration.JsonWebKeySet.Keys.Where(key => !string.IsNullOrEmpty(key.ConvertKeyInfo)).Select(key => key.Kid.ToString() + ": " + key.ConvertKeyInfo));
50+
string convertKeyInfos = string.Join(
51+
"\n",
52+
openIdConnectConfiguration.JsonWebKeySet.Keys.Where(
53+
key => !string.IsNullOrEmpty(key.ConvertKeyInfo))
54+
.Select(key => key.Kid.ToString() + ": " + key.ConvertKeyInfo));
55+
4756
return new ConfigurationValidationResult
4857
{
4958
ErrorMessage = LogHelper.FormatInvariant(

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

Lines changed: 140 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the MIT License.
33

44
using System;
5-
using System.Diagnostics.Contracts;
65
using System.Net.Http;
76
using System.Threading;
87
using System.Threading.Tasks;
@@ -20,17 +19,22 @@ namespace Microsoft.IdentityModel.Protocols
2019
public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationManager<T> where T : class
2120
{
2221
private DateTimeOffset _syncAfter = DateTimeOffset.MinValue;
23-
private DateTimeOffset _lastRefresh = DateTimeOffset.MinValue;
22+
private DateTimeOffset _lastRequestRefresh = DateTimeOffset.MinValue;
2423
private bool _isFirstRefreshRequest = true;
2524

26-
private readonly SemaphoreSlim _refreshLock;
2725
private readonly IDocumentRetriever _docRetriever;
2826
private readonly IConfigurationRetriever<T> _configRetriever;
2927
private readonly IConfigurationValidator<T> _configValidator;
3028
private T _currentConfiguration;
31-
private Exception _fetchMetadataFailure;
3229
private TimeSpan _bootstrapRefreshInterval = TimeSpan.FromSeconds(1);
3330

31+
// task states are used to ensure the call to 'update config' (UpdateCurrentConfiguration) is a singleton. Uses Interlocked.CompareExchange.
32+
// metadata is not being obtained
33+
private const int ConfigurationRetrieverIdle = 0;
34+
// metadata is being retrieved
35+
private const int ConfigurationRetrieverRunning = 1;
36+
private int _configurationRetrieverState = ConfigurationRetrieverIdle;
37+
3438
/// <summary>
3539
/// Instantiates a new <see cref="ConfigurationManager{T}"/> that manages automatic and controls refreshing on configuration data.
3640
/// </summary>
@@ -92,7 +96,6 @@ public ConfigurationManager(string metadataAddress, IConfigurationRetriever<T> c
9296
MetadataAddress = metadataAddress;
9397
_docRetriever = docRetriever;
9498
_configRetriever = configRetriever;
95-
_refreshLock = new SemaphoreSlim(1);
9699
}
97100

98101
/// <summary>
@@ -145,83 +148,149 @@ public async Task<T> GetConfigurationAsync()
145148
public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
146149
{
147150
if (_currentConfiguration != null && _syncAfter > DateTimeOffset.UtcNow)
148-
{
149151
return _currentConfiguration;
150-
}
151152

152-
await _refreshLock.WaitAsync(cancel).ConfigureAwait(false);
153-
try
153+
Exception fetchMetadataFailure = null;
154+
155+
// LOGIC
156+
// if configuration != null => configuration has been retrieved before
157+
// reach out to the metadata endpoint
158+
// else
159+
// if task is running, return the current configuration
160+
// else kick off task to update current configuration
161+
if (_currentConfiguration == null)
154162
{
155-
if (_syncAfter <= DateTimeOffset.UtcNow)
163+
try
156164
{
157-
try
165+
// Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
166+
// The transport should have it's own timeouts, etc..
167+
var configuration = await _configRetriever.GetConfigurationAsync(MetadataAddress, _docRetriever, CancellationToken.None).ConfigureAwait(false);
168+
if (_configValidator != null)
158169
{
159-
// Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
160-
// The transport should have it's own timeouts, etc..
161-
var configuration = await _configRetriever.GetConfigurationAsync(MetadataAddress, _docRetriever, CancellationToken.None).ConfigureAwait(false);
162-
if (_configValidator != null)
163-
{
164-
ConfigurationValidationResult result = _configValidator.Validate(configuration);
165-
if (!result.Succeeded)
166-
throw LogHelper.LogExceptionMessage(new InvalidConfigurationException(LogHelper.FormatInvariant(LogMessages.IDX20810, result.ErrorMessage)));
167-
}
168-
169-
_lastRefresh = DateTimeOffset.UtcNow;
170-
// Add a random amount between 0 and 5% of AutomaticRefreshInterval jitter to avoid spike traffic to IdentityProvider.
171-
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
172-
TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
173-
_currentConfiguration = configuration;
170+
ConfigurationValidationResult result = _configValidator.Validate(configuration);
171+
// in this case we have never had a valid configuration, so we will throw an exception if the validation fails
172+
if (!result.Succeeded)
173+
throw LogHelper.LogExceptionMessage(new InvalidConfigurationException(LogHelper.FormatInvariant(LogMessages.IDX20810, result.ErrorMessage)));
174174
}
175-
catch (Exception ex)
176-
{
177-
_fetchMetadataFailure = ex;
178175

179-
if (_currentConfiguration == null) // Throw an exception if there's no configuration to return.
176+
// Add a random amount between 0 and 5% of AutomaticRefreshInterval jitter to avoid spike traffic to IdentityProvider.
177+
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
178+
TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
179+
180+
_currentConfiguration = configuration;
181+
}
182+
catch (Exception ex)
183+
{
184+
fetchMetadataFailure = ex;
185+
186+
// In this case configuration was never obtained.
187+
if (_currentConfiguration == null)
188+
{
189+
if (_bootstrapRefreshInterval < RefreshInterval)
180190
{
181-
if (_bootstrapRefreshInterval < RefreshInterval)
182-
{
183-
// Adopt exponential backoff for bootstrap refresh interval with a decorrelated jitter if it is not longer than the refresh interval.
184-
TimeSpan _bootstrapRefreshIntervalWithJitter = TimeSpan.FromSeconds(new Random().Next((int)_bootstrapRefreshInterval.TotalSeconds));
185-
_bootstrapRefreshInterval += _bootstrapRefreshInterval;
186-
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, _bootstrapRefreshIntervalWithJitter);
187-
}
188-
else
189-
{
190-
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
191-
}
192-
193-
throw LogHelper.LogExceptionMessage(
194-
new InvalidOperationException(
195-
LogHelper.FormatInvariant(LogMessages.IDX20803, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), LogHelper.MarkAsNonPII(_syncAfter), LogHelper.MarkAsNonPII(ex)), ex));
196-
}
191+
// Adopt exponential backoff for bootstrap refresh interval with a decorrelated jitter if it is not longer than the refresh interval.
192+
TimeSpan _bootstrapRefreshIntervalWithJitter = TimeSpan.FromSeconds(new Random().Next((int)_bootstrapRefreshInterval.TotalSeconds));
193+
_bootstrapRefreshInterval += _bootstrapRefreshInterval;
194+
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, _bootstrapRefreshIntervalWithJitter);
195+
}
197196
else
198197
{
199198
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
200-
201-
LogHelper.LogExceptionMessage(
202-
new InvalidOperationException(
203-
LogHelper.FormatInvariant(LogMessages.IDX20806, LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), LogHelper.MarkAsNonPII(ex)), ex));
204199
}
200+
201+
throw LogHelper.LogExceptionMessage(
202+
new InvalidOperationException(
203+
LogHelper.FormatInvariant(
204+
LogMessages.IDX20803,
205+
LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
206+
LogHelper.MarkAsNonPII(_syncAfter),
207+
LogHelper.MarkAsNonPII(ex)),
208+
ex));
209+
}
210+
else
211+
{
212+
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
213+
214+
LogHelper.LogExceptionMessage(
215+
new InvalidOperationException(
216+
LogHelper.FormatInvariant(
217+
LogMessages.IDX20806,
218+
LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
219+
LogHelper.MarkAsNonPII(ex)),
220+
ex));
205221
}
206222
}
223+
}
224+
else
225+
{
226+
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
227+
{
228+
_ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
229+
}
230+
}
231+
232+
// If metadata exists return it.
233+
if (_currentConfiguration != null)
234+
return _currentConfiguration;
235+
236+
throw LogHelper.LogExceptionMessage(
237+
new InvalidOperationException(
238+
LogHelper.FormatInvariant(
239+
LogMessages.IDX20803,
240+
LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
241+
LogHelper.MarkAsNonPII(_syncAfter),
242+
LogHelper.MarkAsNonPII(fetchMetadataFailure)),
243+
fetchMetadataFailure));
244+
}
245+
246+
/// <summary>
247+
/// This should be called when the configuration needs to be updated either from RequestRefresh or AutomaticRefresh, first checking the state checking state using:
248+
/// if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning).
249+
/// </summary>
250+
private void UpdateCurrentConfiguration()
251+
{
252+
#pragma warning disable CA1031 // Do not catch general exception types
253+
try
254+
{
255+
T configuration = _configRetriever.GetConfigurationAsync(
256+
MetadataAddress,
257+
_docRetriever,
258+
CancellationToken.None).ConfigureAwait(false).GetAwaiter().GetResult();
207259

208-
// Stale metadata is better than no metadata
209-
if (_currentConfiguration != null)
210-
return _currentConfiguration;
260+
if (_configValidator == null)
261+
{
262+
_currentConfiguration = configuration;
263+
}
211264
else
212-
throw LogHelper.LogExceptionMessage(
213-
new InvalidOperationException(
214-
LogHelper.FormatInvariant(
215-
LogMessages.IDX20803,
216-
LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
217-
LogHelper.MarkAsNonPII(_syncAfter),
218-
LogHelper.MarkAsNonPII(_fetchMetadataFailure)),
219-
_fetchMetadataFailure));
265+
{
266+
ConfigurationValidationResult result = _configValidator.Validate(configuration);
267+
268+
if (!result.Succeeded)
269+
LogHelper.LogExceptionMessage(
270+
new InvalidConfigurationException(
271+
LogHelper.FormatInvariant(
272+
LogMessages.IDX20810,
273+
result.ErrorMessage)));
274+
else
275+
_currentConfiguration = configuration;
276+
}
277+
}
278+
catch (Exception ex)
279+
{
280+
LogHelper.LogExceptionMessage(
281+
new InvalidOperationException(
282+
LogHelper.FormatInvariant(
283+
LogMessages.IDX20806,
284+
LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
285+
ex),
286+
ex));
220287
}
221288
finally
222289
{
223-
_refreshLock.Release();
290+
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
291+
Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);
224292
}
293+
#pragma warning restore CA1031 // Do not catch general exception types
225294
}
226295

227296
/// <summary>
@@ -232,10 +301,8 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
232301
/// <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>
233302
public override async Task<BaseConfiguration> GetBaseConfigurationAsync(CancellationToken cancel)
234303
{
235-
var obj = await GetConfigurationAsync(cancel).ConfigureAwait(false);
236-
if (obj is BaseConfiguration)
237-
return obj as BaseConfiguration;
238-
return null;
304+
T obj = await GetConfigurationAsync(cancel).ConfigureAwait(false);
305+
return obj as BaseConfiguration;
239306
}
240307

241308
/// <summary>
@@ -246,14 +313,15 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
246313
public override void RequestRefresh()
247314
{
248315
DateTimeOffset now = DateTimeOffset.UtcNow;
249-
if (_isFirstRefreshRequest)
316+
317+
if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest )
250318
{
251-
_syncAfter = now;
252319
_isFirstRefreshRequest = false;
253-
}
254-
else if (now >= DateTimeUtil.Add(_lastRefresh.UtcDateTime, RefreshInterval))
255-
{
256-
_syncAfter = now;
320+
_lastRequestRefresh = now;
321+
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
322+
{
323+
_ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
324+
}
257325
}
258326
}
259327

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,22 @@ public HttpDocumentRetriever(HttpClient httpClient)
8484
public async Task<string> GetDocumentAsync(string address, CancellationToken cancel)
8585
{
8686
if (string.IsNullOrWhiteSpace(address))
87-
throw LogHelper.LogArgumentNullException("address");
87+
throw LogHelper.LogArgumentNullException(nameof(address));
8888

8989
if (!Utility.IsHttps(address) && RequireHttps)
90-
throw LogHelper.LogExceptionMessage(new ArgumentException(LogHelper.FormatInvariant(LogMessages.IDX20108, address), nameof(address)));
90+
throw LogHelper.LogExceptionMessage(
91+
new ArgumentException(
92+
LogHelper.FormatInvariant(
93+
LogMessages.IDX20108,
94+
LogHelper.MarkAsNonPII(address)),
95+
nameof(address)));
9196

9297
Exception unsuccessfulHttpResponseException;
9398
HttpResponseMessage response;
9499
try
95100
{
96101
if (LogHelper.IsEnabled(EventLogLevel.Verbose))
97-
LogHelper.LogVerbose(LogMessages.IDX20805, address);
102+
LogHelper.LogVerbose(LogMessages.IDX20805, LogHelper.MarkAsNonPII(address));
98103

99104
var httpClient = _httpClient ?? _defaultHttpClient;
100105
var uri = new Uri(address, UriKind.RelativeOrAbsolute);
@@ -104,13 +109,24 @@ public async Task<string> GetDocumentAsync(string address, CancellationToken can
104109
if (response.IsSuccessStatusCode)
105110
return responseContent;
106111

107-
unsuccessfulHttpResponseException = new IOException(LogHelper.FormatInvariant(LogMessages.IDX20807, address, response, responseContent));
112+
unsuccessfulHttpResponseException = new IOException(
113+
LogHelper.FormatInvariant(
114+
LogMessages.IDX20807,
115+
LogHelper.MarkAsNonPII(address),
116+
response,
117+
responseContent));
118+
108119
unsuccessfulHttpResponseException.Data.Add(StatusCode, response.StatusCode);
109120
unsuccessfulHttpResponseException.Data.Add(ResponseContent, responseContent);
110121
}
111122
catch (Exception ex)
112123
{
113-
throw LogHelper.LogExceptionMessage(new IOException(LogHelper.FormatInvariant(LogMessages.IDX20804, address), ex));
124+
throw LogHelper.LogExceptionMessage(
125+
new IOException(
126+
LogHelper.FormatInvariant(
127+
LogMessages.IDX20804,
128+
LogHelper.MarkAsNonPII(address)),
129+
ex));
114130
}
115131

116132
throw LogHelper.LogExceptionMessage(unsuccessfulHttpResponseException);

0 commit comments

Comments
 (0)