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

Revise std::thread #22435

Merged
merged 2 commits into from
Feb 18, 2015
Merged

Revise std::thread #22435

merged 2 commits into from
Feb 18, 2015

Conversation

aturon
Copy link
Member

@aturon aturon commented Feb 17, 2015

This commit makes several changes to std::thread in preparation for
final stabilization:

  • It removes the ability to handle panics from scoped children; see
    JoinGuard::join returning an Err is **really** unsafe #20807 for discussion
  • It adds a JoinHandle structure, now returned from spawn, which
    makes it possible to join on children that do not share data from
    their parent's stack. The child is automatically detached when the
    handle is dropped, and the handle cannot be copied due to Posix
    semantics.
  • It moves all static methods from std::thread::Thread to free
    functions in std::thread. This was done in part because, due to the
    above changes, there are effectively no direct Thread constructors,
    and the static methods have tended to feel a bit awkward.
  • Adds an io::Result around the Builder methods scoped and
    spawn, making it possible to handle OS errors when creating
    threads. The convenience free functions entail an unwrap.
  • Stabilizes the entire module. Despite the fact that the API is
    changing somewhat here, this is part of a long period of baking and
    the changes are addressing all known issues prior to alpha2. If
    absolutely necessary, further breaking changes can be made prior to beta.

Closes #20807

[breaking-change]

r? @alexcrichton

@aturon
Copy link
Member Author

aturon commented Feb 17, 2015

This is WIP because I haven't yet moved the rest of the codebase off of now-deprecated functions. I wanted to get feedback on the direction of the API first.

cc @tomaka @pythonesque @sfackler @wycats @nikomatsakis

@aturon aturon changed the title [WIP] Revise std::thread semantics [WIP] Revise std::thread Feb 17, 2015
@tomaka
Copy link
Contributor

tomaka commented Feb 17, 2015

👍 for me!

@pythonesque
Copy link
Contributor

Looks great!

@nikomatsakis
Copy link
Contributor

👍

1 similar comment
@sfackler
Copy link
Member

👍

/// to recover from such errors.
#[unstable(feature = "thread", reason = "recently moved")]
pub fn scoped<'a, T, F>(f: F) -> JoinGuard<'a, T> where
T: Send + 'a, F: FnOnce() -> T, F: Send + 'a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this 'a bound on T required? I would expect that the compiler would already ensure that T couldn't refer to anything inside the local environment of F. As in, I expect 'a to only be relevant to the lifetime of the closure and that would in turn imply that it bounds T.

@alexcrichton
Copy link
Member

This looks great to me, r=me with a small nits and doc tests passing.

@alexcrichton
Copy link
Member

I also talked with @aturon today and I would be comfortable marking these functions #[stable].

This commit makes several changes to `std::thread` in preparation for
final stabilization:

* It removes the ability to handle panics from `scoped` children; see
  rust-lang#20807 for discussion

* It adds a `JoinHandle` structure, now returned from `spawn`, which
  makes it possible to join on children that do not share data from
  their parent's stack. The child is automatically detached when the
  handle is dropped, and the handle cannot be copied due to Posix
  semantics.

* It moves all static methods from `std::thread::Thread` to free
  functions in `std::thread`. This was done in part because, due to the
  above changes, there are effectively no direct `Thread` constructors,
  and the static methods have tended to feel a bit awkward.

* Adds an `io::Result` around the `Builder` methods `scoped` and
  `spawn`, making it possible to handle OS errors when creating
  threads. The convenience free functions entail an unwrap.

* Stabilizes the entire module. Despite the fact that the API is
  changing somewhat here, this is part of a long period of baking and
  the changes are addressing all known issues prior to alpha2. If
  absolutely necessary, further breaking changes can be made prior to beta.

Closes rust-lang#20807

[breaking-change]
@aturon aturon changed the title [WIP] Revise std::thread Revise std::thread Feb 17, 2015
@aturon
Copy link
Member Author

aturon commented Feb 17, 2015

@alexcrichton OK, I've pushed an update including a fallout commit. It hasn't gotten through tests yet, but I think any trouble there should be minor (if you want to stick it in a rollup). I'll keep pushing through the compilation meanwhile.

@alexcrichton
Copy link
Member

@bors: r+ d0de2b4

/// handle: the ability to join a child thread is a uniquely-owned
/// permission.
#[stable(feature = "rust1", since = "1.0.0")]
pub struct JoinHandle(JoinInner<()>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a non-tuple struct per #22045?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component is private, so I don't think there's any problem here.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 18, 2015
Conflicts:
	src/test/bench/rt-messaging-ping-pong.rs
	src/test/bench/rt-parfib.rs
	src/test/bench/task-perf-spawnalot.rs
@tshepang
Copy link
Member

Since scoped also starts a thread, shouldn't it be a verb too... maybe something like spawn_scoped?

@sfackler
Copy link
Member

While we're poking at the Thread API, there's another change that we may want to consider. Advanced configuration of a Thread happens via the thread::Builder type, which is constructed via its new method. Working with it ends up being a bit verbose: thread::Builder::new()... compared to thread::spawn(). One thing that Guava does that I think is kind of nice is to add a builder static method to the type that's buildable. For example, ImmutableSet is constructed via the ImmutableSet.Builder inner class. The normal way to work with it would be new ImmutableSet.Builder().add(1).build(), but you can instead just do ImmutableSet.builder().add(1).build().

I've done something similar with r2d2's Config builder: http://sfackler.github.io/r2d2/doc/r2d2/struct.Config.html#method.builder to allow Config::builder().pool_size(10).build() instead of config::Builder::new().pool_size(10).build(). Since this change seems to be moving away from static methods on Thread, a builder free function in thread may be more applicable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JoinGuard::join returning an Err is **really** unsafe
8 participants