-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set internal _syncAfter using only AutomaticRefreshInterval. #2865
Conversation
test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
Show resolved
Hide resolved
@@ -313,7 +313,7 @@ private void UpdateCurrentConfiguration() | |||
} | |||
finally | |||
{ | |||
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval); | |||
_syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval); | |||
Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't seem to set _configurationRetrieverState
to Running anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is set to running just before this method gets started in a new task. Or does the fact that it's in a new task change the call context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On ln 251? It's being set to ConfigurationRetrieverIdle
. Regardless, I think we should address it outside of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so the arguments are flipped in the call to Interlocked.CompareExchange
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. (The CompareExchange is not super intuitive since it compares the first and third parameters...)
if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) != ConfigurationRetrieverRunning)
{
_ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
}
Logic:
If the state is idle (is not running),
then set state to running and call the update
If the state is running
then don't do anything.
at the end of Update method, we correctly reset the state to idle
Line 317 in e8e9ecf
Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle); |
The internal _syncAfter field controls if metadata is refreshed (http call).
The value was set to the minimum of AutomaticRefreshInterval and RefreshInterval.
RefreshInterval controls the interval that must pass for RequestRefresh() to make an http call.
RefreshInterval is usually a much smaller timespan, so in effect many more calls would be made to obtain metadata.
This fix only uses the AutomaticRefreshInterval to set _syncAfter.
This has the side effect of extending the time for the next AutomaticRequest, however, that makes sense.
#2866