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

Allow to check if sync::Once is already initialized #53027

Merged
merged 2 commits into from
Sep 5, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 59 additions & 14 deletions src/libstd/sync/once.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,9 @@ impl Once {
/// [poison]: struct.Mutex.html#poisoning
#[stable(feature = "rust1", since = "1.0.0")]
pub fn call_once<F>(&self, f: F) where F: FnOnce() {
// Fast path, just see if we've completed initialization.
// An `Acquire` load is enough because that makes all the initialization
// operations visible to us. The cold path uses SeqCst consistently
// because the performance difference really does not matter there,
// and SeqCst minimizes the chances of something going wrong.
if self.state.load(Ordering::Acquire) == COMPLETE {
return
// Fast path check
if self.is_completed() {
return;
}

let mut f = Some(f);
Expand Down Expand Up @@ -280,13 +276,9 @@ impl Once {
/// ```
#[unstable(feature = "once_poison", issue = "33577")]
pub fn call_once_force<F>(&self, f: F) where F: FnOnce(&OnceState) {
// same as above, just with a different parameter to `call_inner`.
// An `Acquire` load is enough because that makes all the initialization
// operations visible to us. The cold path uses SeqCst consistently
// because the performance difference really does not matter there,
// and SeqCst minimizes the chances of something going wrong.
if self.state.load(Ordering::Acquire) == COMPLETE {
return
// Fast path check
if self.is_completed() {
return;
}

let mut f = Some(f);
Expand All @@ -295,6 +287,55 @@ impl Once {
});
}

/// Returns true if some `call_once` call has completed
/// successfuly. Specifically, `is_completed` will return false in
/// the following situtations:
/// * `call_once` was not called at all,
/// * `call_once` was called, but has not yet completed,
/// * the `Once` instance is poisoned
///
/// It is also possible that immediately after `is_completed`
/// returns false, some other thread finishes executing
/// `call_once`.
///
/// # Examples
///
/// ```
/// #![feature(once_is_completed)]
/// use std::sync::Once;
///
/// static INIT: Once = Once::new();
///
/// assert_eq!(INIT.is_completed(), false);
Copy link
Member

Choose a reason for hiding this comment

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

Think these would normally be written as assert!(INIT.is_completed()) / assert!(!INIT.is_completed())

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for boolean-returning methods writing an assert_eq! in docs specifically helps a bit with readability. Here's an example from result:

https://doc.rust-lang.org/std/result/enum.Result.html#method.is_ok

/// INIT.call_once(|| {
/// assert_eq!(INIT.is_completed(), false);
/// });
/// assert_eq!(INIT.is_completed(), true);
/// ```
///
/// ```
/// #![feature(once_is_completed)]
/// use std::sync::Once;
/// use std::thread;
///
/// static INIT: Once = Once::new();
///
/// assert_eq!(INIT.is_completed(), false);
/// let handle = thread::spawn(|| {
/// INIT.call_once(|| panic!());
/// });
/// assert!(handle.join().is_err());
/// assert_eq!(INIT.is_completed(), false);
/// ```
#[unstable(feature = "once_is_completed", issue = "42")]
Copy link

Choose a reason for hiding this comment

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

Isn't this issue number wrong? Issue #42 seems totally irrelevant to this code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a placeholder, there's no tracking issue for this yet

Copy link

@mzji mzji Aug 6, 2018

Choose a reason for hiding this comment

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

Well, I just found that 42 is the answer to everything, however I still hope it could be replaced by the relevant issue ^v^

Copy link
Member Author

Choose a reason for hiding this comment

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

As soon as @rust-lang/libs decides that we indeed want this feature and creates a tracking issue, I'll update the PR.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation!

pub fn is_completed(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Once is not generic, so this is missing an #[inline]. (@anp found a rayon bench regression likely caused by this)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@eddyb is that documented in the api guidelines or somewhere similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #54662

// An `Acquire` load is enough because that makes all the initialization
// operations visible to us, and, this being a fast path, weaker
// ordering helps with performance. This `Acquire` synchronizes with
// `SeqCst` operations on the slow path.
self.state.load(Ordering::Acquire) == COMPLETE
}

// This is a non-generic function to reduce the monomorphization cost of
// using `call_once` (this isn't exactly a trivial or small implementation).
//
Expand All @@ -310,6 +351,10 @@ impl Once {
fn call_inner(&self,
ignore_poisoning: bool,
init: &mut dyn FnMut(bool)) {

// This cold path uses SeqCst consistently because the
// performance difference really does not matter there, and
// SeqCst minimizes the chances of something going wrong.
let mut state = self.state.load(Ordering::SeqCst);

'outer: loop {
Expand Down