ESD-32688: Improve locking and blocking associated with key retrieval #225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lock for refresh Previously, whenever the cached JWKS expired there would be a block until the request has completed and the cache has been updated. Now, when the JWKS cache expires the refresh is triggered in the background and the cached JWKS will continue to be returned until the background refresh has been completed. If that background refresh succeeds then the cache will be updated, if it errors then the cached JWKS will be deleted and on the next call of KeyFunc the JWKS refresh will be attempted again, which (if the error persists) would raise the error. This now means that the only time KeyFunc will block is in the instance where there is no JWKS for an issuer, when a cached JWKS expires we will no longer block
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #225 +/- ##
==========================================
+ Coverage 94.78% 95.98% +1.19%
==========================================
Files 7 7
Lines 307 324 +17
==========================================
+ Hits 291 311 +20
+ Misses 12 10 -2
+ Partials 4 3 -1 ☔ View full report in Codecov by Sentry. |
adamjmcgrath
approved these changes
Nov 30, 2023
This was referenced Dec 1, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Checklist
🔧 Changes
This PR consists of 2 commits.
The first improves the locking of the KeyFunc by moving to a RWMutex, calling
RLockin KeyFunc to allow multiple readers when retrieving the cached value and only calling theLockmethod when we need to refresh the JWKS.The second moves to refreshing the JWKS in the background when the CacheTTL is hit rather than blocking until the JWKS has been refreshed. Previously, whenever the cached JWKS expired there would be a block until the request has completed
and the cache has been updated. Now, when the JWKS cache expires the refresh is triggered in the background and the cached JWKS will continue to be returned until the background refresh has been completed. If that background refresh succeeds then the cache will be updated, if it errors then the cached JWKS will be deleted and on the next call of KeyFunc the JWKS refresh will be attempted again, which (if the error persists) would raise the error.
This now means that the only time KeyFunc will block is in the instance where there is no JWKS for
an issuer, when a cached JWKS expires we will no longer block
📚 References
#194
🔬 Testing
Have tested this with the sample app/in repo example and is covered by tests