Tracking issue for thread::Builder::spawn_unchecked

[oliver-giersch]

This issue is for tracking the thread_spawn_unchecked feature (PR #55043).

The feature adds an unsafe method to thread::Builder called spawn_unchecked, which allows spawning a thread without restrictions on the lifetimes of either the supplied closure or its return type. The method is unsafe because the caller has to guarantee that the spawned thread is explicitely joined before any referenced data is dropped. The main use for this feature is as building block for libraries that build safe abstractions around it, such as scoped threads. Currently such libraries have to rely on subversions of the type system/borrow checker.

[jethrogb]

@oliver-giersch what do you think about changing

imp::Thread::new(stack_size, Box::new(main))

to

imp::Thread::new(stack_size, transmute::<Box<FnBox()>, Box<FnBox()>>(Box::new(main)))

This removes the burden of dealing with lifetime fumbling from the native implementations to where the unsafety is actually introduced.

[oliver-giersch]

I'm sorry but I fail to see what this would accomplish, could you explain further?

[jethrogb]

Context: I'm running into issues implementing a new target in std after your recent chanages.

Previously, the implementation was approximately this:

pub fn spawn<F, T>(self, f: F) -> io::Result<JoinHandle<T>> where
    F: FnOnce() -> T, F: Send + 'static, T: Send + 'static
{
    imp::Thread::new(..., Box::new(f))
}

such that the impl Thread in sys could take a Box<FnBox()> (implied static bound).

Now, you have

pub unsafe fn spawn_unchecked<F, T>(self, f: F) -> io::Result<JoinHandle<T>> where
    F: FnOnce() -> T, F: Send, T: Send 
{
    imp::Thread::new(..., Box::new(f))
}

which requires the impl Thread in sys takes a Box<FnBox() + 'a>. However, since native threading APIs don't have a notion of lifetimes, this requires the implementation code to think about what to do with this lifetime 'a, which probably just ends up getting converted to 'static or such.

Since spawn_unchecked already documents this behavior as part of its unsafe API (“The caller has to ensure that no references in the supplied thread closure or its return type can outlive the spawned thread's lifetime.”) I think it makes sense to pass on the most liberal lifetime possible ('static) to the underlying native implementation.

If this is all too vague, happy to ping you about this again when my new target PR is ready.

[oliver-giersch]

It seems to me that you have some misconceptions about lifetimes.

I think it makes sense to pass on the most liberal lifetime possible ('static) to the underlying native implementation.

This seems to be at the core of it, as the 'static lifetime is in fact quite the opposite: It is the most restrictive lifetime bound possible (this chapter of the Rustonomicon contains some clarifications on the topic IIRC). A completely unbounded lifetime like 'a is the most lenient bound (or non-bound) that can be applied to a type or a trait. You are correct that native thread implementations have no notion of lifetimes, which is precisely why it makes sense to not bound lifetimes in any way, which is why the bounds for imp::Thread::new and spawn_unchecked are the way they are. The 'static lifetime bound is just tacked on later by the Rust std::thread implementation in order to achieve memory safety.

I hope this explanations of the way I understand it makes sense to you.

[jethrogb]

A completely unbounded lifetime like 'a is the most lenient bound (or non-bound) that can be applied to a type or a trait.

Most lenient from the perspective of the caller. I'm talking about the perspective of the callee. A function receiving Box<FnBox() + 'a> can basically do absolutely nothing with that closure besides calling it right away. It certainly can't send it to another thread.

[FenrirWolf]

Is there anything blocking this from stablization?

[oliver-giersch]

Is there anything I can do to help this along to get stabilized? In my opinion the addition is straight forward, as it doesn't really alter the previous implementation and just exposes an intermediate step. I have also laid out the safety concerns in the new unsafe spawn_unchecked function quite clearly I think.

If it helps, here is a link to a small demonstration how this function could be used to build a safe scoped thread abstraction that uses very little unsafe code (less than the current crossbeam implementation, which has to do lifetime transmutations).

[oliver-giersch]

Bumping again, would like to see this stabilized.

[Amanieu]

Would it make sense to also remove the Send requirement? As the name says, this is unchecked and unsafe anyways.

[RustyYato]

I don't think so because it is never fine to send !Send things to another thread, and if you are determined to do so, you can implement Send for a wrapper type AssertSend, and use that.

[oliver-giersch]

That is not correct, consider the following example:

let rc = Rc::new(1);
let handle = unsafe { thread::spawn_unchecked(move || assert_eq!(*rc, 1)) };
handle.join().unwrap();

Rc is !Send but in this example it is sound (but pointless) to send it to a thread, since the reference is not actually shared. Even if it were cloned and the clone sent into the thread there would be no issue as long as the reference in the main thread were dropped only after the join.

So I'd think it might be ok to remove the Send requirement for spawn_unchecked. As a consequence, this would make it using this function in application code less desirable (one more invariant that has to be checked), which is probably something that would be considered to be a plus by most (keeping people from using unsafe whenever possible) and move this function firmly into the realm of (internally unsafe) library territory.

[RustyYato]

Ok, I'll adjust my statement. We already have an easy way to implement Send with unsafe (just unsafe impl Send for ...), so we don't need to remove the bound from spawn_unchecked. Keeping the bound will make checking spawn_unchecked a bit easier because you don't have to worry about accidentally sending !Send data.

tldr: keeping the bound makes things more orthogonal, and easier to check

[Elrendio]

Hello !

Any blockers for stabilisation ?

Thanks a lot 🙂

[JonoGSmith]

Hello all, any updates on this?

[nickpdemarco]

Per the stdlib developer's guide, here's a ping to @cuviper (found here) to see if this is ready for stabilization. I (on behalf of Adobe) need this stabilized for a concurrency library I'm developing.

[dtolnay]

@rust-lang/libs-api: @rfcbot fcp merge

https://doc.rust-lang.org/nightly/std/thread/struct.Builder.html#method.spawn_unchecked

Example of correct usage, adapted from https://github.com/asajeffrey/thread-init:

use std::cell::RefCell;
use std::io;
use std::sync::mpsc::{self, Sender};
use std::thread::{self, JoinHandle};

// Allows initialization of a thread using borrowed data.
fn spawn_init<F, G, T>(f: F) -> io::Result<JoinHandle<T>>
where
    F: FnOnce() -> G + Send,
    G: FnOnce() -> T + 'static,
    T: Send + 'static,
{
    let (sender, receiver) = mpsc::channel();

    let fg = move || {
        struct Guard(Sender<()>);

        impl Drop for Guard {
            fn drop(&mut self) {
                self.0.send(()).unwrap();
            }
        }

        let guard = Guard(sender);
        let g = f();
        drop(guard);
        g()
    };

    let thread_builder = thread::Builder::new();
    let join_handle = unsafe { thread_builder.spawn_unchecked(fg) }?;
    receiver.recv().unwrap();
    Ok(join_handle)
}

fn main() {
    thread_local! {
        static LOCAL_STATE: RefCell<Vec<u8>> = panic!();
    }

    let mut shared_state = b"...".to_vec();
    let thread = spawn_init(|| {
        // access shared state
        LOCAL_STATE.set(shared_state.clone());
        || {
            // access only local state
            LOCAL_STATE.with(|state| println!("{:?}", state));
        }
    })
    .unwrap();

    // shared state is no longer being accessed by the other thread
    shared_state.clear();

    thread.join().unwrap();
}

Alternatives:

let join_handle = thread_builder.spawn(unsafe {
    mem::transmute::<
        Box<dyn FnOnce() -> T + Send>,
        Box<dyn FnOnce() -> T + Send + 'static>,
    >(Box::new(fg))
})?;

[rfcbot]

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

• [x] @Amanieu
• [ ] @BurntSushi
• [x] @dtolnay
• [x] @joshtriplett
• [ ] @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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]

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

[Amanieu]

The signature of spawn_unchecked has a 'a lifetime which seems entirely unnecessary and should be removed.

[dtolnay]

Lifetime cleanup: https://github.com/rust-lang/rust/pull/128745

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.