-
Notifications
You must be signed in to change notification settings - Fork 46
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 Arc::into_unique
for converting back to UniqueArc
(redux).
#109
base: master
Are you sure you want to change the base?
Conversation
cc @steffahn |
Arc::into_unique
for converting back to UniqueArc
.Arc::into_unique
for converting back to UniqueArc
(redux).
Tests are now fixed after a little bit of wrangling. |
Probably should have a comment about the necessity of AcqRel (and the other options on the table). I'll defer merging this until @steffahn has a look. |
As noted in #103 (comment) So the implementation is fine this way; however as long as the comment explaining this code just states a simple
I would expect that the could should do exactly as As for test case, I'd appreciate if something like let a = Arc::new(0);
let b = a.clone();
let t1 = std::thread::spawn(move || {
let _value = *b;
});
let t2 = std::thread::spawn(move || {
std::thread::sleep(std::time::Duration::from_millis(100));
if let Some(mut u) = Arc::into_unique(a) {
*u += 1
}
});
t1.join().unwrap();
t2.join().unwrap(); was also added (in addition to the existing one without any sleep) as that setup should more reliably catch the particular previous issue of insufficient synchronization, without relying too much on “random” execution order decisions of miri. The existing sleep-less, join-less variant is useful, too, because it can have more different orders of execution. So assuming e.g. a setting where we (or someone else) would be running multiple rounds of |
@Manishearth @steffahn I've updated the inline comments around Let me know if this addresses your concerns. |
Looks good to me. |
This is a spiritual equivalent to
std::sync::Arc::into_inner
where we can let sharedArc
instances be atomically dropped or converted back to an owningUniqueArc
.Notes
This is a reworking of #103, where we now use
AcqRel
ordering (instead ofRelease
) when decrementing the strong count, in order to avoid a data race. We've also added an additional unit test that is able to exercise the data race if the ordering wasRelease
instead.Admittedly, this unit test might not even properly detect the data race depending on nightly version and the instruction ordering in the compiled test binary... but it was much simpler to add than a full
loom
test, and hopefully can serve as some additional incremental coverage in the meantime?