-
Notifications
You must be signed in to change notification settings - Fork 401
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
Use int to guard when metadata update is due #2988
base: dev
Are you sure you want to change the base?
Conversation
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.
I need a little more time with this, but this is my first pass.
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.TestUtils/EventDrivenConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Show resolved
Hide resolved
bffe52c
to
7e58f8e
Compare
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.
I want to make sure @pmaytak gets a chance to look at this, but this is looking good.
SummarySummary
CoverageMicrosoft.IdentityModel.Abstractions - 70.2%
Microsoft.IdentityModel.JsonWebTokens - 80%
Microsoft.IdentityModel.Logging - 74.9%
Microsoft.IdentityModel.Protocols - 89.2%
Microsoft.IdentityModel.Protocols.OpenIdConnect - 81.4%
Microsoft.IdentityModel.Protocols.SignedHttpRequest - 93.4%
Microsoft.IdentityModel.Protocols.Tests - 0%
Microsoft.IdentityModel.Protocols.WsFederation - 80.9%
Microsoft.IdentityModel.TestUtils - 86.7%
Microsoft.IdentityModel.Tokens - 76.4%
Microsoft.IdentityModel.Tokens.Saml - 71.1%
Microsoft.IdentityModel.Tokens.Tests - 4.6%
Microsoft.IdentityModel.Validators - 95.8%
Microsoft.IdentityModel.Xml - 79.3%
System.IdentityModel.Tokens.Jwt - 83.8%
System.IdentityModel.Tokens.Jwt.Tests - 20.1%
|
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Show resolved
Hide resolved
8eec2f6
to
172519f
Compare
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
Outdated
Show resolved
Hide resolved
d1d31c6
to
54b2cfb
Compare
54b2cfb
to
75fd4d3
Compare
private TimeSpan _lastKnownGoodLifetime = DefaultLastKnownGoodConfigurationLifetime; | ||
private BaseConfiguration _lastKnownGoodConfiguration; | ||
private DateTime? _lastKnownGoodConfigFirstUse; | ||
internal Random _random = new(); |
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.
Can be private
and static
?
internal int _timeInSecondsWhenLastRefreshOccurred; | ||
internal int _timeInSecondsWhenLastRequestRefreshWasRequested; |
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.
Internal variables should be cased as TimeInSecondsWhenLastRefreshOccurred
, TimeInSecondsWhenLastRequestRefreshWasRequested
.
internal double _automaticRefreshIntervalInSeconds = DefaultAutomaticRefreshInterval.TotalSeconds; | ||
internal double _maxJitter = DefaultAutomaticRefreshInterval.TotalSeconds / 20; | ||
private TimeSpan _refreshInterval = DefaultRefreshInterval; | ||
internal double _requestRefreshIntervalInSeconds = DefaultRefreshInterval.TotalSeconds; |
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.
Internal variables should be cased as AutomaticRefreshIntervalInSeconds
, MaxJitter
, RequestRefreshIntervalInSeconds
.
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 with a %5 random jitter. |
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.
// Seconds since the BaseConfigurationManager was created when the last refresh occurred with a %5 random jitter. | |
// Seconds since the BaseConfigurationManager was created when the last refresh occurred with a 5% random jitter. |
} | ||
|
||
/// <summary> | ||
/// Incremented each time <see cref="RequestRefresh"/> results in a http request. |
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.
/// Incremented each time <see cref="RequestRefresh"/> results in a http request. | |
/// Incremented each time <see cref="RequestRefresh"/> results in a HTTP request. |
internal DateTimeOffset StartupTime { get; set; } = DateTimeOffset.UtcNow; | ||
|
||
/// <summary> | ||
/// Incremented each time <see cref="GetBaseConfigurationAsync(CancellationToken)"/> results in a http request. |
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.
/// Incremented each time <see cref="GetBaseConfigurationAsync(CancellationToken)"/> results in a http request. | |
/// Incremented each time <see cref="GetBaseConfigurationAsync(CancellationToken)"/> results in a HTTP request. |
/// <summary> | ||
/// Incremented each time <see cref="GetBaseConfigurationAsync(CancellationToken)"/> results in a http request. | ||
/// </summary> | ||
internal long _numberOfTimesAutomaticRefreshRequested; |
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.
Currently we have to remember to use Interlocked when incrementing the variable. Can this be made private
and another method be added to increment this? Something like
internal void IncrementNumberOfTimesAutomaticRefreshRequested() => Interlocked.Increment(ref NumberOfTimesAutomaticRefreshRequested);`
This way we for sure will use Interlocked when incrementing and it won't be possible to set this variable to some other value.
Same comment for _numberOfTimesRequestRefreshRequested
.
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.
Likewise, can we expose methods/properties for accessing these values externally? Something like this (variables are probably wrong but this general idea):
public long NumberOfTimesAutomaticRefreshRequested => Interlocked.Read(ref _numberOfTimesAutomaticRefreshRequested);
The previous guard was a DateTimeOffset which could be in an indeterminate state when being read.
This PR records the DateTimeOffset when ConfigurationManager is instantiated StartupTime.
The next refresh (automatic or requestRefresh) is an offset in seconds from the StartupTime.
An int is used to store the offset of the last refresh.
If a service runs over 68 years, int will overflow, we could have some complicated logic to avoid this, it seems ok to leave it as is.
We also could have performed a simple calculation of:
NumberOfTimesAutomaticRefreshRequested * _automaticRefreshIntervalInSeconds > (StartupTime - UtcNow).TotalSeconds
We add a small amount %5 in a random manner when calculating the next interval, adds complexity.
We added internals to aid in testing.