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

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

Open
BartoGabriel opened this issue Jun 25, 2024 · 3 comments
Assignees

Comments

@BartoGabriel
Copy link

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

  • Improved Read Performance: By allowing multiple concurrent reads, applications can achieve lower latency and better scalability.
@leastprivilege
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

leastprivilege commented Jun 27, 2024

Thanks! Assigning Joe for consideration

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

No branches or pull requests

3 participants