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

Add support for scoped threads #312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

akonradi
Copy link
Contributor

@akonradi akonradi commented Jun 5, 2023

Add loom::thread::scope and loom::thread::Scope to mirror the API provided by the standard library for implementing scoped threads.

Fixes #308

@akonradi akonradi force-pushed the scoped-thread branch 2 times, most recently from 225fe95 to c181071 Compare June 12, 2023 19:53
@akonradi
Copy link
Contributor Author

Ping for review?

@akonradi
Copy link
Contributor Author

@taiki-e pinging you since you approved PR 311, which was in a similar area. Can you TAL or help me find a reviewer?

@taiki-e
Copy link
Member

taiki-e commented Aug 9, 2023

Thanks for the PR!

It is difficult to implement scoped threads correctly and this implementation is also unsound. Consider the following test case (based on crossbeam-rs/crossbeam#844 (comment)).

// tests/thread_api.rs
#[test]
fn scoped_thread_t() {
    struct PrintOnDrop<'a>(&'a str);

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

    loom::model(|| {
        let a = "hello".to_string();
        let r = &a;
        thread::scope(|s| {
            s.spawn(move || PrintOnDrop(r));
        });
        drop(a);
    });
    std::thread::sleep(std::time::Duration::from_secs(1));
}

thread::scope must wait for all spawned threads to complete, but in the current implementation, thread::scope does not wait for the spawned thread, so the code in the spawned thread accesses the freed variable, or the program itself terminates before the spawned thread completes.

$ cargo test --package loom --test thread_api --all-features -- scoped_thread_t --exact --nocapture
running 1 test
`L�F
test scoped_thread_t ... ok
running 1 test
test scoped_thread_t ... ok

akonradi and others added 2 commits August 19, 2023 10:33
Add loom::thread::scope to mirror std::thread::scope provided by the
standard library.
Fix a bug with thread::scope where the result of a spawned thread was being
saved even when the scoped thread's handle had been dropped. This was
causing the result to be dropped after the spawned thread was "finished",
which was after the point at which the call to thread::scope had already
completed. Add a regression test for this behavior.
@akonradi
Copy link
Contributor Author

akonradi commented Aug 19, 2023

Thank you very much for the review. This is my first serious foray into unsafe Rust, so I'm not at all surprised that what I wrote was unsound!

The example actually tickled a bug in the dropping of unused thread results. The spawned thread was completing but its result was being written to memory behind an Arc that wasn't dropped until after the main thread had been unblocked. I've pushed a fix with a regression test (that doesn't double-panic, as my naive s/println/assert_eq alteration of your test case did 😄).

@notgull
Copy link
Contributor

notgull commented May 1, 2024

Any chance this PR can be reviewed? It's blocking me adding loom support for crossbeam-queue.

@Darksonn
Copy link
Contributor

Darksonn commented May 1, 2024

The scope API here uses a lot of the loom internals directly. Would it be possible to move that part of the logic into a method analogous to spawn_unchecked, and then implement the scoped API on top of that? This way, the scoped API doesn't have to touch loom internals.

@notgull The loom crate has very little reviewer bandwidth, but if this will let you use it for crossbeam-queue, then I'm willing to prioritize it.

@akonradi
Copy link
Contributor Author

I don't have time to work on this now, so if someone wants to take over and do the refactor, go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a method for spawning scoped threads
4 participants