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

Conversation

matklad
Copy link
Member

@matklad matklad commented Aug 3, 2018

Hi!

I propose to expose a way to check if a Once instance is initialized.

I need it in once_cell. OnceCell is effetively a pair of (Once, UnsafeCell<Option<T>>), which can set the T only once. Because I can't check if Once is initialized, I am forced to add an indirection and check the value of ptr instead:

https://github.com/matklad/once_cell/blob/8127a81976c3f2f4c0860562c3f14647ebc025c0/src/lib.rs#L423-L429

https://github.com/matklad/once_cell/blob/8127a81976c3f2f4c0860562c3f14647ebc025c0/src/lib.rs#L457-L461

The parking_lot's version of Once exposes the state as an enum: https://docs.rs/parking_lot/0.6.3/parking_lot/struct.Once.html#method.state.

I suggest, for now, just to add a simple bool function: this fits my use-case perfectly, exposes less implementation details, and is forward-compatible with more fine-grained state checking.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2018
/// 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!

@matklad
Copy link
Member Author

matklad commented Aug 4, 2018

Here's a somewhat better comparison which explains why .is_completed would be neat.

parking lot std

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 5, 2018
Copy link
Member

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

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

This feels like a highly abusable / misusable feature from the outside looking in. With such a function, I can see people wrapping their usages of Once with an extra guard "for the fast path". I don't know if that's a real concern or even if it would be an antipattern, but maybe you could chime in on that a bit?

///
/// 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

/// ```
#[unstable(feature = "once_is_completed", issue = "42")]
pub fn is_completed(&self) -> bool {
self.state.load(Ordering::Acquire) == COMPLETE
Copy link
Member

Choose a reason for hiding this comment

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

I'm never smart enough to use any ordering besides SeqCst, but perhaps you can explain why this ordering is appropriate to whoever is clever enough to understand?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Reshuffled the code a bit to reuse an existing comment :-)

@shepmaster
Copy link
Member

Passing on to someone smarter...

r? @Kimundi

@rust-highfive rust-highfive assigned Kimundi and unassigned shepmaster Aug 6, 2018
@matklad
Copy link
Member Author

matklad commented Aug 6, 2018

With such a function, I can see people wrapping their usages of Once with an extra guard "for the fast path". I don't know if that's a real concern or even if it would be an antipattern, but maybe you could chime in on that a bit?

Interesting point! It is indeed would be an anti pattern! However, I believe there are cases where you really need to check if once is initialized, without actually initializing it.

For example, a Once may guard initialization of some resource, and some part of code might want to assume that the resources is initialized, without the ability to initialized the resource itself. For example, you might want to have a global logger, initialized in main from command-line arguments. To be able to use the logger in the rest of the program safely, you'll need to get an Option<Logger> somehow, and then unwrap it. And to do be able to differentiate between Some and None, you'll need either is_completed, or to maintain your own atomic state variable, which is error-prone.

@Kimundi
Copy link
Member

Kimundi commented Aug 8, 2018

I've also got feature requests for such functionality in the past in lazy_static, so it seems like a useful addition.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 8, 2018

Team member @Kimundi has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 8, 2018
@SimonSapin
Copy link
Contributor

@rfcbot document the race condition

I think there is a potential race condition:

  • is_completed does its atomic load, and returns false
  • Then another thread completes initialization
  • Then the first thread acts on the boolean return value, which is now outdated.

This race is probably fine in some cases. I think it is in the use case described by #53027 (comment). But still, this should be mentioned in the doc-comment to try and make callers aware of it.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 9, 2018
@rfcbot
Copy link

rfcbot commented Aug 9, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@matklad
Copy link
Member Author

matklad commented Aug 9, 2018

But still, this should be mentioned in the doc-comment to try and make callers aware of it.

Yup, added this case to the doc. Ideally, these all should be formulated in terms of "happens before", but I can't find a short way to express that.

As a non-native speaker, I am also not sure whether it should be is_completed or is_complete.

@sfackler
Copy link
Member

sfackler commented Aug 9, 2018

has_completed?

@RalfJung
Copy link
Member

Yup, added this case to the doc. Ideally, these all should be formulated in terms of "happens before", but I can't find a short way to express that.

In this case, the intuition I usually try to convey is that the return value is outdated. So people should think of is_completed as saying "at some point in the past (when I was checking), the Once was [not] initialized". That should hopefully make it clear that it might have been initialized since then.

Maybe it should be called was_completed? ;)

@SimonSapin
Copy link
Contributor

Oops I messed up the rcfbot command to formally register my concern. But that’s ok, since my concern is resolved now. Thanks!

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 19, 2018
@rfcbot
Copy link

rfcbot commented Aug 19, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@pietroalbini
Copy link
Member

Ping from triage @Kimundi! The FCP ended.

@TimNN
Copy link
Contributor

TimNN commented Sep 4, 2018

Ping from triage @Kimundi / @rust-lang/libs: This PR requires your review.

@alexcrichton
Copy link
Member

@bors: r+

I'm gonna go ahead and r+ this to merge but we can of course continue to bikeshed the name while it's unstable!

@bors
Copy link
Contributor

bors commented Sep 4, 2018

📌 Commit e1bd0e7 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2018
@bors
Copy link
Contributor

bors commented Sep 5, 2018

Testing commit e1bd0e7 with merge f68b7cc...

bors added a commit that referenced this pull request Sep 5, 2018
Allow to check if sync::Once is already initialized

Hi!

I propose to expose a way to check if a `Once` instance is initialized.

I need it in `once_cell`. `OnceCell` is effetively a pair of `(Once, UnsafeCell<Option<T>>)`, which can set the `T` only once. Because I can't check if `Once` is initialized, I am forced to add an indirection and check the value of ptr instead:

https://github.com/matklad/once_cell/blob/8127a81976c3f2f4c0860562c3f14647ebc025c0/src/lib.rs#L423-L429

https://github.com/matklad/once_cell/blob/8127a81976c3f2f4c0860562c3f14647ebc025c0/src/lib.rs#L457-L461

The `parking_lot`'s version of `Once` exposes the state as an enum: https://docs.rs/parking_lot/0.6.3/parking_lot/struct.Once.html#method.state.

I suggest, for now, just to add a simple `bool` function: this fits my use-case perfectly, exposes less implementation details, and is forward-compatible with more fine-grained state checking.
@bors
Copy link
Contributor

bors commented Sep 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f68b7cc to master...

@bors bors merged commit e1bd0e7 into rust-lang:master Sep 5, 2018
/// assert_eq!(INIT.is_completed(), false);
/// ```
#[unstable(feature = "once_is_completed", issue = "42")]
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

bors added a commit that referenced this pull request Oct 7, 2018
Fix tracking issue for Once::is_completed

#53027 was merged without a tracking issue. I just filed #54890. CC @matklad
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 9, 2018
Fix tracking issue for Once::is_completed

rust-lang#53027 was merged without a tracking issue. I just filed rust-lang#54890. CC @matklad
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 10, 2018
Fix tracking issue for Once::is_completed

rust-lang#53027 was merged without a tracking issue. I just filed rust-lang#54890. CC @matklad
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Oct 11, 2018
Fix tracking issue for Once::is_completed

rust-lang#53027 was merged without a tracking issue. I just filed rust-lang#54890. CC @matklad
kennytm added a commit to kennytm/rust that referenced this pull request Oct 12, 2018
Fix tracking issue for Once::is_completed

rust-lang#53027 was merged without a tracking issue. I just filed rust-lang#54890. CC @matklad
@matklad matklad deleted the once_is_completed branch July 9, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet