Fix bugs in thread::scope() #28
Conversation
There was a problem hiding this comment.
I like this a lot. 👍
Function scope returning Result will be slightly annoying to users (the compiler warns on unused result), but I think that's for the better. After all, the scope joins threads and .join() demands handling the returned result. So previously we were just silently ignoring those results.
We should definitely write some tests for scoped threads. It's a non-trivial piece of code. I'm okay with merging this PR now, but let's write some tests before the next release.
| @@ -214,14 +221,10 @@ pub struct ScopedJoinHandle<'a, T: 'a> { | |||
| /// crossbeam_utils::thread::scope(|scope| { | |||
| /// scope.defer(|| println!("Exiting scope")); | |||
| /// scope.spawn(|| println!("Running child thread in scope")) | |||
| /// }); | |||
| /// // Prints messages in the reverse order written | |||
There was a problem hiding this comment.
It's true actually, but the methods spec doesn't guarantee it. On the contrary to what is commented before, there's no order between joining spawned threads and running deferred functions. I just removed the comment, effectively leaving the order unspecified.
Definitely, we need to discuss what's the desired specification. Asking the same question you raised before, what's the purpose of defer()? What do we expect from it? I think need to answer those questions before releasing a new version. Of course, we have to write tests, too (#12).
There was a problem hiding this comment.
This is the only use of defer I could find:
https://github.com/wayslog/statsd/blob/f27113df8667407b247d0ff6e63396a54c612e76/src/sender/mod.rs#L18
But looks like it's not necessary at all - calls to .join() can be invoked without defer as well.
I guess defer is actually useless and the same effect can be achieved using scopeguard::defer! anyway. So my suggestion would be to remove it.
This PR fixes some bugs in
crossbeam_utils::threadthat @stjepang mentioned in #7.If a scoped thread panics, other threads are not joined at all.
I guaranteed that all threads are joined.
Panic should be propagated.
I think it's a little bit subtle. In
std::threadandcrossbeam_utils::thread, when a spawned thread panics,join()returnsErrand does not propagate panics. I think the same thing should happen at the end of ascope: returningResultrather than propagatingpanic. It is a breaking change, but anyway I'm throwing it away...Closes #6
Closes #8