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

Change panic handling in scoped thread #844

Closed
wants to merge 1 commit into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented May 31, 2022

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

TODO: if we don't do #844 (comment), we probably shouldn't change the signature of scope function.

Fixes #816
Closes #724

@taiki-e
Copy link
Member Author

taiki-e commented May 31, 2022

BTW, in addition to this, I'm considering porting the rust-lang/rust#94559's changes to crossbeam.

@m-ou-se
Copy link

m-ou-se commented Jun 1, 2022

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
;]Æǟ

@taiki-e
Copy link
Member Author

taiki-e commented Jun 1, 2022

@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
Copy link
Member Author

taiki-e commented Jul 23, 2022

bors r+

bors bot added a commit that referenced this pull request Jul 23, 2022
844: Change panic handling in scoped thread r=taiki-e a=taiki-e

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

Fixes #816
Closes #724

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e
Copy link
Member Author

taiki-e commented Jul 23, 2022

bors r-

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

@bors
Copy link
Contributor

bors bot commented Jul 23, 2022

Canceled.

- Panic instead of returning result
- Do not own join handles in main thread
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Memory leak in crossbeam_utils::thread::scope(): Unbounded memory usage with long-lived scopes
2 participants