Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for thread::Builder::spawn_unchecked #55132

Open
oliver-giersch opened this issue Oct 16, 2018 · 20 comments
Open

Tracking issue for thread::Builder::spawn_unchecked #55132

oliver-giersch opened this issue Oct 16, 2018 · 20 comments
Labels
A-concurrency Area: Concurrency related issues. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@oliver-giersch
Copy link
Contributor

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.

@Havvy Havvy added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Oct 17, 2018
@jethrogb
Copy link
Contributor

jethrogb commented Nov 1, 2018

@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
Copy link
Contributor Author

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

@jethrogb
Copy link
Contributor

jethrogb commented Nov 5, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

Is there anything blocking this from stablization?

@oliver-giersch
Copy link
Contributor Author

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
Copy link
Contributor Author

Bumping again, would like to see this stabilized.

@jonas-schievink jonas-schievink added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Nov 26, 2019
@Amanieu
Copy link
Member

Amanieu commented Jan 1, 2020

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

@RustyYato
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@KodrAus KodrAus added A-concurrency Area: Concurrency related issues. Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 31, 2020
@Elrendio
Copy link
Contributor

Hello !

Any blockers for stabilisation ?

Thanks a lot 🙂

@JonoGSmith
Copy link

Hello all, any updates on this?

@nickpdemarco
Copy link

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
Copy link
Member

dtolnay commented Apr 5, 2024

@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
Copy link

rfcbot commented Apr 5, 2024

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

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 5, 2024
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Aug 6, 2024
@rfcbot
Copy link

rfcbot commented Aug 6, 2024

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 6, 2024
@Amanieu
Copy link
Member

Amanieu commented Aug 6, 2024

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

@dtolnay
Copy link
Member

dtolnay commented Aug 6, 2024

Lifetime cleanup: #128745

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency related issues. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests