Yes, of course. If you need the extra capabilities that it offers you over AtomicBool, then you should use it.
6 Likes
Or, if using it increases the chances the future you, the one performing maintenance, will understand the code, then use it.
A corollary is "premature optimization is the root of all evil". When Mutex<bool> is a good choice AtomicBool is an optimization.
2 Likes
Alternatively, if you use the mutex when the atomic is enough, future you might wonder why the extra capabilities of the mutex are necessary.
Since the atomic option is more restrictive, it can express intent more clearly. Mutex has 4 states: locked + true, locked + false, unlocked + true, unlocked + false. This is not always necessary.
8 Likes
I didn't look too deeply at your example you posted, but when used correctly, mutexes should capture the data that requires mutual exclusion.
In your example, it seems like you're using the mutex to protect self.inner_txn, when the mutex should contain inner_txn.
IMO if it's difficult for some reason to put your data inside the mutex, then you should just use Mutex<()> (and keep closed as it's own field) as this will signal to the reader that the mutual exclusion involves more than just what is contained inside the mutex
3 Likes
Interesting point! And I think this is also the reason why an AtomicBool would be unsound in my example, I guess: I'm protecting not only the bool, but also require synchronization of whether self.inner_txn is dangling (if I overlook my own example correctly right now).
Related topic: Can I use a Mutex<()> to avoid conflicts when accessing an UnsafeCell?
1 Like
It's not just about the state of the mutex. It's also about its implicit synchronization guarantees (see above link regarding Mutex<()>). These guarantees are not really documented well in my opinion.
That brings me back to @FRGH's comment:
I wonder: Aren't we often requiring synchronization that goes beyond whats actually inside the mutex?
For example in the original thread, @Heliozoa proposed to use an (async) Mutex<bool>:
Here the mutex also synchronizes not just the bool but also whatever "// do stuff" does. Yet I feel like using a Mutex<()> and a separate bool would be rather confusing than helpful (edit: and, moreover, not even possible with safe Rust; edit2: except if we make the Bool an AtomicBool 
or wrap it in yet another Mutex 
).
Of course you could also use a dedicated synchronization primitive like @alice proposed here, but I still feel like it's okay (and idiomatic) to rely on Mutex<T> synchronizing more than T[1]. Not sure though.
As I pointed out above when writing "not even possible with safe Rust", it will not work unless I make the Bool and AtomicBool or wrap it in yet another Mutex or use UnsafeCell:
@@ -331,12 +331,12 @@ impl<'a> TxnBackend<'a> {
fn close_cursors(&mut self) {
for cursor in take(&mut self.cursors).iter() {
// NOTE: mutex should never be poisoned
- let mut closed = cursor.closed.lock().unwrap();
- *closed = true;
+ let guard = cursor.mutex.lock().unwrap();
+ cursor.closed = true;
unsafe {
lmdb::mdb_cursor_close(cursor.inner);
}
- drop(closed);
+ drop(guard);
}
}
}
@@ -542,7 +542,8 @@ impl<K: ?Sized, V: ?Sized, C> Eq for Db<K, V, C> {}
#[derive(Debug)]
struct CursorBackend {
inner: *mut lmdb::MDB_cursor,
- closed: Mutex<bool>,
+ mutex: Mutex<()>,
+ closed: bool,
inner_txn: *mut lmdb::MDB_txn,
}
@@ -552,20 +553,21 @@ unsafe impl Sync for CursorBackend {}
impl CursorBackend {
/// Panic if cursor handle does not belong to transaction
fn assert_txn_backend(&self, txn_backend: &TxnBackend) {
- let closed = self.closed.lock().unwrap();
- if *closed || self.inner_txn != txn_backend.inner {
+ let guard = self.mutex.lock().unwrap();
+ if self.closed || self.inner_txn != txn_backend.inner {
// Avoid mutex poisoning by dropping mutex guard before panicking
- drop(closed);
+ drop(guard);
panic!("cursor does not belong to transaction");
}
- drop(closed);
+ drop(guard);
}
}
impl Drop for CursorBackend {
fn drop(&mut self) {
// NOTE: mutex should never be poisoned
- if !*self.closed.get_mut().unwrap() {
+ self.mutex.get_mut().unwrap();
+ if !self.closed {
unsafe {
lmdb::mdb_cursor_close(self.inner);
}
@@ -1576,6 +1578,7 @@ fn new_cursor<K, V, C>(&mut self, db: &Db<K, V, C>) -> io::Result<Cursor<K, V, C
let inner_cursor = inner_cursor.assume_init();
let cursor_backend = Arc::new(CursorBackend {
inner: inner_cursor,
+ mutex: Default::default(),
closed: Default::default(),
inner_txn: self.inner,
});
Compiler errors:
error[E0594]: cannot assign to data in an `Arc`
--> src/lib.rs:335:13
|
335 | cursor.closed = true;
| ^^^^^^^^^^^^^^^^^^^^ cannot assign
|
= help: trait `DerefMut` is required to modify through a dereference, but it is not implemented for `Arc<CursorBackend>`
For more information about this error, try `rustc --explain E0594`.
error: could not compile `mmtkvdb` due to previous error
Thus I feel like using Mutex<bool> is the best choice here, even if synchronization also covers the raw pointer(s) in addition to the bool.
-
even if
Tis distinct from()↩︎
I have been thinking about this again, and wondered how I could use an AtomicBool instead.
Note that the following is just an exercise for me, and I don't really plan to replace the Mutex with an atomic. I merely do this to understand the implications of atomics better.
My idea is, I could do this:
@@ -330,13 +333,11 @@ struct TxnBackend<'a> {
impl<'a> TxnBackend<'a> {
fn close_cursors(&mut self) {
for cursor in take(&mut self.cursors).iter() {
- // NOTE: mutex should never be poisoned
- let mut closed = cursor.closed.lock().unwrap();
- *closed = true;
+ cursor.closed.store(true, Ordering::Release);
+ let _ = cursor.closed.load(Ordering::Acquire); // we need this
unsafe {
- lmdb::mdb_cursor_close(cursor.inner);
+ lmdb::mdb_cursor_close(cursor.inner); // to avoid this being reordered before the store
}
- drop(closed);
}
}
}
@@ -542,7 +543,7 @@ impl<K: ?Sized, V: ?Sized, C> Eq for Db<K, V, C> {}
#[derive(Debug)]
struct CursorBackend {
inner: *mut lmdb::MDB_cursor,
- closed: Mutex<bool>,
+ closed: AtomicBool,
inner_txn: *mut lmdb::MDB_txn,
}
@@ -552,20 +553,15 @@ unsafe impl Sync for CursorBackend {}
impl CursorBackend {
/// Panic if cursor handle does not belong to transaction
fn assert_txn_backend(&self, txn_backend: &TxnBackend) {
- let closed = self.closed.lock().unwrap();
- if *closed || self.inner_txn != txn_backend.inner {
- // Avoid mutex poisoning by dropping mutex guard before panicking
- drop(closed);
+ if self.closed.load(Ordering::Acquire) || self.inner_txn != txn_backend.inner {
panic!("cursor does not belong to transaction");
}
- drop(closed);
}
}
impl Drop for CursorBackend {
fn drop(&mut self) {
- // NOTE: mutex should never be poisoned
- if !*self.closed.get_mut().unwrap() {
+ if !self.closed.load(Ordering::Relaxed) { // no other threads have access now
unsafe {
lmdb::mdb_cursor_close(self.inner);
}
Note that:
- I added an
let _ = cursor.closed.load(Ordering::Acquire);to prohibitlmdb::mdb_cursor_closefrom being ordered before thestore. If I understand it right, then even astore(true, Ordering::SeqCst);would not make sure thatlmdb::mdb_cursor_closedoesn't get ordered before that. [1] - I assume that in the
Drophandler ofCursorBackend, it would be okay to useOrdering::Relaxed, because I already have a&mut self, which is exclusive.
What I would like to show is that simply replacing the Mutex<bool> with an AtomicBool is counterintuitive, because unless I take special precautions, operations may be reordered and result in UB.
However, I'm not an expert, and I would like to know if the let _ = cursor.closed.load(Ordering::Acquire); statement is
- necessary
- sufficient
for ensuring that lmdb::cursor_close doesn't get executed too early due to reordering.
Mutex<bool> is great for Condvar blocking, as shown right there in the example, and I know Rayon uses that too.
6 Likes
Use it when you need blocking behavior. For example, if it takes a long time to compute the boolean and you don't want to run this operation multiple times concurrently, then use the mutex.
OTOH if you always want to flip the boolean as quickly as possible without waiting, then use atomic.
2 Likes
I think that a mutex serves more purposes than just allowing blocking. It also acts as some sort of memory fence (which Acq/Rel-atomics do as well, but only in one direction: no operations can be reordered after a store or before a load). That's why I believe using atomics is to be considered dangerous here.
I don't think mutex does anything more magical than atomic. You just need a strong-enough ordering. After all, the atomic aquire-release are mutex-related terms.
2 Likes
Yes, a mutex isn't doing anything that goes beyond atomics (except parking the thread maybe if it's not a spin-lock).
However, *mutex.lock().unwrap() = true is substantially different from atomic.store(true, SeqCst).
Consider:
foo();
let guard = mutex.lock().unwrap();
*guard = true;
drop(guard);
bar();
Here, bar() cannot be reordered before setting the boolean in the mutex to true locking the mutex.
Edit: That is because locking the mutex always performs an acquire-load on its lock-state.
And:
foo();
atomic.store(true, SeqCst);
bar();
Here, bar() could be reordered before the store (if I understand it right), such that we have:
foo();
bar();
atomic.store(true, SeqCst);
See C++ memory order for reasoning.
P.S.: The notion of "reordering" I made in this post is a bit simplified. Actually the synchronization doesn't happen unless you also do an acquire-load later. But my point is that atomic.store(true, SeqCst) doesn't give the same guarantees as *mutex.lock().unwrap() = true does (if you later read the boolean through the mutex as well).
If that was true, then spinlocks and lock-free data structures would be impossible.
Which statement exactly are you referring to by "that"?
(I made a couple of statements.)
To clarify:
This is because it internally uses atomics, of course, but also because of how it uses those atomics. I.e. *mutex.lock().unwrap() = true results (at least) in an aquire-load of the lock-state, followed by writing the boolean, followed by an release-store of the lock-state.
Opposed to atomic.store(true, SeqCst), which is just a single store.
"this" I mean reordering of bar(), as if SeqCst wasn't a fence:
I believe that on x86, this reordering doesn't happen, i.e. x86 guarantees that it's a fence, but this doesn't hold for all platforms (see C++ reference, where the guarantees for an SeqCst-store are listed).
I'm seeing the fence behavior on aarch64 target too.
I would argue that:
- This doesn't mean it holds for any architecture fulfilling the standard.
- This doesn't mean compilers aren't allowed to reorder.
Ultimately, it matters what the Rust docs say about memory ordering, and they refer to the C++ standard.
However, I might misunderstand/misinterpret the C++ standard here.
The Rust docs further say:
[…], a store-load pair of
Ordering::SeqCstoperations synchronize other memory while additionally preserving a total order of such operations across all threads.
Note that it's a store-load pair(!) here, which performs the synchronization.
Sorry, I'm having trouble following your example. Could you give an example of how you'd expect this to affect the behavior as seen by another thread? If there's only one thread, then the compiler must observably behave as-if everything is sequenced as written.
I think it should be something like this:
Thread 1:
foo();
atomic.store(true, SeqCst);
bar();
Thread 2:
if atomic.load(SeqCst) == false {
// may observe effects from `foo` and `bar`
}