Use vector for dtors in thread and remove defer() #32

jeehoonkang · 2018-07-26T14:06:00Z

I replaced a custom list with Vec and corrected comments.

Fixes the point 1 of #29
Fixes the point 3 of #29
Closes #33 (actually using vector for dtors was intertwined with removing defer(); sorry for making a duplicated PR.)

cc @oliver-giersch @stjepang

ghost

Nice cleanup!

jeehoonkang · 2018-07-26T14:16:47Z

Before merging it, I'd like to add tests first. While I'm quite confident it's correct, but my feeling is often wrong... :)

jeehoonkang · 2018-07-27T02:23:42Z

Some changes:

I added some tests. But it's not enough to close Write tests for scoped threads #12.
In course of doing so, I found a bug: I think deferred functions should be executed before joining threads. A related discussion is in Remove Scope::defer #33. A relevant test case is here.

@stjepang May I ask if you could review it again? Sorry for sneaking in an approved PR...

oliver-giersch

Some improvement suggestions, nothing critical

oliver-giersch · 2018-07-26T20:14:16Z

src/thread.rs

    fn drop_all(&mut self) -> thread::Result<()> {
        let mut ret = Ok(());
-        while let Some(dtor) = DtorChain::pop(&mut self.dtors.borrow_mut()) {
+        let dtors = mem::replace(&mut *self.dtors.borrow_mut(), Vec::new());


I'd suggest to use for dtor in self.dtors.borrow_mut().drain(..) { instead (more expressive/idiomatic).
Alternatively self.dtors.borrow_mut().clear() could be called after the loop.

Now this PR uses drain(..) :) Thanks for suggestion!

oliver-giersch · 2018-07-26T20:15:45Z

src/thread.rs

 pub struct Scope<'a> {
    /// The list of the deferred functions and thread join jobs.
-    dtors: RefCell<Option<DtorChain<'a, thread::Result<()>>>>,
+    dtors: RefCell<Vec<Box<FnBox<thread::Result<()>> + 'a>>>,


Alternatively RefCell<Vec<Box<dyn FnBox<thread::Result<()>> + 'a>>> could be used here (I believe bare trait objects are going to be deprecated at some point),

I'd like to switch to dyn Trait when we're starting to support Rust 2018. Probably we have other lint warnings for Rust 2018, and I want to remove them at once. What do you think?

oliver-giersch · 2018-07-28T06:44:43Z

For consistency, the vector should probably be reverse iterated during drop_all(), since the linked list uses a push_front/pop_front combination.

ghost · 2018-07-28T11:40:22Z

What should the following do?

catch_unwind(AssertUnwindSafe(|| {
    crossbeam::scope(|scope| {
        scope.defer(|| panic!());
        panic!();
    });
})

I'd say "catch the second panic and execute the deferred function", but this will currently abort the process. The reason is that the deferred function is executed while we're already unwinding (inside Scope::drop), which leads to abort.

To solve this problem, I suggest removing the Drop impl for Scope and executing the scope function like this:

// Executes the scoped function.
let result = catch_unwind(AssertUnwindSafe(|| f(&scope)));
scope.drop_all();
let ret = result?;

Can we add the example above as a test to make sure double panics don't abort the process?

I think deferred functions should be executed before joining threads.

I agree, but... What is going to happen once we allow borrowing Scope inside scoped threads? Also, what if we allow borrowing Scope inside deferred functions? In that case, deferred functions can spawn new threads and spawned threads can defer new functions. Then we'd have to make the order unspecified and just keep joining/executing until there are no more threads/functions.

jeehoonkang · 2018-07-29T01:53:53Z

@stjepang I agree with you: there should be no order between thread joins and deferred functions, and f(&scope) should be catch_unwinded.

After thinking of this issue a little bit more, I finally agree with you in removing defer() at all (#33). First of all, it became clear that we still don't have an agreed semantics of defer() because of subtle issues discussed here and #33. Second, I think whatever we want for defer() can be implemented on top of scope() without defer(). As discussed in the context of logging (#26), we can declare a container of deferred function before starting a scope(), add deferred functions to the container when necessary, and then execute it after scope().

Maybe we can provide this deferred function container and logger as a companion to the thread library (related #27, cc @oliver-giersch). What do you think?

jeehoonkang · 2018-07-29T12:53:15Z

@stjepang @oliver-giersch I updated this PR as follows:

I just removed defer(). In a near future, I intend to make a PR that implements defer() on top of scoped threads.
I added the test @stjepang mentioned (panic_twice()).

I think it's ready to be merged. Would you please give another round of reviews? Thanks!

ghost · 2018-07-29T12:59:17Z

src/thread.rs

+    let result = panic::catch_unwind(panic::AssertUnwindSafe(|| f(&scope)));
+
+    // Joins all the threads.
+    scope.drop_all();


At this point, it'd probably make sense to rename drop_all to join_all (nothing is dropped, really - we just join threads).

ghost · 2018-07-29T12:59:56Z

src/thread.rs

    fn drop(&mut self) {
-        // Actually, there should be no deferred functions left to be run.
-        self.drop_all().unwrap();
+        // Note that `self.dtors` can be non-empty when the code inside a `scope()` panics and


This comment needs to be updated. self.dtors doesn't exist anymore.

ghost · 2018-07-29T13:08:09Z

src/thread.rs

+
+    // Joins all the threads.
+    scope.drop_all();
+    let panic = scope.panics.borrow_mut().pop();


Before this line, we could do:

let result = match result { Ok(r) => r, Err(e) => panic::resume_unwind(e), };

And return the result as Ok(result) at the end of function.

This way we only catch panics inside spawned threads, not inside the scope itself.

Do you agree with this, or do you think we should catch panics inside the scope, too? I'm personally not 100% sure yet...

Hmm. I'm also not 100% sure. So let's look at how rayon does: https://docs.rs/rayon/1.0.2/rayon/fn.scope.html#panics "If a panic occurs, either in the closure given to scope() or in any of the spawned jobs, that panic will be propagated and the call to scope() will panic. If multiple panics occurs, it is non-deterministic which of their panic values will propagate. Regardless, once a task is spawned using scope.spawn(), it will execute, even if the spawning task should later panic. scope() returns once all spawned jobs have completed, and any panics are propagated at that point."

Basically rayon doesn't distinguish panics inside scope() and panics of the spawned threads. I'd like to follow the same thing: either (1) propagating a panic, as rayon does, or (2) returning a panic as Err, but not mixing two schemes. We've decided to return thread panics as Err, but we've not released it and we're free to go back to the old behavior (propagating it). An advantage of it is it's behavior/signature is matching with rayon.

What do you think?

Yeah, following whatever Rayon does makes sense.

I'll also ask one more person for opinion, prz on IRC, who's been following recent developments in crossbeam-utils.

So there are a number of differences between scopes in crossbeam and rayon:

In crossbeam, spawned threads can return a result. In rayon, they can't.

In crossbeam, spawned threads can be manually joined. In rayon, they can't.

In crossbeam, panics from spawned threads can be handles using .join(). In rayon, they can't.

In crossbeam, panics from unjoined threads can be handled as the Result returned from scope. In rayon, scope simply propagates all panics and returns a T, not Result.

Basically, it looks like in rayon all panics are propagated, while in crossbeam panics tend to be caught and handled manually.

I agree -- let's return Result instead of propagating panic. I'm merging it. Thanks!

ghost

Ok, I'm happy with this PR. Feel free to merge. 👍

ghost approved these changes Jul 26, 2018

View reviewed changes

jeehoonkang mentioned this pull request Jul 27, 2018

Remove Scope::defer #33

Closed

oliver-giersch reviewed Jul 27, 2018

View reviewed changes

jeehoonkang changed the title ~~Use vector for dtors in thread~~ [WIP] Use vector for dtors in thread Jul 29, 2018

jeehoonkang added 5 commits July 29, 2018 21:31

Use vector for dtors in thread

43fec3f

Add tests

bb139b6

Fix semantics of defer and join

5456810

Rustfmt

fb0edaf

Remove Scope::defer()

f355b1f

jeehoonkang changed the title ~~[WIP] Use vector for dtors in thread~~ Use vector for dtors in thread and remove defer() Jul 29, 2018

Update comments

0052bd1

ghost reviewed Jul 29, 2018

View reviewed changes

Revise (@stjepang's comments)

afd90a2

ghost approved these changes Jul 29, 2018

View reviewed changes

jeehoonkang merged commit 60eed74 into crossbeam-rs:master Jul 29, 2018

Use vector for dtors in thread and remove defer() #32

Use vector for dtors in thread and remove defer() #32

Uh oh!

Conversation

jeehoonkang commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

jeehoonkang commented Jul 26, 2018

Uh oh!

jeehoonkang commented Jul 27, 2018

Uh oh!

oliver-giersch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliver-giersch commented Jul 28, 2018

Uh oh!

ghost commented Jul 28, 2018

Uh oh!

jeehoonkang commented Jul 29, 2018

Uh oh!

jeehoonkang commented Jul 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

jeehoonkang commented Jul 26, 2018 •

edited

Loading