mderriey commented 3 weeks ago

Update locking and caching for `OpenIdConnectCachingSecurityTokenProvider`

[X] You've read the Contributor Guide and Code of Conduct.
[ ] You've included unit or integration tests for your change, where applicable.
[X] You've included inline docs for your change, where applicable.
[X] 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:

Keep a lightweight write lock through Interlocked.Exchange to ensure a single thread calls ConfigurationManager.GetConfigurationAsync at a time.
Threads will not wait to acquire a write lock anymore, and will return existing data if they can't.
Remove read lock on the assumption that that variable assignments and reads are atomic operations for reference types (as per https://github.com/AzureAD/microsoft-identity-web/pull/3118#issuecomment-2452708548).

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 commented 3 weeks ago

There's this comment from the other PR: https://github.com/AzureAD/microsoft-identity-web/pull/3118#issuecomment-2452916786

jennyf19 commented 2 weeks ago

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

@mderriey not sure if you saw this ^

mderriey commented 2 weeks ago

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.

AzureAD / microsoft-identity-web