Unchecked thread spawning

[oliver-giersch]

Summary

Add an unsafe interface for spawning lifetime-unrestricted threads for
library authors to build less-contrived, less-hacky safe abstractions
on.

Motivation

So a few years back scoped threads were entirely removed from the Rust
stdlib, the reason being that it was possible to leak the scoped thread's
join guards without resorting to unsafe code, which meant the concept
was not completely safe, either.
Only a maximally-restrictive safe API for thread spawning was kept in the
stdlib, that requires 'static lifetime bounds on both the thread closure
and its return type.
A number of 3rd party libraries sprung up to offer their implementations
for safe scoped threads implementations.
These work by essentially hiding the join guards from the user, thus
forcing them to join at the end of an (internal) function scope.

However, since these libraries have to use the maximally restrictive
thread spawning API, they have to resort to some very contrived manipulations
and subversions of Rust's type system to basically achieve what this commit does
with some minimal restructuring of the current code and exposing a new unsafe
function signature for spawning threads without lifetime restrictions.
Obviously this is unsafe, but its main use would be to allow library authors
to write safe abstractions with and around it.
To further illustrate my point, here's a quick summary of the hoops that,
for instance crossbeam, has to jump through to spawn a lifetime unrestricted
thread, all of which would not be necessary if an unsafe API existed as part
of the stdlib:

1. Allocate an Arc<Option<T>> on the heap where the result with type
   T: 'a will go (in practice requires Mutex or UnsafeCell as well).

2. Wrap the desired thread closure with lifetime bound 'a into another
   closure (also ..: 'a) that returns (), executes the inner closure and
   writes its result into the pre-allocated Option<T>.

3. Box the wrapping closure, cast it to a trait object (FnBox) and
   (unsafely) transmute its lifetime bound from 'a to 'static.

So while this new spawn_unchecked function is certainly not very relevant
for general use, since scoped threads are so common I think it makes sense
to expose an interface for libraries implementing these to build on.
The changes implemented are also very minimal: The current spawn function
(which internally contains unsafe code) is moved into an unsafe spawn_unchecked
function, which the safe function then wraps around.

Issues

• so far, no documentation for the new function (yet)
• the name of the function might be controversial, as *_unchecked more commonly
  indicates that some sort of runtime check is omitted (unrestricted may be
  more fitting)
• if accepted, it might make sense to add a freestanding thread::spawn_unchecked
  function similar to the current thread::spawn for convenience.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kimundi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[RalfJung]

I agree this is a reasonable thing to add. But doesn't this need an RFC? It's a new API addition, after all.

[oliver-giersch]

I agree this is a reasonable thing to add. But doesn't this need an RFC? It's a new API addition, after all.

I may have misread the guidelines, but it was may understanding that additions and changes to the stdlib do not necessarily require an RFC.

[RalfJung]

Changes for sure do, or at least did... that's what got me my first RFC ;)

Additions, not sure. Needs an FCP at least. Cc @rust-lang/libs

[Centril]

@RalfJung This does not require an RFC; signoff / fcp merge on this PR by T-libs is sufficient.

(We treat language changes differently than minor libs additions...)

[RalfJung]

This PR looks good for me, except for the missing doc, which needs to be added before merging.

Could someone initiate an FCP?

[oliver-giersch]

I've added the missing documentation.

[withoutboats]

@rfcbot fcp merge

@rust-lang/libs: we may want an RFC on this because the API being added is pretty fundamental, but I don't really think there's much design space to explore here. This PR adds an unsafe equivalent to the thread::spawn function that does not require the function passed to it implement 'static. The function added is currently named spawn_unchecked.

This is intended to be used as a building block for other concurrency library that want access to the platform thread spawning mechanism but intend to allow non-'static thread operations through some sort of safe API abstraction, like the scoped threads in crossbeam. This allows them to avoid lying to the type system by pretending the function they pass to thread::spawn is 'static somehow (which may also have a runtime overhead).

To me this API seems like a clear solution to a real problem and consistent with our exposure of other unsafe functions used as building blocks in the std APIs. I don't know what alternative designs would be. I'm not certain if std::thread::spawn_unchecked is the right path to this function, but I don't have an alternative proposal.

[rfcbot]

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

• @Kimundi
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[alexcrichton]

@oliver-giersch oh yeah feel free to file an issue lik @stjepang, and we can fill out the tags and such after-the-fact

[oliver-giersch]

@oliver-giersch You can open a tracking issue, too. I'm not on the Rust team either and similarly had to open a tracking issue for Cell::update. :) See this comment.

Thanks for the heads-up :)

The tracking issue is #55132

[rfcbot]

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

[rfcbot]

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

[alexcrichton]

@bors: r+

[bors]

📌 Commit 7849aed has been approved by alexcrichton

[bors]

⌛ Testing commit 7849aed with merge 3345cad44c04051079a42fe5d485fcd09037c9fa...

[bors]

💔 Test failed - status-appveyor

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 7849aed with merge bcb05a0...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing bcb05a0 to master...

[mejrs]

I have some concerns about the absence of panic safety in the documentation for this.

For example take this innocently looking variation of the example listed in the documentation:

use std::{panic, thread, time};

let result = panic::catch_unwind(|| {
    let builder = thread::Builder::new();
    let builder2 = thread::Builder::new();

    let x = 1;
    let thread_x = &x;

    let handler = unsafe {
        builder
            .spawn_unchecked(|| {
                panic!("kaboom");
            })
            .unwrap()
    };

    let handler2 = unsafe {
        builder2
            .spawn_unchecked(|| {
                time::Duration::from_millis(100);
                println!("x = {}", *thread_x);
            })
            .unwrap()
    };

    // caller has to ensure `join()` is called, otherwise
    // it is possible to access freed memory if `x` gets
    // dropped before the thread closure is executed!
    handler.join().unwrap();
    handler2.join().unwrap();
});

thread::sleep(time::Duration::from_millis(200));

In particular, the following events happen:

• thread 2 is spawned
• thread 1 panics
• handler.join().unwrap() is called
• the main thread starts unwinding
• x is freed
• thread 2 dereferences thread_x

Some more footguns:

• a thread fails to spawn, unwrap is called and the main thread starts unwinding
• someone spawns unchecked threads and runs other code (that could panic) before joining all the threads.
• all of these are much worse if the panic is caught upstream