[Messaging] Stop Unnecessary Refresh of JWT Tokens #23129
Conversation
jsquire
added
Service Bus
Event Hubs
Client
jsquire
requested review from
christothes,
conniey,
schaabs,
serkantkaraca and
JoshLove-msft
| string[] requiredClaims) => | ||
| Credential switch | ||
| { | ||
| _ when Credential.IsSharedAccessCredential => await AcquireSharedAccessTokenAsync().ConfigureAwait(false), |
There was a problem hiding this comment.
We cannot be sure that a shared key was not rolled/updated within the credential, which would would invalidate our cached token ahead of its expiration, so we don't want to cache these. The shared access credentials also manage the token lifetime and extend when appropriate, so the only benefit of caching would be to avoid a small number of allocations in a background task doing network IO.
| /// | ||
| private async Task<CbsToken> AcquireJwtTokenAsync() | ||
| { | ||
| var token = Volatile.Read(ref _cachedJwtToken); |
There was a problem hiding this comment.
AMQP authorization is refreshed only when a token is due to expire, a token was rejected, or a new connection/link is being established, most often taking place on a background thread. Because this isn't on a performance-critical path called during every network operation, ensuring a longer-lived token is more valuable than quick token acquisition for overall throughput.
Rather than doing something more complicated, like returning a short-lived token and requesting a fresh token in the background, I believe that we're better served by requesting the token on-demand and just synchronizing to ensure we don't make redundant requests. As a result, this is intentionally simple, just using the good old lock+double-check pattern.
|
/azp run net - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConnectionScope.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/CbsTokenProvider.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/CbsTokenProvider.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/CbsTokenProvider.cs
Outdated
Show resolved
Hide resolved
|
@JoshLove-msft: I'm planning on not merging this until after the August release, so that I've got time to run the suite of long-running stress tests. Any concerns with this not making it in immediately? |
SGTM |
jsquire
force-pushed
the
messaging/cache-token
branch
from
de15c63 to
547f67d
Compare
The focus of these changes is to cache JWT tokens used for authorization of AMQP connections and links to avoid making unnecesary requests due to network changes while the token is not near expiration. For features like Service Bus sessions, links can be opened/closed frequently, leading to a non-trivial amount of requests to acquire tokens that are likely to remain valid for the duration of their use. Also included is the addition of a small amount of random jitter when calculating the time that AMQP authorization should be resent; this is intended to reduce contention when a new token needs to be acquired.
jsquire
force-pushed
the
messaging/cache-token
branch
from
547f67d to
26f9b2c
Compare
Summary
The focus of these changes is to cache JWT tokens used for authorization of AMQP connections and links to avoid making unnecessary requests due to network changes while the token is not near expiration. For features like Service Bus sessions, links can be opened/closed frequently, leading to a non-trivial amount of requests to acquire tokens that are likely to remain valid for the duration of their use.
Also included is the addition of a small amount of random jitter when calculating the time that AMQP authorization should be resent; this is intended to reduce contention when a new token needs to be acquired.
References and Related