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

Catch thread in example #33668

Merged
merged 1 commit into from
May 21, 2016
Merged

Catch thread in example #33668

merged 1 commit into from
May 21, 2016

Conversation

dns2utf8
Copy link
Contributor

Since this is an example, the code will be copied by many people and should be over correct.

?r @steveklabnik

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@apasel422 apasel422 assigned steveklabnik and unassigned brson May 16, 2016
@brson
Copy link
Contributor

brson commented May 17, 2016

The example looks correct as written to me. Why join it?

//! spinlock_clone.store(0, Ordering::SeqCst);
//! });
//!
//! // Wait for the other thread to release the lock
//! while spinlock.load(Ordering::SeqCst) != 0 {}
//!
//! thread.join();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will get a "must use Result" warning -- if the idea is to put good code in examples, it should be handled.

@steveklabnik
Copy link
Member

I agree with @brson here: unless you're specifically doing something with the thread variable, why explicitly join it?

@dns2utf8
Copy link
Contributor Author

I had the situation, where the inner thread outlived the outer and caused unexpeced behavior: Run on play

use std::sync::Arc;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::thread;
use std::time::Duration;

fn main() {
    let spinlock = Arc::new(AtomicUsize::new(1));

    let spinlock_clone = spinlock.clone();
    let thread = thread::spawn(move|| {
        spinlock_clone.store(0, Ordering::SeqCst);

        thread::sleep(Duration::from_secs(2));
        for i in 0..10 {
            println!("{}", i);
        }
    });

    // Wait for the other thread to release the lock
    while spinlock.load(Ordering::SeqCst) != 0 {}

    // thread.join().is_ok();
    println!("END");
}

I could also add a hint to the threading model of rust, but I could not find a reference explaining what happens in this case with the inner thread.

@durka
Copy link
Contributor

durka commented May 17, 2016

Yeah, the main thread exits as soon as the inner thread releases the lock (and the program ends when the main thread exits) so the numbers are never printed. If the spinlock_clone.store line is moved until after the loop, then the numbers are indeed printed.

As for using the Result, calling is_ok and not looking at the return value will technically silence the warning but is actually no better.

@steveklabnik
Copy link
Member

@dns2utf8 ahhh yes, I can see that. Hmmm. (and @durka's explanation is correct)

@dns2utf8
Copy link
Contributor Author

@durka you are right about is_ok.

I am thinking about a link to std::thread#The treading model in the text section.

@dns2utf8
Copy link
Contributor Author

Hm, I seem to have broken the build with the link. Do you have an idea?

@@ -26,7 +26,9 @@
//! [1]: http://llvm.org/docs/LangRef.html#memory-model-for-concurrent-operations
//!
//! Atomic variables are safe to share between threads (they implement `Sync`)
//! but they do not themselves provide the mechanism for sharing. The most
//! but they do not themselves provide the mechanism for sharing and follow the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line has a trailing whitespace, that's what's failing the build

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, thanks 👍

 - Consume result of thread join()
 - Add link to threading model
@dns2utf8
Copy link
Contributor Author

Thanks @steveklabnik, I rebased it to current master.

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented May 19, 2016

📌 Commit 22615ad has been approved by steveklabnik

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 20, 2016
Catch thread in example

Since this is an example, the code will be copied by many people and should be over correct.

?r @steveklabnik
bors added a commit that referenced this pull request May 20, 2016
Rollup of 6 pull requests

- Successful merges: #33668, #33676, #33683, #33734, #33739, #33745
- Failed merges: #33578
@bors bors merged commit 22615ad into rust-lang:master May 21, 2016
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.

6 participants