Thread local Cell methods. by m-ou-se · Pull Request #3184 · rust-lang/rfcs · Gi...

This feels a bit weird to me, and inconsistent with "desugaring" to with(...) -- can you say more about why we want it?

In particular, set() presumably does run the destructor in general, so the behavior would only be different for the first-time initialization. So we probably can't avoid checking whether the value is initialized before writing to the thread local.

Maybe we can adjust thread local declarations to permit omitting the initialization expression entirely and have set() still work on those, but other methods (like get(), take(), with()) fail?

This feels a bit weird to me, and inconsistent with "desugaring" to with(...) -- can you say more about why we want it?

The alternative is that .set() (the first time on each thread) will run the (possibly expensive) initialization expression, directly drop the resulting value, and then write the value you gave it. With X.with(|x| x.set(..)) that might be somewhat expected (but still easy to miss), but with just X.set(..) that'd be quite surprising.

So we probably can't avoid checking whether the value is initialized before writing to the thread local.

That is true. But we can avoid running the initialization expression, which can be expensive.

Maybe we can adjust thread local declarations to permit omitting the initialization expression entirely and have set() still work on those, but other methods (like get(), take(), with()) fail?

With the proposed changes, you can achieve that with:

thread_local! {
    static ID: Cell<i32> = panic!("ID not set on this thread!");
}

If this pattern is used often, we can consider making the panic implicit when the initialization expression is missing:

// Maybe?
thread_local! {
    static ID: Cell<i32>; // Will panic when used before `.set()`'ing it.
}

With X.with(|x| x.set(..)) that might be somewhat expected (but still easy to miss), but with just X.set(..) that'd be quite surprising.

Huh. I guess maybe our model for the desugaring is different: the way I saw it is that X desugars to X.with(|x| x).set(...), basically akin to a lazy static, which implies that the result here is not surprising, and indeed is the expected behavior.

I also see the "initialization expression" being run as not something that happens in set(...) but sort of separately -- this obviously doesn't match what actually happens in our current implementation.

Maybe it makes sense to move and/or also have a separate initialize(...) on LocalKey<T> that has this behavior? It seems like we actually sort of always have a cell-like primitive here tracking the initialization regardless, so that should be possible, and lets the user get this behavior if they want it without complicating set to have separate codepaths/mental models for already initialized and not initialized thread locals.

I also see the "initialization expression" being run as not something that happens in set(...) but sort of separately -- this obviously doesn't match what actually happens in our current implementation.

This is not just an implementation detail of our current implementaion, but a guarantee we already make in the documentation:

"Initialization is dynamically performed on the first call to with within a thread"

So yes, a thread_local!{} is already basically a 'Lazy' / 'OnceCell'.*

(*Except those with a const { .. } initializer, which are currently unstable. See rust-lang/rust#84223 (comment))

a separate initialize(...) on LocalKey

Yeah maybe, and is_initialized() etc. too. However, right now, if the initializer is just a constant (and doesn't need Drop), we could compile it into a thread local that's not lazily but directly/always initialized, since the difference is not noticable. Adding .set() as proposed here will not break that. Adding a .initialize() will make the difference observable, since .initialize() would then fail or succeed depending on if the optimization is made, or we'd always have to track the 'is_initialized' state even when that would otherwise not be necessary.

(And even if we have .initialize() on all (non-const (?)) thread locals, I'd still like .set() on a thread local Cell without unnecessarily initializing and instantly dropping a value before setting the value you want to set. Unlike all the other functions (including .with, .set is the only one that doesn't give you access to the stored value, so initializing it first seems quite useless. And there are good use cases for skipping it, such as the panic!() example above.)

I guess we can maybe add this to unresolved questions? I think skipping user-written code on particular API calls feels like a footgun, since e.g. maybe there's validation of some kind there (can only be used on particular threads, or some such). Obviously the user maybe shouldn't have written that, but it feels iffy to just bypass user-written code like this.

I feel like the panic example is illustrative of why this is potentially not good: if the user actually wants that behavior, we should encourage use of something like the suggested initialization APIs, with a std-provided panic on with/get/etc, rather than having the user write that.

In particular, it feels weird that an API like set(...) has different behavior if the value in the thread local is not initialized vs. is. I think it'll probably be fine in most cases, but I don't think it buys us much for the potential harm it causes. Anyone actually needing that kind of behavior seems to want a more explicit initialization step, I'd imagine, rather than an arbitrary set call being sufficient...