Implement RwLock::{try_read, try_write}
#72
Conversation
jamesbornholt
force-pushed
the
rwlock_try_lock
branch
from
606b431
to
a8308e4
Compare
| // Check that the only nonzero clock entries are for the threads in the set | ||
| check_clock(|i, c| (c > 0) == set.contains(&i)); | ||
| assert!(!set.contains(&1)); // dummy thread is never in the set | ||
| check_clock(|i, c| !set.contains(&i) || (c > 0)); |
There was a problem hiding this comment.
Not clear why you weakened this check from an iff to an implication?
There was a problem hiding this comment.
It's to handle race condition described at the top of the test—readers can have a non-zero clock without being in the set.
tests/basic/rwlock.rs
Outdated
| /// third that peeks at the value using `try_read.` The `write` must succeed, while each `try_write` | ||
| /// may or may not succeed. Thus we expect to see final values: | ||
| /// * 0 (`try_read` ran first) | ||
| /// * 1 (both `try_writes`s fail), |
There was a problem hiding this comment.
But the reader may see a final value of 1 even if one (or both) try_writes were successful (if the reader went before the try_writes). So this comment is misleading.
One thing you could do is to read the value again after the joins, and then look at all the pairs of values (value_read_by_try_read, value_read_after_joins).
There was a problem hiding this comment.
Good point. I updated the test to check all the possible pairs of values.
| /// easily diagnose potential deadlocks, especially with async tasks that might migrate across | ||
| /// threads in real implementations. | ||
| #[test] | ||
| fn double_try_read() { |
There was a problem hiding this comment.
There's a variant of this where a thread does a read() and then a try_read()
There was a problem hiding this comment.
I figured this would be the same as if the first try_read succeeds (assuming that a successful try_read and a read are equivalent), but may as well add the explicit test anyway.
This is a little tricky because `std` is unclear about whether a thread can acquire the same read lock multiple times. For `read` it says: > This function might panic when called if the lock is already held by > the current thread. So acquiring a second read lock _might_ fail. But for `try_read` it says: > This function will return the WouldBlock error if the RwLock could not > be acquired because it was already locked exclusively. suggesting that `try_read` _must_ succeed the second time (the lock is not held exclusively). We resolve this ambiguity by choosing a conservative semantics that always forbids a thread acquiring the read lock twice. This helps us catch deadlocks, especially in async programs where a task might nondeterministically migrate between threads and only deadlock if that migration didn't happen. Another difficulty with this change is that causality is pretty hairy for the read side of a `RwLock`. In principle, concurrent readers shouldn't inherit each other's causality, as they don't affect whether the lock was readable or not. But that's really hard to implement, especially with `try_write` in the picture too. So again we choose a conservative implementation for our vector clocks that just always inherits causality from all prior lock holders. This is likely too strong, so we'd explore unnecessary symmetries, but I can't convince myself of the correctness of a weaker implementation.
jamesbornholt
force-pushed
the
rwlock_try_lock
branch
from
a8308e4
to
f9f97d3
Compare
| let _ = rx.recv().unwrap(); | ||
| rx.recv().unwrap(); |
There was a problem hiding this comment.
This is a Clippy 1.62 thing.
This is a little tricky because
stdis unclear about whether a threadcan acquire the same read lock multiple times. For
readit says:So acquiring a second read lock might fail. But for
try_readit says:suggesting that
try_readmust succeed the second time (the lock isnot held exclusively).
We resolve this ambiguity by choosing a conservative semantics that
always forbids a thread acquiring the read lock twice. This helps us
catch deadlocks, especially in async programs where a task might
nondeterministically migrate between threads and only deadlock if that
migration didn't happen.
Another difficulty with this change is that causality is pretty hairy
for the read side of a
RwLock. In principle, concurrent readersshouldn't inherit each other's causality, as they don't affect whether
the lock was readable or not. But that's really hard to implement,
especially with
try_writein the picture too. So again we choose aconservative implementation for our vector clocks that just always
inherits causality from all prior lock holders. This is likely too
strong, so we'd explore unnecessary symmetries, but I can't convince
myself of the correctness of a weaker implementation.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.