IdentityModel / IdentityModel.OidcClient

Certified C#/NetStandard OpenID Connect Client Library for native mobile/desktop Applications (RFC 8252)
Apache License 2.0
599 stars 175 forks source link

Optimize Token Refresh Synchronization in RefreshTokenDelegatingHandler to Reduce Unnecessary Lock Contention #436

Open BartoGabriel opened 5 months ago

BartoGabriel commented 5 months ago

The current implementation of RefreshTokenDelegatingHandler uses a SemaphoreSlim to synchronize access to the access and refresh tokens. This synchronization strategy results in every token read being subjected to a lock, potentially leading to performance bottlenecks in high-concurrency environments.

Suggested Enhancements

To address these issues, two main enhancements are proposed:

  1. Implement ReaderWriterLockSlim for Token Access: Replace the SemaphoreSlim with a ReaderWriterLockSlim, which allows multiple concurrent reads of the access token without blocking, while still ensuring that token writes (refreshes) are exclusive and thread-safe. This change could significantly improve read performance by reducing contention. Potential Issue with ReaderWriterLockSlim: While this approach reduces read contention, it may still lead to multiple queued write requests if many threads simultaneously detect an expired token.

  2. Single Active Refresh Operation: Implement a mechanism that ensures only one refresh operation is queued or in progress at a time. This could be achieved by:

    • Introducing a timestamp or flag that marks the initiation of a refresh operation.
    • Before entering a write lock to refresh the token, check whether another thread has already started the refresh process.
    • If a refresh is in progress, other threads should wait or poll until the refresh is completed before retrying their requests with the new token.

Benefits

leastprivilege commented 5 months ago

This library is for native client applications like iOS apps - not exactly what I would call a "high-concurrency environment". Am I missing something?

BartoGabriel commented 5 months ago

I realize I may have stretched the term "high-concurrency environment" a bit far. I wanted to point out that even though we're talking about native client applications, there are scenarios where multiple API calls are made simultaneously. For example, mobile apps that fetch data from several endpoints to display on a dashboard or to dynamically fill combo boxes.

In such cases, we might need to make more than five concurrent calls. I believe that blocking a thread every time the access token is accessed can be costly, especially since we're applying a continuous lock for an action (token renewal) that occurs very rarely in the application's lifecycle.

I was thinking of looking for a way to minimize this cost and reserve the locks only for when the token is being renewed, which happens less frequently. I know we're talking about a few milliseconds, but I think optimizing this could improve efficiency in certain scenarios.

leastprivilege commented 5 months ago

Thanks! Assigning Joe for consideration