-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
sync: add mem::forget to RwLockWriteGuard::downgrade. #2957
Conversation
Currently when `RwLockWriteGuard::downgrade` the `MAX_READS - 1` permits are added to the semaphore. When `RwLockWriteGuard::drop` gets invoked however another `MAX_READS` permits are added. This results in releasing more permits that were actually aquired when downgrading a write to a read lock. This is why we need to call `mem::forget` on the `RwLockWriteGuard` in order to avoid invoking the destructor. Fixes: tokio-rs#2941
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.
Thanks 👍 Looks like the loom test file is not included (so not running?).
@@ -76,3 +76,24 @@ fn concurrent_read_write() { | |||
assert_eq!(10, *guard); | |||
}); | |||
} | |||
#[test] | |||
fn downgrade() { |
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.
It does not look like loom_rwlock.rs
is being included here.
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.
It is not. I was not sure whether this is intentional or not. Will include
Also, maybe this PR should be submitted against 0.2 and then forward ported. |
Sorry for the delay, I will come back to this soon. Working on v0.3.1 atm. |
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.
Thanks 👍
Btw, should we call out the deadlock in the docs and explain that it would deadlock because it is fair? |
@carllerche yes we should. Just need to think of a succinct way to put it. :) |
Motivation
Currently when
RwLockWriteGuard::downgrade
theMAX_READS - 1
permits are added to the semaphore. When
RwLockWriteGuard::drop
gets invoked however another
MAX_READS
permits are added. Thisresults in releasing more permits that were actually aquired when
downgrading a write to a read lock.
Solution
This is why we need to call
mem::forget
on theRwLockWriteGuard
in order to avoidinvoking the destructor.
Additionally doc test for
downgrade
was not correct as it can hang in the situation where after downgrading all available permits are assigned to the waiting writer before the additional reader gets the change to obtain a permit.Fixes: #2941