Safe access to a `#[thread_local]` should be disallowed

[alexcrichton]

Today this code compiles just fine, but it probably shouldn't:

#![feature(thread_local)]
#[thread_local]
static FOO: uint = 3;

fn main() {
    let a = &FOO;
    spawn(proc() {
        println!("{}", a);
    });
}

I'm not nominating this because thread_local is behind a feature gate, just wanted an issue to track it.

[alexcrichton]

Another route would just be disallowing borrows as well, similar to how we only allow borrows of statics in statics.

[thestinger]

It could be allowed if it gave borrows something like a 'task lifetime instead of 'static.

[arielb1]

Where is the unsoundness there? uint implements Sync, so sending a reference to it to a subtask is completely fine (unless the inner task can outlive the outer task, in which case the proc shouldn't be :'static and the spawn should be illegal).

[alexcrichton]

The lifetime 'static means that it outlives the entire program, but the lifetime of a thread-local variable is just for the thread that it's running on, which doesn't necessarily outlive the entire program. This means that interior pointers can be sent to other threads which become stale once the sending thread exits.

[thestinger]

It seems like it would be really easy to add 'task and keep it behind the same feature gate to make this sound. Eventually, #[thread_local] will be available on all relevant platforms and it could be used directly to handle simple cases rather than using wrappers. It's missing C++11 style destructor support so it isn't able to completely eliminate the need for a higher-level API.

[thestinger]

@alexcrichton: It also permits data races now that internal mutability in a static is permitted.

[eddyb]

Do we need 'task? Couldn't we have a NoSend wrapper around &'static T, implementing Deref?

[thestinger]

@eddyb: It needs 'task to be sound as an unwrapped static variable. It's necessary to expose a proper &T reference regardless.

[arielb1]

@thestinger

Preventing data races is a job for Sync (rather than Send), which (for some weird reason) #[thread_local] seem to require.

@eddyb
Having values that are 'static but not really does not seem like a good idea to me – it's fighting against the lifetime system.

[thestinger]

Preventing data races is a job for Sync (rather than Send), which (for some weird reason) #[thread_local] seem to require.

A #[thread_local] static requiring Sync is a recently introduced bug. It's not a mitigating factor for this bug, which does allow data races in the absence of that other bug.

[pythonesque]

I don't think adding a new 'task lifetime is actually that helpful for thread-locals. If they are Send, and are using fork-join concurrency, 'task means something different in a child thread from a parent thread, so a child 'task might outlive a parent 'task--how do you disambiguate? Does Rust know when the new 'task lifetime is created in the child so it can reason about this soundly? If it does, and they have different lifetime names, why use 'task at all and not just a regular lifetime?

On the other hand, if you can't Send them, just make them NoSend and have done with it. The only problem with making thread locals NoSend is that then you can't send them between tasks in fork-join style APIs. If you can't resolve the issues above, there's probably no sane way of determining that anyway so you might as well ban it (and really, I think it's a pretty niche usecase to want shared thread local data).

I don't think semantics are a good argument here. There are lots of examples of things that aren't "semantically" 'static, like data in Rcs (which can never outlive its calling thread) that nonetheless get along without special lifetimes.

[thestinger]

The point of 'task is that it would be like 'static but would not be Send.

I don't think adding a new 'task lifetime is actually that helpful for thread-locals. If they are Send, and are using fork-join concurrency, 'task means something different in a child thread from a parent thread, so a child 'task might outlive a parent 'task--how do you disambiguate? Does Rust know when the new 'task lifetime is created in the child so it can reason about this soundly? If it does, and they have different lifetime names, why use 'task at all and not just a regular lifetime?

It would be trivial to add it, and it doesn't need to know anything about other tasks.

On the other hand, if you can't Send them, just make them NoSend and have done with it. The only problem with making thread locals NoSend is that then you can't send them between tasks in fork-join style APIs. If you can't resolve the issues above, there's probably no sane way of determining that anyway so you might as well ban it (and really, I think it's a pretty niche usecase to want shared thread local data).

It's possible to get a reference to the value inside the thread-local variable, and it should be considered as having a 'static lifetime but without being considered Send. It will be possible to do this regardless of how you wrap it. It's possible to restrict the lifetime with a context object (like RefCell) but that's overly strict.

I don't think semantics are a good argument here. There are lots of examples of things that aren't "semantically" 'static, like data in Rcs (which can never outlive its calling thread) that nonetheless get along without special lifetimes.

That's not the point here. The Rc type isn't a form of static variable that needs to be provided by the compiler.

[pythonesque]

There is already a way to identify 'statics that are not Send, NoSend. As far as I know, it has all the properties you're looking for. What is the purpose of adding something functionally identical? Am I missing some way in which it would or could conceivably work differently?

[thestinger]

It's not functionally identical. It should be possible for a thread-local static to contain Cell<u32> and to obtain an &Cell<u32> reference to it with a static lifetime, but without it being sendable. The NoSend marker only helps when you're creating a new type, which is not able to express the same things.

[thestinger]

There's already a feature gate for thread_local and it makes sense to fix the soundness issues with it behind that feature gate. It makes sense to have macros wrapping it in the short-term before the compiler implementation is solid / sound and fully portable, but eventually it would be nice to have an implementation that's on par with C++ and D without the need for macro hacks. Using macros / wrappers reduces the expressiveness and makes it more painful to use.

In C++, you just mark a global variable as thread_local and everything just works including lazy initialization on first access and destructors in reverse order of the lazy initialization. There's no reason this can't be implemented in Rust with macros for the near future and eventually by just fixing the edge cases in the compiler feature.

[pythonesque]

Okay, I see your objection now, that makes sense.

[thestinger]

Also, the lifetime would likely be called 'thread_local or 'thread, not 'task.

[thestinger]

Anyway, until Rust no longer has to worry about 32-bit iOS and old versions of Android it's a dead end. It doesn't really matter if it's safe or not at the moment because it's just an implementation detail for a safe library making use of it. I'm just pointing out that Rust could have TLS that's as easy to use as it is in C++11 and D rather than doing it via library hacks.

[steveklabnik]

Updated code:

#![feature(thread_local)]
#[thread_local]
static FOO: usize = 3;

fn main() {
    let a = &FOO;
    let jg = std::thread::spawn(move || {
        println!("{}", a);
    });

    jg.join().unwrap();
}

[bltavares]

The updated code still compiles on rustc 1.9.0-nightly (c9629d61c 2016-03-10), when it shouldn't.

Code used:

#![feature(thread_local)]
#[thread_local]
static FOO: usize = 3;

fn main() {
    let a = &FOO;
    let jg = std::thread::spawn(move || {
        println!("{}", a);
    });

    jg.join().unwrap();
}

[eddyb]

Coming back to this: I think I've come to accept the simpler solution of not giving TLS variables a specific named lifetime but rather the outermost scope of the current function.

That would require banning references from any other statics too.

[alexcrichton]

label: T-lang

[alexcrichton]

er, meant to add that as a tag...

[nikomatsakis]

Removing nomination as it doesn't seem that important to resolve this imminently. It'd be good to have a summary comment though of major points in the thread.

[nikomatsakis]

triage: P-medium

[nagisa]

Short summary:

• Is still a problem, thread-local accesses gain the 'static lifetime;
• Need 'task/'thread/etc lifetime, which is equal to (or smaller than) lifetime of the thread which accesses the thread-local;
• NoSend-equivalent is not a solution, because the static may contain some internal fields of which you could still take a 'static reference of (i.e. Something(Cell<T>) → &'static Cell<T> and send that reference);
• Giving the TLS data accessed the function’s outermost scope lifetime would work¹;

¹: Can’t freely use (e.g. return out of function which accesses the TLS) the reference within the thread either, though?

[Mark-Simulacrum]

So since it seems we don't want the 'thread lifetime (judging by comments on this RFC: rust-lang/rfcs#1705, should we close this as well? It seems we can't implement this without that RFC's thread lifetime, and as such it's not worth keeping this open. Arguably, maybe #[thread_local] should be deprecated and removed in favor of thread_local!, though I don't know the differences between the two.

[eddyb]

I still want the semantics that make thread-local accesses like function arguments: can't let them escape.
In @nagisa's list, it's the last option. IMO the only one we can be certain of soundness for (or at least it allows no more than what thread_local! already does) and it's fairly straight-forward to implement.
If you want you can assign to me (famous last words).

[pythonesque]

To clarify: IMO it's still important to be able to be able to use thread locals directly, since thread_local! does have significant overhead by comparison.

[nikomatsakis]

@pythonesque do you have benchmarks to back that up? IIRC, @alexcrichton gathered various numbers that seemed to suggest the opposite, but I may be mis-remembering.

[stjepang]

@nikomatsakis Can confirm that thread_local! is slow.

I'm implementing my own channels (similar to std::sync::mpsc). When selecting over multiple channels, thread locals are accessed several times. I'm timing 2 million channel selects:

• With thread_local!: 200 ms
• With #[thread_local]: 130 ms

The difference is significant enough to be a real-world problem.

[alexcrichton]

Yes there's no debate that thread_local! has overhead, a simple example looking at the assembly and the IR will show that. What I gathered previously is that in all use cases we wanted thread_local! for in the standard library it had non-measurable overhead.

To actually quantify what's going on here, the #[thread_local] version of TLS keys initializes itself with None and like all other thread_local! variables it's lazily initialized. This forces a branch on all accesses and stores of the TLS variable of "am I initialized yet?" The raw #[thread_local] version is obviously initialized as part of the static, so this check does not exist. This means that direct #[thread_local] will have fewer branches with it.

I do not think we should strive to stabilize #[thread_local]. It's incredibly not portable which maeks it not too useful for most software. We should strive to improve thread_local!. The feature to implement is for the compiler to understand whether the initialization expression is a constant expression or not. If it's a constant expression then we can bypass the None storage and "am I initialized checks", making it equivalent to raw #[thread_local]

[stjepang]

I do not think we should strive to stabilize #[thread_local]. It's incredibly not portable which makes it not too useful for most software. We should strive to improve thread_local!.

Ok, since #[thread_local] isn't going to be stabilized, its only purpose is to allow faster implementation of thread_local! on some platforms. Can we then fix the soundness issue by making any access to a #[thread_local] unsafe and call it a day?

The feature to implement is for the compiler to understand whether the initialization expression is a constant expression or not.

I like this idea. What would it take to implement this today, or at least what would be the first step forward?

[eddyb]

Can we then fix the soundness issue by making any access to a #[thread_local] unsafe and call it a day?

I'll just open a PR with my solution, it shouldn't be hard at all...

The feature to implement is for the compiler to understand whether the initialization expression is a constant expression or not.

That's easy in the compiler but thread_local! is not implemented in the compiler.

[arielb1]

That's easy in the compiler but thread_local! is not implemented in the compiler.

... But we could add an eager_thread_local! macro for that case.

[alexcrichton]

@stjepang yeah right now #[thread_local] is just an implementation detail essentially of the thread_local! macro. Making all access to a #[thread_local] static would be fine by me, although I would personally still not advocate for its stabilization, even if this bug were fixed.

As for how to implement a "const expr detection" in a macro I'm not entirely sure. We could either move the implementation into the compiler (which I'd prefer to avoid) or take @arielb1's suggestion of a new macro or a variant of the current macro's syntax.

For example we could "perverse" the meaning via: thread_local!(const A: i32 = 3); where static in the macro means "lazily initialized, but any expression valid" and const means "must be a constant expression". I don't think this is a good idea, but an example of what we might do.

[stjepang]

Thanks @eddyb for fixing this issue!

Can we create a new issue for the optimization part (thread locals initialized by a constant expression should bypass the "am I initialized" check on every access)?

[eddyb]

@stjepang Sure, but I'm not sure how to do that cleanly to be honest.