The Wayback Machine - https://web.archive.org/web/20201124061622/https://github.com/crossbeam-rs/crossbeam/issues/196
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

A method to query the scope if anything panicked #196

Open
vorner opened this issue Jul 24, 2018 · 6 comments
Open

A method to query the scope if anything panicked #196

vorner opened this issue Jul 24, 2018 · 6 comments

Comments

@vorner
Copy link
Contributor

@vorner vorner commented Jul 24, 2018

Let's say I have a code like this:

thread::scope(|scope| {
    scope.thread(|| for item in long_vector {
        do_stuff(item);
    });
    scope.thread(|| for item in another_long_vector {
        do_stuff(item);
    });
});

Now, if one of the threads panics, the scope has little choice than wait for the other to terminate as well, which could be a long time (or, in certain cases, might never happen, because it relies on the existence of the dead thread).

It would still be useful to at least be able to query the scope if any of the threads panicked and let the thread terminate on its own will, eg:

scope.thread(|| for item in long_vector {
    if scope.anything_panicked() {
      break;
    }
    do_stuff(item);
});
@stjepang
Copy link
Contributor

@stjepang stjepang commented Jul 24, 2018

It seems this should be easy to implement manually, perhaps like this:

let panicked = AtomicBool::new(false);
crossbeam::scope(|scope| {
    let spawn = |f| {
        scope.spawn(move || {
            match panic::catch_unwind(f) => {
                Ok(r) => r,
                Err(e) => {
                    panicked.store(true, SeqCst);
                    panic::resume_unwind(e);
                }
            }
        })
    };

    spawn(|| {
        for item in long_vector {
            if panicked.load(SeqCst) {
                break;
            }
            do_stuff(item);
        }
    });
});

But it might be worth implementing this functionality in crossbeam. Here's a suggestion:

impl Scope<'_> {
    fn running_threads() -> usize;
    fn panicked_threads() -> usize;
    fn completed_threads() -> usize;
}
@vorner
Copy link
Contributor Author

@vorner vorner commented Jul 24, 2018

Implementing it manually in every single one thread might be quite tedious I think. It would be nice to already have such support ‒ if it would not be too much work to do it inside (which it probably shouldn't be).

Yes, the proposed methods actually look better, they are more general.

@stjepang
Copy link
Contributor

@stjepang stjepang commented Sep 29, 2018

My last suggestion on how to implement this manually was not very ergonomic, but here's a better idea using scopeguard and the new AtomicCell:

let panicked = AtomicCell::new(false);

thread::scope(|scope| {
    scope.thread(|| {
        defer_on_unwind! { panicked.store(true); }

        for item in long_vector {
            do_stuff(item);
        }
    });

    scope.thread(|| {
        for item in another_long_vector {
            if panicked.load() {
                break;
            }
            do_stuff(item);
        }
    });
});

It's even easy to do counters:

let finished = AtomicCell::new(0);
let panicked = AtomicCell::new(0);

thread::scope(|scope| {
    scope.thread(|| {
        defer! { finished.fetch_add(1); }
        defer_on_unwind! { panicked.fetch_add(1); }

        for item in long_vector {
            do_stuff(item);
        }
    });
});

Given the simplicity of this solution, I wonder whether it's worth having built-in counters in Scope at all.

Note that this way you also have more precise control over which threads are actually tracked. For example, you might want to check for panics only in some of the threads rather than in all threads spawned within the scope.

@vorner
Copy link
Contributor Author

@vorner vorner commented Sep 29, 2018

The scopeguard trick certainly lowers the cost of implementing it. Though I still wonder if it would be more natural to be able to query the scope for this.

Just out of curiosity, what is the advantage of AtomicCell over AtomicBool/AtomicUsize here?

@stjepang
Copy link
Contributor

@stjepang stjepang commented Sep 30, 2018

Just out of curiosity, what is the advantage of AtomicCell over AtomicBool/AtomicUsize here?

AtomicCell is more ergonomic because you don't have to specify orderings.

@stjepang stjepang transferred this issue from crossbeam-rs/crossbeam-utils Nov 5, 2018
@jeehoonkang
Copy link
Contributor

@jeehoonkang jeehoonkang commented Mar 3, 2019

I agree with @stjepang on this issue. We can implement status trackers on top of scoped threads and other utilities, so I don't see much necessity for adding it as a feature. Furthermore, tracking the status will add a slight overhead, which I would like to avoid in a general-purpose library.

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.