std: Rewrite Once with poisoning #32325

alexcrichton · 2016-03-18T02:05:38Z

This commit rewrites the std::sync::Once primitive with poisoning in mind in
light of #31688. Currently a panic in the initialization closure will cause
future initialization closures to run, but the purpose of a Once is usually to
initialize some global state so it's highly likely that the global state is
corrupt if a panic happened. The same strategy of a mutex is taken where a panic
is propagated by default.

A new API, call_once_force, was added to subvert panics like is available on
Mutex as well (for when panicking is handled internally).

Adding this support was a significant enough change to the implementation that
it was just completely rewritten from scratch, primarily to avoid using a
StaticMutex which needs to have destroy() called on it at some point (a pain
to do).

Closes #31688

alexcrichton · 2016-03-18T02:05:48Z

r? @aturon

cc @rust-lang/libs

sfackler · 2016-03-18T02:45:40Z

Oooh, is this the first use of thread::park?

alexcrichton · 2016-03-18T03:04:16Z

Nah unfortunately that trophy went to channels long ago :)

huonw · 2016-03-18T03:24:02Z

src/libstd/sync/once.rs

+    /// this function will continue to attempt to call initialization functions
+    /// until one of them doesn't panic.
+    ///
+    /// The closure `f` is yielded a boolean indicating whether this `Once` is


Is true "I am poisoned", or "I am valid"?

Probably an indicator that this should be an enum or an opaque struct! Currently it means "I'm poisoned"

Stebalien · 2016-03-18T16:20:35Z

There is actually a use case for panicking in the Once initializer: it's the only way to abort initialization. This could be useful for, e.g., caching result of an expensive RPC call that is expected to succeed but may fail on occasion.

alexcrichton · 2016-03-18T17:48:34Z

@Stebalien that's an interesting data point! When we discussed this at the libs triage meeting we didn't reach a firm conclusion about whether we were comfortable to move forward with this or not, so it's good to have this data!

aturon · 2016-03-25T15:13:47Z

src/libstd/sync/once.rs

+// primitive works now!
+//
+// So to recap, the guarantees of a Once are that it will call the
+// initialization closure exactly once, and it will never return until the one


s/exactly/at most/

aturon · 2016-03-25T17:04:08Z

src/libstd/sync/once.rs

+                    }
+
+                    // Run the initialization routine, letting it know if we're
+                    // poisoned or not, and then just return immediately after


Would be helpful for this comment to mention that Finish does the wakeups.

aturon · 2016-03-25T17:09:35Z

Beautiful -- this is pretty much exactly what I imagined when we discussed the algorithm the other day.

Just a couple minor nits -- would be good to introduce an enum/struct before landing, rather than having to do that adjustment later. With that, r=me.

aturon · 2016-03-25T18:12:58Z

src/libstd/sync/once.rs

-    mutex: StaticMutex,
-    cnt: AtomicIsize,
-    lock_cnt: AtomicIsize,
+    state: AtomicUsize,


This struct should have some PhantomData describing the kind of data state can point to. (Basically this should be done any time a pointer is hidden in a usize, so that auto traits etc work correctly.)

alexcrichton · 2016-03-25T23:28:17Z

@bors: r=aturon a755a67

std: Rewrite Once with poisoning This commit rewrites the `std::sync::Once` primitive with poisoning in mind in light of rust-lang#31688. Currently a panic in the initialization closure will cause future initialization closures to run, but the purpose of a Once is usually to initialize some global state so it's highly likely that the global state is corrupt if a panic happened. The same strategy of a mutex is taken where a panic is propagated by default. A new API, `call_once_force`, was added to subvert panics like is available on Mutex as well (for when panicking is handled internally). Adding this support was a significant enough change to the implementation that it was just completely rewritten from scratch, primarily to avoid using a `StaticMutex` which needs to have `destroy()` called on it at some point (a pain to do). Closes rust-lang#31688

Rollup of 18 pull requests - Successful merges: #32131, #32199, #32257, #32325, #32383, #32387, #32432, #32435, #32440, #32447, #32448, #32456, #32468, #32470, #32476, #32478, #32484, #32492 - Failed merges: #32240, #32469, #32482

Manishearth · 2016-03-26T08:13:21Z

@bors r-

<core macros>:2:4: 2:12 error: cannot apply unary operator `!` to type `&sync::once::OnceState`
<core macros>:2 if ! $ cond {
                   ^~~~~~~~
../src/libstd/sync/once.rs:443:13: 443:30 note: in this expansion of assert! (defined in <core macros>)
<core macros>:2:4: 2:12 error: cannot apply unary operator `!` to type `&sync::once::OnceState`
<core macros>:2 if ! $ cond {
                   ^~~~~~~~
../src/libstd/sync/once.rs:466:17: 466:28 note: in this expansion of assert! (defined in <core macros>)
error: aborting due to 2 previous errors
make: *** [x86_64-unknown-linux-gnu/stage2/test/stdtest-x86_64-unknown-linux-gnu] Error 101

http://buildbot.rust-lang.org/builders/auto-linux-64-nopt-t/builds/8503/steps/test/logs/stdio

Rollup of 11 pull requests - Successful merges: #32131, #32199, #32257, #32325, #32435, #32447, #32448, #32456, #32469, #32476, #32482 - Failed merges: #32240

This commit rewrites the `std::sync::Once` primitive with poisoning in mind in light of rust-lang#31688. Currently a panic in the initialization closure will cause future initialization closures to run, but the purpose of a Once is usually to initialize some global state so it's highly likely that the global state is corrupt if a panic happened. The same strategy of a mutex is taken where a panic is propagated by default. A new API, `call_once_force`, was added to subvert panics like is available on Mutex as well (for when panicking is handled internally). Adding this support was a significant enough change to the implementation that it was just completely rewritten from scratch, primarily to avoid using a `StaticMutex` which needs to have `destroy()` called on it at some point (a pain to do). Closes rust-lang#31688

alexcrichton · 2016-03-26T17:33:52Z

@bors: r=aturon c966c33

bors · 2016-03-26T22:14:29Z

⌛ Testing commit c966c33 with merge 97ec69f...

std: Rewrite Once with poisoning This commit rewrites the `std::sync::Once` primitive with poisoning in mind in light of #31688. Currently a panic in the initialization closure will cause future initialization closures to run, but the purpose of a Once is usually to initialize some global state so it's highly likely that the global state is corrupt if a panic happened. The same strategy of a mutex is taken where a panic is propagated by default. A new API, `call_once_force`, was added to subvert panics like is available on Mutex as well (for when panicking is handled internally). Adding this support was a significant enough change to the implementation that it was just completely rewritten from scratch, primarily to avoid using a `StaticMutex` which needs to have `destroy()` called on it at some point (a pain to do). Closes #31688

bors · 2016-03-27T00:29:57Z

huonw · 2016-04-21T02:01:12Z

There is actually a use case for panicking in the Once initializer: it's the only way to abort initialization. This could be useful for, e.g., caching result of an expensive RPC call that is expected to succeed but may fail on occasion.

Was this discussed further? (I've unfortunately missed quite a few libs meetings recently.)

It seems like it might be worth providing a separate function something like try_call_once<F: FnOnce() -> Status>(...) enum Status { Succeeded, Retry } that handles this case in a non-hacky way.

aturon · 2016-04-21T05:22:24Z

@huonw IIRC, we decided that functionality like this can be easily added in the future if desired.

alexcrichton assigned aturon Mar 18, 2016

huonw reviewed Mar 18, 2016
View reviewed changes

alexcrichton force-pushed the panic-once branch from 8a69160 to a3c3d8a Compare March 18, 2016 17:45

aturon reviewed Mar 25, 2016
View reviewed changes

alexcrichton force-pushed the panic-once branch from a3c3d8a to 00f6841 Compare March 25, 2016 15:59

aturon reviewed Mar 25, 2016
View reviewed changes

alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 25, 2016

alexcrichton force-pushed the panic-once branch 2 times, most recently from 316c24e to a755a67 Compare March 25, 2016 23:28

Manishearth mentioned this pull request Mar 26, 2016

Rollup of 18 pull requests #32495

Closed

Manishearth mentioned this pull request Mar 26, 2016

Rollup of 11 pull requests #32496

Merged

bors added a commit that referenced this pull request Mar 26, 2016

Auto merge of #32496 - Manishearth:rollup, r=Manishearth

d322f99

Rollup of 11 pull requests - Successful merges: #32131, #32199, #32257, #32325, #32435, #32447, #32448, #32456, #32469, #32476, #32482 - Failed merges: #32240

alexcrichton force-pushed the panic-once branch from a755a67 to c966c33 Compare March 26, 2016 17:33

bors merged commit c966c33 into rust-lang:master Mar 27, 2016

bors mentioned this pull request Mar 27, 2016

Alter Once to remove unnecessary SeqCst usage #31650

Closed

alexcrichton deleted the panic-once branch March 27, 2016 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: Rewrite Once with poisoning #32325

std: Rewrite Once with poisoning #32325

alexcrichton commented Mar 18, 2016

alexcrichton commented Mar 18, 2016

sfackler commented Mar 18, 2016

alexcrichton commented Mar 18, 2016

huonw Mar 18, 2016

alexcrichton Mar 18, 2016

aturon Mar 25, 2016

Stebalien commented Mar 18, 2016

alexcrichton commented Mar 18, 2016

aturon Mar 25, 2016

aturon Mar 25, 2016

aturon commented Mar 25, 2016

aturon Mar 25, 2016

alexcrichton commented Mar 25, 2016

Manishearth commented Mar 26, 2016

alexcrichton commented Mar 26, 2016

bors commented Mar 26, 2016

bors commented Mar 27, 2016

huonw commented Apr 21, 2016

aturon commented Apr 21, 2016

std: Rewrite Once with poisoning #32325

std: Rewrite Once with poisoning #32325

Conversation

alexcrichton commented Mar 18, 2016

alexcrichton commented Mar 18, 2016

sfackler commented Mar 18, 2016

alexcrichton commented Mar 18, 2016

huonw Mar 18, 2016

Choose a reason for hiding this comment

alexcrichton Mar 18, 2016

Choose a reason for hiding this comment

aturon Mar 25, 2016

Choose a reason for hiding this comment

Stebalien commented Mar 18, 2016

alexcrichton commented Mar 18, 2016

aturon Mar 25, 2016

Choose a reason for hiding this comment

aturon Mar 25, 2016

Choose a reason for hiding this comment

aturon commented Mar 25, 2016

aturon Mar 25, 2016

Choose a reason for hiding this comment

alexcrichton commented Mar 25, 2016

Manishearth commented Mar 26, 2016

alexcrichton commented Mar 26, 2016

bors commented Mar 26, 2016

bors commented Mar 27, 2016

huonw commented Apr 21, 2016

aturon commented Apr 21, 2016