unsafe: ScopedKey allows for Sync-ification of non-Sync data

[Manishearth]

There is an unsafe impl<T> Sync for KeyInner<T> { } in the scoped TLS module, which allows for sharing of data that wasn't meant to be shared.

An unqualified unsafe impl<T> Sync for Foo { } should only be used when Foo provides some sort of threadsafe locking mechanism. KeyInner and ScopedKey do not.

This lets us, for example, clone an Rc or RefCell between threads, in safe code:

scoped_thread_local!(static RC: Rc<u8>);
fn main() {
 RC.set(&Rc::new(1), ||{
     let y = &RC;
     let guard = scoped(|| {
        println!("Started 1");
        y.with(|slot|{
            for i in 1..10 {
                println!("Cloned in 1");
                slot.clone();
                sleep_ms(10);
            }
        });
        println!("Ended 1");
     });
     println!("Started 0");
     y.with(|slot| {
            for i in 1..10 {
                println!("Cloned in 0");
                slot.clone();
                sleep_ms(10);
            }
     });
     println!("Ended 0");
     guard.join();
 });
}

playpen

Here, we have Rc clones from different threads being interleaved, which can cause unsafety if there's a race on the refcount. We could also cause unsafety with a RefCell, or any other non-Sync type. ScopedKey basically lets us share &T across threads even if T: !Sync. I put the sleeps in there because somehow without them the routines are run serially.

Note that this is not a problem with the scoped() API. I'm using scoped() here because to exploit &T: Send, T:!Sync one needs to be able to share across threads, and Arc has an additional Send bound which ScopedKey doesn't satisfy. Any usable scoped API will probably have to make the assumption that &T: Send iff T:Sync. Nor is this a problem with Rc, since it can be done with RefCell, too.

Solution: Make it unsafe impl<T: Sync> Sync for KeyInner<T> { }

... I think. I'm not familiar with this API, and I'm not sure why it's even Sync in the first place.

[Manishearth]

cc @huonw @aturon @nikomatsakis @pythonesque

[huonw]

cc @alexcrichton

This seems concerning: this is designed to be thread-local, so that e.g. storing a Cell works (which would be safe, if it were guaranteed to stay on one thread). However static ...: T currently requires T: Sync unconditionally, hence this overly-accepting impl.

[Manishearth]

Qualifying the impl with T: Sync breaks scoped TLS for Cell (see the associated PR); because ScopedKey becomes Sync no longer and thus can't be stuffed inside a static.

[eddyb]

I believe the only way #[thread_local] statics can be safe is by requiring the absence of Sync.
That may sound odd, but consider the following:

• T: Sync => &T: Send
• sending thread-local references across threads is memory unsafe even with no interior mutability
• a #[thread_local] static with no interior mutability is almost entirely useless, certainly not a common case
• non-Sync containers offering interior mutabilities are cheaper and preferred to Sync ones, where they can be used

The main issue is actually within the libstd TLS implementation: LocalKey is a wrapper for getters so it's unaffected by it, but ScopedKey may live in a #[thread_local] static or a global one, depending on whether the platform natively supports TLS or not.

Send-ing a &'static ScopedKey across threads is actually safe on the architectures which do not have native TLS support.
However, allowing it on some architectures and not on others is undesirable.

Should ScopedKey use an accessor model like LocalKey?
Then the actual borrow of a #[thread_local] static is an implementation detail and happens on the thread where the key is used, not where it was itself borrowed.

[pythonesque]

"I believe the only way #[thread_local] statics can be safe is by requiring the absence of Sync."

This is wrong, I think. For example, one might use thread local data for intrusive linked lists of hazard pointers. They would be thread local but might still be Sync (one can guarantee existence with scoped threads or in certain circumstances where threads are bound to cores). That said you can obviously come up with other representations. In any case, I've mostly come around to believing that we need a separate thread_local lifetime (not just for this, but for things like ensuring that catch_panic can't catch an error in a thread that changes thread local data, violating exception safety).

[Kimundi]

Based on what @eddyb wrote, it seems like the TLS interface requires a static that only allows non-Sync data and is still accessible like one.

One idea I had of how to enable this in todays Rust is to make scoped_thread_local!() generate a unit like struct instead of a static. That struct would be !Sync and have a Deref impl:

// Today:
RC.set(...); // autoref of static "RC" lvalue because:

scoped_thread_local!(static RC: Rc<u8>);
// = expands to =>
static RC: ScopedKey<Rc<u8>> = ...;

// With this change:
RC.set(...); // autoref of local temporay "RC" rvalue because

scoped_thread_local!(static RC: Rc<u8>);
// = expands to =>
struct RC;
impl Deref<Target=Rc<u8>> for RC { 
    static mut ... // can have a !Sync in a static mut
    ... 
}
impl !Sync for RC {}

According to @eddyb, this could also help with LocalKey, but I'm not familiar enough with the TLS API in general to really reason about this.

[eddyb]

@Kimundi I was expecting the Deref impl to target an intermediary type, not the contents themselves.
ScopedKey and LocalKey have their own APIs to deal with.

@pythonesque You would still be about to use the TLS API provided by libstd with thread-safe data, it's just the low-level #[thread_local] which would have the requirement.

Ideas of thread-local lifetimes predate the removal of 'static from Send, and I'm not sure how relevant they are anymore.
I'm also conflating thread-safe data access and thread lifetime... but how long does a thread live?
What would &'thread_local T unify with?

Actually, there's an... easy solution for this. Somehow we were blinded by language semantics, but their details are fluid.
It is trivial to make borrows of #[thread_local] behave like borrows of an arbitrary rvalue, allowing the resulting reference to be passed down, but not returned.

That would ensure that the the thread is just as alive for such borrows as it is for stack ones.
ScopedKey would still require a facade like LocalKey has, because it may not be Sync when native TLS is supported.
But both their APIs are compatible with such a change.

[pythonesque]

Wouldn't that limit the usability of thread locals in some cases, though? e.g., wouldn't that make it difficult to have a thread local point to another thread local? Maybe I just have to think about it.

(BTW, I'm aware we're just talking about the low-level API :)).