-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
Fix deadlock in token reloading #830
Conversation
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution makes sense to me, but I can't seem to repro the original issue...
@clux Or we could create a separate 0.69 branch that we cherry-pick this merge into. |
yeah, that would be even better actually. |
Ok, got a repro running, and this seems to fix it. |
Fix deadlock in token reloading
Fix deadlock in token reloading Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
bearer_header(token) | ||
} else { | ||
bearer_header(token_file.write().await.token()) | ||
bearer_header(locked.token()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this can lead to the deadlock explained in the docs?
Note that under the priority policy of
RwLock
, read locks are not granted until prior write locks, to prevent starvation. Therefore deadlock may occur if a read lock is held by the current task, a write lock attempt is made, and then a subsequent read lock attempt is made by the current task.Returns an RAII guard which will drop this read access of the
RwLock
when dropped.
I thought the RwLockReadGuard
from .read().await
is dropped before the else
block :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So did I, to be honest.. :/
lock at one point before entering the branch to prevent two locks being attempted