crossbeam icon indicating copy to clipboard operation
crossbeam copied to clipboard

Change panic handling in scoped thread

Open taiki-e opened this issue 2 years ago • 6 comments

  • Panic instead of returning result
  • Do not own join handles in main thread

Fixes #816 Closes #724

taiki-e avatar May 31 '22 16:05 taiki-e

BTW, in addition to this, I'm considering porting the https://github.com/rust-lang/rust/pull/94559's changes to crossbeam.

taiki-e avatar May 31 '22 16:05 taiki-e

This implementation introduces a soundness bug, since the WaitGroup is dropped before the result of a thread (which might still borrow things) is dropped:

use std::{thread::sleep, time::Duration};

struct PrintOnDrop<'a>(&'a str);

impl Drop for PrintOnDrop<'_> {
    fn drop(&mut self) {
        sleep(Duration::from_millis(100));
        println!("{}", self.0);
    }
}

fn main() {
    let a = "hello".to_string();
    let r = &a;
    crossbeam::scope(|s| {
        s.spawn(move |_| PrintOnDrop(r));
    });
    drop(a);
    sleep(Duration::from_millis(200));
}
$ cargo r -q
;]Æǟ

m-ou-se avatar Jun 01 '22 08:06 m-ou-se

@m-ou-se Thanks! I pushed a fix for that bug

EDIT: The fix is:

  • If the closure is already finished when the handle is dropped, drop the result.
  • If the closure is not yet finished when the handle is dropped, drop the closure result without storing it to result.
  • Add WaitGroup to the handle.

The last one is needed to properly handle cases where handles are not dropped:

-     s.spawn(move |_| X(actually_finished));
+     s.spawn(move |s| mem::forget(s.spawn(move |_| X(actually_finished))));

taiki-e avatar Jun 01 '22 09:06 taiki-e

bors r+

taiki-e avatar Jul 23 '22 07:07 taiki-e

bors r-

miri failed due to thread leaks: https://github.com/crossbeam-rs/crossbeam/runs/7479452681?check_suite_focus=true

taiki-e avatar Jul 23 '22 07:07 taiki-e

Canceled.

bors[bot] avatar Jul 23 '22 07:07 bors[bot]