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

Update locking and caching for OpenIdConnectCachingSecurityTokenProvider #3124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mderriey
Copy link

@mderriey mderriey commented Nov 2, 2024

Update locking and caching for OpenIdConnectCachingSecurityTokenProvider

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Update locking and caching for OpenIdConnectCachingSecurityTokenProvider

Description

This is a proposed alternative to #3118.

Changes:

  1. Keep a lightweight write lock through Interlocked.Exchange to ensure a single thread calls ConfigurationManager.GetConfigurationAsync at a time.
  2. Threads will not wait to acquire a write lock anymore, and will return existing data if they can't.
  3. Remove read lock on the assumption that that variable assignments and reads are atomic operations for reference types (as per Update locking and caching for OpenIdConnectCachingSecurityTokenProvider #3118 (comment)).

Thanks for offering me to open a PR for this.

Please get as many eyes as you can on this to validate my assumptions. I don't have much confidence in my multi-threading, locking, and synchronization abilities.

Also, feel free to take over that PR to adjust to your coding preferences; one thing I noticed is the code base seems to prefer using Interlocked.CompareExchange over Interlocked.Exchange.
Essentially, this is now yours, although I'll definitely keep an eye on it and answer questions where I can.

Fixes #3078

@msbw2
Copy link
Contributor

msbw2 commented Nov 5, 2024

There's this comment from the other PR: #3118 (comment)

@jennyf19
Copy link
Collaborator

There's this comment from the other PR: #3118 (comment)

@mderriey not sure if you saw this ^

@mderriey
Copy link
Author

There's this comment from the other PR: #3118 (comment)

@mderriey not sure if you saw this ^

I did, yes, I replied further down in the thread.

Alex's comments make sense to me, but I can't comment on whether they're better than what's in this PR.

I opened this PR with the goal of potentially helping other people as much as getting some validation on the changes we've been running in production.

As mentioned in the PR description, I now consider this PR yours. I'm happy to contribute my thoughts, but I won't push in one direction or the other as I wouldn't be confident in my knowledge or experience when it comes to multithreading, thread synchronisation, or locking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants