Tracking issue for thread_local_state stabilization

[aturon]

This feature allows you to query a thread-local variable for its state: http://static.rust-lang.org/doc/master/std/thread/enum.LocalKeyState.html, which will then tell you what may happen if you try to read from the variable.

The functionality is somewhat niche, but occasionally essential. The APIs appear to be in pretty decent shape as-is.

cc @alexcrichton

[aturon]

Nominating for stabilization in 1.6.

[alexcrichton]

The libs team decided to not move this into FCP, some worries around this being:

• Checking the state and then accessing hits TLS twice, which may be too slow for some use cases.
• This is very similar to RefCell::borrow_state which has also not yet been stabilized. It's likely that the enum here wants to move to boolean accessors so we can add more states in the future.

[alexcrichton]

Although I nominated this for 1.8, libs decided to remove the nomination as I forgot that the reason this is not in FCP yet is because you hit TLS twice when querying the state.

[apasel422]

I think this would be better expressed like RefCell::try_borrow.

[PlasmaPower]

See also: rust-lang/rfcs#2030

[PlasmaPower]

PR for try_with (similar to RefCell::try_borrow) wrapped into this feature: #43158

[ghost]

The documenation for LocalKey::state says:

A key is initially in the Uninitialized state whenever a thread starts. It will remain in this state up until the first call to with within a thread has run the initialization expression successfully. Once the initialization expression succeeds, the key transitions to the Valid state which will guarantee that future calls to with will succeed within the thread.

But then it also mentions:

Keys may remain in the Destroyed state after destruction has completed. Keys without destructors (e.g. with types that are Copy), may never enter the Destroyed state.

Do we really want to make the first guarantee? Is it not reasonable for Copy types to skip the Uninitialized state altogether and start right off in the Valid state?

[joshlf]

+1 to that.

[alexcrichton]

@stjepang sounds reasonable to me!

[ghost]

Checking the state and then accessing hits TLS twice, which may be too slow for some use cases.

This issue should be resolved now that we have LocalKey::try_with.

This is very similar to RefCell::borrow_state which has also not yet been stabilized. It's likely that the enum here wants to move to boolean accessors so we can add more states in the future.

borrow_state was rejected in favor of try_borrow.
Apart from Unitialized, Valid, and Destroyed state, what may the additional states even be? Is this still a concern? If not, are we ready to kick off FCP?

[joshlf]

Apart from Unitialized, Valid, and Destroyed state, what may the additional states even be? Is this still a concern?

In #43491, I proposed adding a fourth Initializing state between Uninitialized and Valid. The idea was to allow detection of initialization loops. My particular goal was to support allocators (e.g., elfmalloc relies on crossbeam, which in turn allocates, etc), and I opened PR #43550, but it was eventually closed when we discovered that there was no good way to transition keys into Initializing before performing any allocation on platforms that don't support the #[thread_local] attribute. Instead, I updated the documentation (#44396) to clarify that recursively-dependent initializers are not supported.

[ghost]

Until we decide what to do with LocalKeyState, could we stabilize the try_with method only?
I don't see any blockers for that.

[Mark-Simulacrum]

Nominating. I propose that we stabilize the LocalKey::try_with method and deprecate the LocalKey::state method and associated enum. I don't see a good usecase for the enum given this method.

cc @rust-lang/libs

[alexcrichton]

I agree with @Mark-Simulacrum's assessment and would be ok stabilizing try_with!

@rfcbot fcp merge

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @KodrAus
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period is now complete.

[ghost]

I regret stabilizing LocalKey::try_with with the current signature. :(

So the signature of LocalKey::try_with is:

pub fn try_with<F, R>(&'static self, f: F) -> Result<R, AccessError>
where
    F: FnOnce(&T) -> R;

We should've gone with the following instead:

pub fn try_with<F, R>(&'static self, f: F) -> R
where
    F: FnOnce(Result<&T, AccessError>) -> R;

Why? I have a situation with this helper function:

pub fn with_context<F, R>(f: F) -> R
where
    F: FnOnce(&Arc<Context>) -> R,
{
    CONTEXT.try_with(|cx| f(cx)).unwrap_or_else(|_| f(&Context::new()))
}

Unfortunately, the helper function doesn't compile:

error[E0382]: capture of moved value: `f`
   --> src/internal/context.rs:162:53
    |
162 |     CONTEXT.try_with(|cx| f(cx)).unwrap_or_else(|_| f(&Context::new()))
    |                      ----                           ^ value captured here after move
    |                      |
    |                      value moved (into closure) here
    |
    = note: move occurs because `f` has type `F`, which does not implement the `Copy` trait

So F has to be FnMut or additionally Copy, which is annoying.

Had we gone with the other signature for LocalKey::with, this would be easy to do:

pub fn with_context<F, R>(f: F) -> R
where
    F: FnOnce(&Arc<Context>) -> R,
{
    CONTEXT.try_with(|cx| {
        match cx {
            Ok(cx) => f(cx),
            Err(..) => f(&Context::new()),
        }
    })
}