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 optimistic synchronization in JsonWebToken.Audiences #2243

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

stephentoub
Copy link
Collaborator

I didn't see anything in Payload.TryGetValue that would require synchronization with a concurrent call to Payload.TryGetValue. And without that, and since concurrent access is not something to optimize for here, we can instead employ optimistic synchronization and just swap in the result with an interlocked. If there's a race condition and two threads do end up accessing Audiences on the same instance concurrently, they may both end up creating the array, but only one will be published for all to consume.

In addition to making Audiences cheaper to access, the primary benefit here is it saves an allocation per JsonWebToken, which no longer needs to construct the audiences lock object.

I didn't see anything in Payload.TryGetValue that would require synchronization with a concurrent call to Payload.TryGetValue. And without that, and since concurrent access is not something to optimize for here, we can instead employ optimistic synchronization and just swap in the result with an interlocked. If there's a race condition and two threads do end up accessing Audiences on the same instance concurrently, they may both end up creating the array, but only one will be published for all to consume.

In addition to making Audiences cheaper to access, the primary benefit here is it saves an allocation per JsonWebToken, which no longer needs to construct the audiences lock object.
@jennyf19 jennyf19 added this to the 7.0.0-preview4 milestone Aug 22, 2023
Copy link
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brentschmaltz brentschmaltz added this pull request to the merge queue Aug 22, 2023
Merged via the queue into AzureAD:dev7 with commit d8f32eb Aug 22, 2023
2 checks passed
@stephentoub stephentoub deleted the audienceslock branch August 23, 2023 00:19
@jennyf19
Copy link
Collaborator

Included in preview4 release

brentschmaltz pushed a commit that referenced this pull request Sep 6, 2023
I didn't see anything in Payload.TryGetValue that would require synchronization with a concurrent call to Payload.TryGetValue. And without that, and since concurrent access is not something to optimize for here, we can instead employ optimistic synchronization and just swap in the result with an interlocked. If there's a race condition and two threads do end up accessing Audiences on the same instance concurrently, they may both end up creating the array, but only one will be published for all to consume.

In addition to making Audiences cheaper to access, the primary benefit here is it saves an allocation per JsonWebToken, which no longer needs to construct the audiences lock object.
brentschmaltz pushed a commit that referenced this pull request Sep 7, 2023
I didn't see anything in Payload.TryGetValue that would require synchronization with a concurrent call to Payload.TryGetValue. And without that, and since concurrent access is not something to optimize for here, we can instead employ optimistic synchronization and just swap in the result with an interlocked. If there's a race condition and two threads do end up accessing Audiences on the same instance concurrently, they may both end up creating the array, but only one will be published for all to consume.

In addition to making Audiences cheaper to access, the primary benefit here is it saves an allocation per JsonWebToken, which no longer needs to construct the audiences lock object.
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.

None yet

5 participants