So I've been going through this blog post about fixing an issue with unsound MutexGuars: Sync. Before I started reading the post I thought that the fix would be to simply make mutex guard !Sync but the actual fix was a cosntrained Sync implementation.
Here is the current Send/Sync implementation for MutexGuard:
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> !Send for MutexGuard<'_, T> {}
#[stable(feature = "mutexguard", since = "1.19.0")]
unsafe impl<T: ?Sized + Sync> Sync for MutexGuard<'_, T> {}
I understand the !Send part because each thread has to lock the mutex and keep the guard to itself.
However I don't understand how allowing Sync can be useful or even safe.
UPD: this however doesn't (and shouldn't) work playground
1 Like
You should put trait bounds on your implementation; otherwise your implementation won't be sound.
I must be blind 
.
Btw, in order to implement Sync, it must also be Send. From the docs:
- A type is Sync if it is safe to share between threads (T is Sync if and only if
&T is Send).
Sync determines whether it is safe to send a shared reference of a type to another thread (T: Sync basically equivalent to &T: Send). Since the only thing you can do with a shared reference to a MutexGuard is to get a shared reference to its contents it is hence safe for it to be Sync as long as the inner type is also Sync (notice the required T: Sync!). In comparison, the bug in the blog post was caused by the bound T: Sync not being present.
3 Likes
Thanks for the response. Hm, I suppose in the example from the blog post it was crucial that the value within the Mutex was a Cell and then we didn't have to have declare mutex guard mutable.
I am not sure if this is related or not but why does the first scope works but the second doesn't?
use std::sync::Mutex;
use std::thread;
fn main() {
let m = Mutex::new(10);
let mut g = m.lock().unwrap();
thread::scope(|s| {
s.spawn(|| {
let x = *g;
});
});
thread::scope(|s| {
s.spawn(|| {
*g = 10;
});
});
}
fails with
15 | s.spawn(|| {
| ----- ^-
| | |
| ___________|_____within this `{closure@src/main.rs:15:17: 15:19}`
| | |
| | required by a bound introduced by this call
16 | | *g = 10;
17 | | });
| |_________^ `MutexGuard<'_, i32>` cannot be sent between threads safely
UPD: before today I wouldn't even thought of sharing a mutex guard between threads
Cell: !Sync, so that's why it is not safe to share it across threads. The bug described in the blog post explains that it was only necessary for T: Send to allow the guard to be Sync. And because Cell: Send, it was mistakenly allowed.
The guard itself being mutable does not matter: Rust Playground
edit: Your first thread has a closure that captures &MutexGuard, and the second one captures &mut MutexGuard.
This is where you’re wrong already. The fact that MutexGuard is !Send is actually hard to understand intuitively, and in a Rust-only world should be entirely unnecessary. This is because there is no soundness issues (in terms of unsynchronized access to the contained value T in the Mutex<T>) that could ever be possible if MutexGuard was made Send (given T: Send). If you send the guard to a different thread, you still have the property that only one thread can access it at the same type.
The !Send restriction actually comes from an entirely different direction: Some operating systems do simply put up the requirement that “the same thread that locked a mutex must also unlock it”. It’s merely the fact that Rust’s Mutex type, wrapping OS mutexes directly, must provide an interface that is correct on all operating systems, so conservatively the !Send restriction applies; if you take a non-OS-based mutex implementation, e.g. a spin lock, this kind of conservative restriction becomes unnecessary.
Since Sync implementation does not allow sharing ownership of the MutexGuard, it’s not a problem here, as you cannot use shared by-&-reference access to end up dropping the MutexGuard from a different thread.
Now, the safety-requirements that do come up when you think about soundness intra-rust, i.e. to access the T inside the MutexGuard<T> correctly, would be the following: T: Send for MutexGuard<Send> and T: Sync for MutexGuard<T>: Sync. These are because MutexGuard<T> is essentially like a unique mutable reference to T, i.e. like a &mut T, and this is exactly the behavior of &mut T: &mut T: Send requires T: Send, and &mut T: Sync requires T: Sync.
2 Likes
Thanks for your response.
what I am trying to understand is if the current Sync impl prevents data races
also regarding your playground example. the guard itself is not passed to rayon worker threads here right?
let m = Mutex::new([0; 20]);
let mut g = m.lock().unwrap();
g.par_iter_mut().chunks(2).for_each(|mut slice| {
*slice[0] += 1;
});
The !Send restriction actually comes from an entirely different direction: Some operating systems do simply put up the requirement that “the same thread that locked a mutex must also unlock it”.
I wasn't clear enough but this was exactly my intuition
Yes, you are right. I think my example is not really relevant. 
What I wanted to show is that sometimes you want to allow a thread that has acquired a lock to be able to share its contents with other threads. It is safe to pass the guard to child threads with a shared reference to it. And because shared references are definitionally immutable there can be no data races by doing so.
Ah, alright. Then, still, does your question about “if the current Sync impl prevents data races” boil down to understanding why T: Sync suffices for &mut T: Sync (which can be an interesting detail to look at and ponder in and by itself) or is the analogy between &mut T and MutexGuard<T> not clear? Or do you fear that MutexGuard<T>: Sync could otherwise be used to violate the extra requirement from operating systems that “the same thread that locked a mutex must also unlock it”?
I suppose we could start with this bit
Since Sync implementation does not allow sharing ownership of the MutexGuard , it’s not a problem here, as you cannot use shared by-& -reference access to end up dropping the MutexGuard from a different thread.
can "dropping hte MutexGuard" here be replaced with "mutate the guarded value"?
Sure, let’s start there. The concern of mutating the guarded value was, in my mind, boiling down to the MutexGuard<T> === &mut T analogy, which is only broken by the extra OS requirement of unlocking from the same thread.
If we talk &mut T, then shared by-&-reference access to a &mut T reference does in fact not allow mutable access to the value T. It’s the “classic” case of & &mut T not allowing mutation of T, and the same applies to &MutexGuard<'_, T> as well.
2 Likes
ok I think I got it now. this additional T: Sync bound makes sure we don't implement Sync for mutexes over interior-mutability types (which typically are not Sync) and this way we can pass a shared reference to a mutex guard down to some child threads not fearing for any data races to happen
In fact, this case is a common source of confusion for multiple reasons. For example, it does not only not allow any mutation, but also restrict the immutable access further than & & T does. This becomes apparent with lifetimes. If you have a shorter lifetime 's and a longer lifetime 'l, then you can use &'s &'l T to get &'l T access. (The simplest argument as to why this is fine is probably: “you can just copy the &'l T”.) But from &'s &'l mut T, you can only derive a shorter access &'s T, not &'l T. It’s not like “& &mut T access gets demoted to & & T” but in fact & & T gives more power, at least in general. Taking lifetimes into account, &'s &'l mut T is equivalent to &'s &'s T (to the point that you can arguably transmute between the two).
Intuitively speaking, this is because the &mut T inside of the & &mut T still says “my original owner would like to have his/her exclusive access (to the T value) back after my lifetime ends” whereas in & & T the owner of the & T is fine keeping only shared access (to the T value).
The same thing applies to MutexGuard<'l, T>. If you have &'s MutedGuard<'l, T>, you only get &'s T, because after the lifetime-'s borrow ends, the owner of the MutexGuard needs back their ability to mutably access T, or to unlock the mutex by dropping the guard.
4 Likes
Is the lifetime parameter in MutexGuard covariant or invariant?
Should be covariant; why are you asking?
This whole discussion was super interesting, thanks for educating us!
I was wondering about a slightly tangential question: What is the point of passing &MutexGuard instead of just &T? It seems the only thing you can do with the guard is call deref and get the &T. I can only think of two things:
- It allows you to call a method that has a &MutexGuard parameter. Again I'm not sure why the parameter wouldn't be &T, but perhaps its just weird and is not under your control.
- I guess some guard types might have other info attached.
2 Likes
Good question! I don’t think there’s any point in passing &MutexGuard<T>.
However, this doesn’t necessarily mean there aren’t any other consequences of MutexGuard<T>: Sync that can prove useful.
I suppose there could probably be use-cases where a struct containing a MutexGuard<T> could benefit from being Sync. I.e. for struct Foo<'a>(SomethingElse, MutexGuard<'a, Bar>, a &Foo<'_> reference also allows access to the SomethingElse. (And API / lifetime considerations could prevent putting &'a T or &'a mut T into Foo<'a> instead.)
Maybe there’s even more useful consequences I’m not thinking of at the moment.
In any case, there’s always the argument of avoiding any unnecessary restrictions, so keeping the Sync implementation as non-restrictive as soundly possible seems desirable. I suppose, also &MutexGuard<T> can sometimes be more convenient in case the reference is created in a closure capture. You skip the need to write the extra step of let reference = &*mutex_guard; before the closure.
3 Likes
This is a good enough reason to do it. We can't think of everything in advance.
I should have written the type parameter but mostly just curiosity.