Well, since you can build a spin-lock with an atomic, I'm wrong here. I guess my point is that you can't use an AtomicBool in the same way as you can use a Mutex<bool> and achieve the same synchronization. But I'm still not overlooking this issue very well. There was a longer discusson on it in Better understanding atomics, but it's still confusing me each time I'm thinking about it.
I do believe that Mutex<bool> as well as Mutex<()> (see also RawMutex trait, or parking_lot::RawMutex type) are not anti-patterns per-se.
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.
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
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).
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.
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 @aliceproposed 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.
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 prohibit lmdb::mdb_cursor_close from being ordered before the store. If I understand it right, then even a store(true, Ordering::SeqCst); would not make sure that lmdb::mdb_cursor_close doesn't get ordered before that. [1]
I assume that in the Drop handler of CursorBackend, it would be okay to use Ordering::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.
See memory_order_releasehere, which only guarantees that "no reads or writes in the current thread can be reordered after this store"; and performing a store with memory_order_seq_cst is the same as using memory_order_release. ↩︎
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.
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.
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).
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.
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).
[…], a store-load pair of Ordering::SeqCst operations 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.
or wrap it in yet another 
).