Fix concurrent cache access in AcquireToken method#578
Conversation
|
| httpClient: shared.DefaultClient, | ||
| retryPolicyEnabled: true, | ||
| source: source, | ||
| canRefresh: &zero, |
There was a problem hiding this comment.
Also remove the declaration of zero
| for err := range errors { | ||
| t.Error(err) | ||
| } |
There was a problem hiding this comment.
Consider this approach instead. If there were 100 identical errors, would you want to see all of them in go test output?
|
|
||
| // Mock client should only need to respond once if caching works correctly | ||
| mockClient := mock.NewClient() | ||
| // Add multiple responses in case caching fails (but we'll verify it doesn't) |
There was a problem hiding this comment.
if you simply appended 1 response the mock would panic when it gets a second request and you wouldn't have to count the requests yourself
| return | ||
| } | ||
|
|
||
| // Capture the token received |
There was a problem hiding this comment.
why? There can only be one token because the mock client returns a static value, and you don't need this to know whether the goroutine succeeded because if it didn't it would have written an error to the channel
| c.authParams.Scopes = []string{resource} | ||
|
|
||
| cacheAccessorMu.Lock() | ||
| defer cacheAccessorMu.Unlock() |
There was a problem hiding this comment.
The lock is held for the entire duration of AcquireToken, including the c.getToken() HTTP call on the refresh path. This means all goroutines calling AcquireToken are fully serialized globally — if IMDS is slow or times out (up to 30s), every other goroutine blocks.
The original canRefresh CAS was more surgical: it only gated concurrent refreshes; cache hits were still concurrent. Consider holding the lock only around cache reads and writes, not across the network call:
cacheAccessorMu.RLock()
stResp, err := cacheManager.Read(...)
ar, err := base.AuthResultFromStorage(stResp)
cacheAccessorMu.RUnlock()
if needsRefresh {
tr, er := c.getToken(ctx, resource) // no lock held during HTTP
if er == nil {
// re-acquire write lock to update cache
return tr, nil
}
}|
|
||
| // cache never uses the client because instance discovery is always disabled. | ||
| var cacheManager *storage.Manager = storage.New(nil) | ||
| var cacheAccessorMu *sync.RWMutex = &sync.RWMutex{} |
There was a problem hiding this comment.
cacheAccessorMu is declared as *sync.RWMutex but Lock() (exclusive write lock) is always called — RLock() is never used. This gives no benefit over a plain sync.Mutex and adds confusion. Either switch to sync.Mutex:
var cacheAccessorMu sync.Mutexor use RLock/RUnlock for the read-only cache lookup path and reserve Lock only for writes (which also addresses the HTTP-hold concern above).
|
Design note: global mutex serializes across all Client instances Both This is a pre-existing consequence of |
|
4 participants
PR: Fix Concurrent Cache Access in
AcquireTokenMethodSummary
This PR addresses a potential race condition in the
AcquireTokenmethod by properly synchronizing access to the cache using a mutex. Issue number #569Changes
cacheAccessorMu.Lock()/defer cacheAccessorMu.Unlock()around cache operations to ensure thread-safe access.canRefreshatomic variable, which was previously used to prevent concurrent refreshes which is no longer needed as whole cache is in mutexTesting
TestAcquireTokenConcurrencyunit test to ensure correct behavior under concurrent conditions.Breaking Changes