-
Notifications
You must be signed in to change notification settings - Fork 336
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
setTokenHeader is not thread-safe #168
Comments
andreyvit
changed the title
setTokenHeader is not thread-safe
Apr 23, 2020
setTokenHeader
is not thread-safe
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hey, I'm reading the source code prior to using this library in our app, and found a memory model violation when running in token mode.
The problem is with
setTokenHeader
:Here, a write to
c.Token.Bearer
(insidec.Token.GenerateIfExpired
call) is protected by a mutex, but the corresponding read (withinr.Header.Set(...)
call) is unprotected.This causes 2 problems:
There is no happens-before relationship between writes to
c.Token.Bearer
and reads of it from other threads.Because
string
is larger than a single machine word, unsynchronized reads and writes may result in a partially-assigned value to be read.This means that, if there's a constant multithreaded load, after refreshing the token the app might panic, or use the expired token, or use an incorrect token.
I have three choices of recommended fixes. Please pick the one you like.
Option 1, a minimal fix:
c.Token
for the duration ofr.Header.Set
call above.GenerateIfExpired
locks the mutex, all other method, notablyGenerate
, does not).Option 2, a better fix:
Token.GenerateIfExpired
, it should never have messed with the mutex.GenerateIfExpired in
setTokenHeader`, keeping the mutex locked for the entire duration of the call.Token
APIs should be synchronised via its mutex.Option 3, if you don't care about keeping nuanced backwards compatibility of token API:
Token.GenerateIfExpired
.setTokenHeader
call.Token
APIs should be synchronised via its mutex.I'll be happy to send a PR for any of these.
The text was updated successfully, but these errors were encountered: