fix(FIR-46131): Atomic authentication mechanism and token expiry #142
Conversation
ptiurin
changed the title
| } | ||
|
|
||
| // No cached token, acquire write lock and authenticate | ||
| await this.acquireWriteLockAndAuthenticate(); |
There was a problem hiding this comment.
This function already does all the above inside of it. Can we just say authenticate == this.acquireWriteLockAndAuthenticate?
There was a problem hiding this comment.
authenticate gets read lock to fetch the token then falls back to acquireWriteLockAndAuthenticate if looks like cached token needs to be refreshed. It's not equivalent since write lock prevents any other reads and read lock allows multiple locks to be acquired as long as there's no write lock.
Using acquireWriteLockAndAuthenticate would mean we're essentially introducing a bottleneck here.
There was a problem hiding this comment.
Ok I see. It's just the logic duplication confuses me. Can we either
- Only acquire writeLock in
acquireWriteLockAndAuthenticateafter we've validated that no cache entry is present? - Don't check cache in that function at all and let outer function manage it
At this point the fact that we check cache in multiple places seems confusing to me
There was a problem hiding this comment.
We have to check the cache twice. Once - under read lock. If it's not cached then we need to acquire write lock and write to it. However, there could be two "threads" at the same stage here and if one acquires a write lock first then it will refresh the token. There's no need for another "thread" to do the same since "thread" 1 has already done so. But we're under the same stage - past the initial check and before the authentication refresh, so after we acquired a write lock we check again that no other "thread" has refreshed the token while we were waiting on a lock.
This efficiency is especially crucial when we have customers that have 600 query/second bursts and if we're doing an auth each time it will grind to a halt.
| } | ||
|
|
||
| private async tryGetCachedToken(): Promise<string | undefined> { | ||
| async reAuthenticate(): Promise<void> { |
There was a problem hiding this comment.
Can this function just be replaced by adding a skipCache parameter to acquireWriteLockAndAuthenticate? Or just use it under the hood? Since currently a lot of logic seems to be duplicated in these two
There was a problem hiding this comment.
It does a different thing, acquireWriteLockAndAuthenticate checks cache then performs auth, while this function clears cache under write lock and does auth. We can add a flag that would mean either cache is cleared or fetched, but I think it will hurt the readability - it will be another layer of ifs.
There was a problem hiding this comment.
Taking into account my previous confusion, WDYT about removing the cache check from acquireWriteLockAndAuthenticate to leave it a "single responsibility", and just use it in other functions which will take care of the cache?
|
stepansergeevitch
left a comment
stepansergeevitch
left a comment
There was a problem hiding this comment.
The logic makes sense. Approving to deliver the fix, but we should consider improving the readability later
Improving authentication mechanism.
Key Changes: