Fix implementation of Sync for SpinLock

[mlodato517]

This reasoning:

rust_atomics_and_locks/src/spin_lock/mod.rs

Lines 37 to 40 in 8a5e3b7

	// SAFETY: If we're sharing the lock then the other thread can't get a `T` - only a `&/&mut T` so
	// we just need `T` to be `Sync`. The locking mechanism ensures that two shared versions don't get
	// mutable access concurrently.
	unsafe impl<T> Sync for SpinLock<T> where T: Sync {}

is incorrect. The book mentions:

As a precaution, UnsafeCell does not implement Sync, which means that our type is now no longer shareable between threads, making it rather useless. To fix that, we need to promise to the compiler that it is actually safe for our type to be shared between threads. However, since the lock can be used to send values of type T from one thread to another, we must limit this promise to types that are safe to send between threads. So, we (unsafely) implement Sync for SpinLock for all T that implement Send, like this:

unsafe impl<T> Sync for SpinLock<T> where T: Send {}

Note that we don’t need to require that T is Sync, because our SpinLock will only allow one thread at a time to access the T it protects. Only if we were to give multiple threads access at once, like a reader-writer lock does for readers, would we (additionally) need to require T: Sync.

[mlodato517]

This is confusing me because, I believe, if we share &SpinLock with another thread, then that thread can only call .lock() on it. This can get it a SpinLockGuard which can only deref to &T or &mut T. Neither of these are T. So I'm clearly missing something with regards to:

the lock can be used to send values of type T

[mlodato517]

I imagine a test for this would be:

1. Wrap a Sync-but-not-Send type in my SpinLock
2. Share my SpinLock with a thread
3. See if miri hates it

MutexGuard is Sync and !Send so I tried:

#[test]
fn test_sync_bound() {
    let other_data = std::sync::Mutex::new(Vec::new());
    let m_guard = other_data.lock().unwrap();
    let x = SpinLock::new(m_guard);
    std::thread::scope(|s| {
        s.spawn(|| x.lock().push(1));
        s.spawn(|| {
            let mut g = x.lock();
            g.push(2);
            g.push(2);
        });
    });
    let g = x.lock();
    assert!(g.as_slice() == [1, 2, 2] || g.as_slice() == [2, 2, 1]);
}

But Miri was cool with that so maybe that test isn't validating what I hoped it would.

It's good to note that Mutex has the book's "Sync where T: Send" bounds so I'm definitely wrong - I just don't know why yet 😅

[mlodato517]

Hmmm maybe I understand. Sync implies the type can be shared by two threads concurrently. Because of the lock, no two threads will have share the stored value concurrently. So we don’t need Sync.

Send still feels wrong but I guess I’m maybe seeing why Sync is overkill. I feel like the lock is Sync if a reference to the inner data is Send… but that’s the same as Sync 😅Maybe the number of concurrent accessors is more important than the owner? That also doesn’t “feel” right…

[mlodato517]

For tomorrow - https://stackoverflow.com/a/68708557 might clear some things up.

[mlodato517]

Okay the vibe I'm getting is that, if a value of type T is initialized on one thread then:

1. T: Send means T, &T, and/or &mut T can be safely accessed on another thread
2. T: Sync means that &T can be safely accessed by multiple threads concurrently

The docs say:

Types that can be transferred across thread boundaries.

I used to think "transferring a type across thread boundaries" meant "transferring ownership of a type". Now I think it includes sending, for example, *const T to access &T.

So, the situation with our SpinLock is:

1. the stored T will be accessible on another thread (as a &T or &mut T) so it must be Send
2. no two threads will be accessing &T concurrently so it need not be Sync

[mlodato517]

@andrewhalle brings up a good point - being able to send &mut T is a kind of ownership transfer because you can use, for example, std::mem::swap to get and drop an owned T. That being said, we believe ownership transfer shouldn't be the mental model behind Send.