Skip to content
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 up-to-date "now", in case take long time to get Metadata #2063

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

RyanLiu99
Copy link
Contributor

@RyanLiu99 RyanLiu99 commented Apr 25, 2023

This is to address "stale now" issue.

This meta data is cached for x seconds to reduce the call to Idp server to get meta data. But to make the cache works, a global lock is imposed cross requests. Cache start time is set before calling to Id server, before waiting for the lock, not after the call returns. I call it a “stale now”. This is a problem when call to Idp server take longer then x seconds. Ocz x seconds has been changes from 30s to 5m from version 6.8 to version 6.27, make it less sever.

When the call to IdentiyProvider has issue, take much longer time, all requests are blocked at this lock.

Once the call eventually returned from Idp server, SOME requests piling at the lock can just take cached data and keep going. But if the call to Id server take longer than cache time x seconds, those requests come later than stale now + x seconds, still need call Id server again even there is fresh cached data. The global lock is blocking 2nd half requests and new requests again. And also impose more stress on already stressed Idp server since it does not wait x seconds and call it right way again. This can get worse and worse.

cc @brentschmaltz

@brentschmaltz
Copy link
Member

@RyanLiu99 thanks for this work.
We finally got it in, look for the change in the next release.

@brentschmaltz brentschmaltz merged commit 4bdfd73 into AzureAD:dev Apr 27, 2023
@RyanLiu99 RyanLiu99 deleted the RyanLiu-ConfigurationManager branch April 28, 2023 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants