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

Stabilize std::thread #20615

Merged
merged 2 commits into from Jan 7, 2015

Conversation

Projects
None yet
8 participants
@aturon
Copy link
Member

aturon commented Jan 6, 2015

This commit takes a first pass at stabilizing std::thread:

  • It removes the detach method in favor of two constructors -- spawn
    for detached threads, scoped for "scoped" (i.e., must-join)
    threads. This addresses some of the surprise/frustrating debug
    sessions with the previous API, in which spawn produced a guard that
    on destruction joined the thread (unless detach was called).

    The reason to have the division in part is that Send will soon not
    imply 'static, which means that scoped thread creation can take a
    closure over shared stack data of the parent thread. On the other
    hand, this means that the parent must not pop the relevant stack
    frames while the child thread is running. The JoinGuard is used to
    prevent this from happening by joining on drop (if you have not
    already explicitly joined.) The APIs around scoped are
    future-proofed for the Send changes by taking an additional lifetime
    parameter. With the current definition of Send, this is forced to be
    'static, but when Send changes these APIs will gain their full
    flexibility immediately.

    Threads that are spawned, on the other hand, are detached from the
    start and do not yield an RAII guard.

    The hope is that, by making scoped an explicit opt-in with a very
    suggestive name, it will be drastically less likely to be caught by a
    surprising deadlock due to an implicit join at the end of a scope.

  • The module itself is marked stable.

  • Existing methods other than spawn and scoped are marked stable.

The migration path is:

Thread::spawn(f).detached()

becomes

Thread::spawn(f)

while

let res = Thread::spawn(f);
res.join()

becomes

let res = Thread::scoped(f);
res.join()

[breaking-change]

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 6, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jan 6, 2015

@pythonesque

This comment has been minimized.

Copy link
Contributor

pythonesque commented Jan 6, 2015

This API makes sense, and it's good to see that this usecase will be supported in the standard library.

The one concern I have is that with this API, you have no choice but to detach the thread immediately, which is a limitation that didn't previously exist. But I can't immediately think of a reason not to detach immediately if you were going to detach at all, so maybe this is acceptable.

@aturon aturon force-pushed the aturon:stab-2-thread branch from b026326 to ca4380c Jan 6, 2015

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jan 6, 2015

@pythonesque We could always re-introduce a detach method on JoinGuard<'static, T>. I wanted to avoid that for now, to keep our options maximally open.

Thanks for the feedback!

@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Jan 6, 2015

I have the same concerns as @pythonesque. I like this API, but I think we should still have a way to explicitly detach() when necessary, because I could imagine edge cases where a program can't decide if it wants to detach until the the thread has already started work.

Here's an example of a toy program that requires this ability:

use std::thread::Thread;
use std::sync::mpsc::channel;
use std::rand::{self, Rng};
use std::io::timer::sleep;
use std::time::duration::Duration;

fn run_thread() {
    let (tx, rx) = channel();
    println!("running thread");
    let guard = Thread::spawn(move || {
        tx.send(rand::thread_rng().gen_range(0f32, 1f32)).unwrap();
        println!("thread sleeping");
        sleep(Duration::seconds(2));
        println!("thread finished sleeping");
    });
    if rx.recv().unwrap() < 0.5 {
        println!("detaching");
        guard.detach()
    } else {
        println!("joining")
    }
}

fn main() {
    run_thread();
    println!("exiting main thread");
}
@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Jan 6, 2015

I wanted to avoid that for now, to keep our options maximally open.

I'd suggest reintroducing detach() but leave it marked #[unstable] or #[experimental], if you don't want to commit to keeping it.

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jan 6, 2015

Thanks for the feedback @kballard! Reintroducing as #[experimental] seems harmless enough; I'll update.

@aturon aturon force-pushed the aturon:stab-2-thread branch from ca4380c to 248b3c4 Jan 6, 2015

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jan 6, 2015

Added back detach.

@aturon aturon referenced this pull request Jan 6, 2015

Closed

Stabilization metabug: 1.0-alpha #19260

140 of 142 tasks complete
@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 6, 2015

Are we okay with panicing if thread creation fails? Should spawn and detach return Results?

@pythonesque

This comment has been minimized.

Copy link
Contributor

pythonesque commented Jan 6, 2015

Ah yeah, that would be useful as well.

/// A handle to a thread.
pub struct Thread {
inner: Arc<Inner>,
}

#[stable]
unsafe impl Sync for Thread {}

This comment has been minimized.

@alexcrichton

alexcrichton Jan 6, 2015

Member

This impl shouldn't be necessary, right? The Arc implies as much?

@@ -525,17 +525,17 @@ mod tests {
#[test]
fn test_null_byte() {
use thread::Thread;
let result = Thread::spawn(move|| {
let result = Thread::scopedspawn(move|| {

This comment has been minimized.

@sfackler

sfackler Jan 6, 2015

Member

scopedspawn?

@aturon aturon force-pushed the aturon:stab-2-thread branch 2 times, most recently from 1c84946 to 6ee660b Jan 6, 2015

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jan 6, 2015

@sfackler

Are we okay with panicing if thread creation fails? Should spawn and detach return Results?

It's a good question. My and @alexcrichton's instinct here is that this is a relatively niche concern that could be relegated to std::os::thread (or something like that, details still being worked out) -- a lower-level, cross-platform thread API.

In any case, these are #[unstable] for now so we can revisit during alpha.

@alexcrichton Nits addressed.

@@ -0,0 +1,857 @@
// Copyright 2012 The Rust Project Developers. See the COPYRIGHT

This comment has been minimized.

@alexcrichton

alexcrichton Jan 6, 2015

Member

It's back from the dead!

@thestinger

This comment has been minimized.

Copy link
Contributor

thestinger commented Jan 6, 2015

@sfackler: It's not much different from memory allocation failing. It needs to be handled in robust software but Rust's libraries are not usable for that anyway.

pub struct JoinGuard<T> {
#[must_use]
#[unstable = "may change with specifics of new Send semantics"]
pub struct JoinGuard<'a, T> {

This comment has been minimized.

@alexcrichton

alexcrichton Jan 6, 2015

Member

Should this also have T: 'a ?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 6, 2015

This is a pretty conservative step, so r=me with a few nits, we've still got some room to tweak interfaces here and there as necessary.

Stabilize std::thread
This commit takes a first pass at stabilizing `std::thread`:

* It removes the `detach` method in favor of two constructors -- `spawn`
  for detached threads, `scoped` for "scoped" (i.e., must-join)
  threads. This addresses some of the surprise/frustrating debug
  sessions with the previous API, in which `spawn` produced a guard that
  on destruction joined the thread (unless `detach` was called).

  The reason to have the division in part is that `Send` will soon not
  imply `'static`, which means that `scoped` thread creation can take a
  closure over *shared stack data* of the parent thread. On the other
  hand, this means that the parent must not pop the relevant stack
  frames while the child thread is running. The `JoinGuard` is used to
  prevent this from happening by joining on drop (if you have not
  already explicitly `join`ed.) The APIs around `scoped` are
  future-proofed for the `Send` changes by taking an additional lifetime
  parameter. With the current definition of `Send`, this is forced to be
  `'static`, but when `Send` changes these APIs will gain their full
  flexibility immediately.

  Threads that are `spawn`ed, on the other hand, are detached from the
  start and do not yield an RAII guard.

  The hope is that, by making `scoped` an explicit opt-in with a very
  suggestive name, it will be drastically less likely to be caught by a
  surprising deadlock due to an implicit join at the end of a scope.

* The module itself is marked stable.

* Existing methods other than `spawn` and `scoped` are marked stable.

The migration path is:

```rust
Thread::spawn(f).detached()
```

becomes

```rust
Thread::spawn(f)
```

while

```rust
let res = Thread::spawn(f);
res.join()
```

becomes

```rust
let res = Thread::scoped(f);
res.join()
```

[breaking-change]

@aturon aturon force-pushed the aturon:stab-2-thread branch from 6ee660b to 1bd26f7 Jan 6, 2015

@pythonesque

This comment has been minimized.

Copy link
Contributor

pythonesque commented Jan 6, 2015

@thestinger Failing to initialize a new thread happens a lot more often than failing to allocate memory (at least in my experience). Though I guess in some sense the problem fixes itself by killing the thread :P

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 6, 2015

And conversely, thread creation is comparatively much rarer than memory allocation so the cost of returning a Result from thread creation is far lower than returning one from any memory allocation location.

@pythonesque

This comment has been minimized.

Copy link
Contributor

pythonesque commented Jan 6, 2015

On the implementation side, would it be better to use Option instead of having a "joined" flag? That should save a word in the struct size.

@aturon aturon force-pushed the aturon:stab-2-thread branch from 1bd26f7 to 7bf6892 Jan 6, 2015

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jan 6, 2015

@pythonesque @sfackler @alexcrichton I talked with some of you all about this on IRC, but: I think a good middle ground here is for the spawn and scoped methods on the builder to return Results, but on Thread (where they are meant as conveniences) to panic in these situations.

Usually we avoid such duplication (i.e., no spawn and spawn_result variants), but here the API is already duplicated for convenience; the builder is a kind of power tool. This seems to strike a good balance between making the common case easy and providing the tools for extra robustness if you want them.

All that said, I'd like to move forward with this PR as-is since there's a limited window to land these changes before alpha. Since the relevant bits of the API are #[unstable], we can revisit shortly after the alpha release.

@pythonesque

This comment has been minimized.

Copy link
Contributor

pythonesque commented Jan 6, 2015

Seems reasonable to me.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 6, 2015

👍

@aturon aturon force-pushed the aturon:stab-2-thread branch from 7bf6892 to caca9b2 Jan 6, 2015

@alexcrichton

This comment has been minimized.

Copy link

alexcrichton commented on caca9b2 Jan 6, 2015

r+

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 7, 2015

rollup merge of rust-lang#20615: aturon/stab-2-thread
This commit takes a first pass at stabilizing `std::thread`:

* It removes the `detach` method in favor of two constructors -- `spawn`
  for detached threads, `scoped` for "scoped" (i.e., must-join)
  threads. This addresses some of the surprise/frustrating debug
  sessions with the previous API, in which `spawn` produced a guard that
  on destruction joined the thread (unless `detach` was called).

  The reason to have the division in part is that `Send` will soon not
  imply `'static`, which means that `scoped` thread creation can take a
  closure over *shared stack data* of the parent thread. On the other
  hand, this means that the parent must not pop the relevant stack
  frames while the child thread is running. The `JoinGuard` is used to
  prevent this from happening by joining on drop (if you have not
  already explicitly `join`ed.) The APIs around `scoped` are
  future-proofed for the `Send` changes by taking an additional lifetime
  parameter. With the current definition of `Send`, this is forced to be
  `'static`, but when `Send` changes these APIs will gain their full
  flexibility immediately.

  Threads that are `spawn`ed, on the other hand, are detached from the
  start and do not yield an RAII guard.

  The hope is that, by making `scoped` an explicit opt-in with a very
  suggestive name, it will be drastically less likely to be caught by a
  surprising deadlock due to an implicit join at the end of a scope.

* The module itself is marked stable.

* Existing methods other than `spawn` and `scoped` are marked stable.

The migration path is:

```rust
Thread::spawn(f).detached()
```

becomes

```rust
Thread::spawn(f)
```

while

```rust
let res = Thread::spawn(f);
res.join()
```

becomes

```rust
let res = Thread::scoped(f);
res.join()
```

[breaking-change]

@alexcrichton alexcrichton merged commit caca9b2 into rust-lang:master Jan 7, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

alkor added a commit to alkor/rust-http that referenced this pull request Jan 11, 2015

@aturon aturon referenced this pull request Feb 18, 2015

Closed

Stabilization for 1.0-alpha2 #20761

29 of 38 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.