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

Add Arc::into_unique for converting back to UniqueArc (redux). #109

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tobz
Copy link
Contributor

@tobz tobz commented Dec 29, 2024

This is a spiritual equivalent to std::sync::Arc::into_inner where we can let shared Arc instances be atomically dropped or converted back to an owning UniqueArc.

Notes

This is a reworking of #103, where we now use AcqRel ordering (instead of Release) 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 was Release 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?

@tobz
Copy link
Contributor Author

tobz commented Dec 29, 2024

cc @steffahn

@tobz tobz changed the title Add Arc::into_unique for converting back to UniqueArc. Add Arc::into_unique for converting back to UniqueArc (redux). Dec 29, 2024
@tobz
Copy link
Contributor Author

tobz commented Jan 1, 2025

Tests are now fixed after a little bit of wrangling.

@Manishearth
Copy link
Owner

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.

@steffahn
Copy link
Contributor

steffahn commented Jan 2, 2025

As noted in #103 (comment) AcqRel in this place is definitely sufficient; as would be Release paired with the subsequent .load(Acquire) like Drop does, and the store back to 1 doesn't need any ordering at all (not even atomic operations, necessarily).

So the implementation is fine this way; however as long as the comment explaining this code just states a simple

Perform the same logic as `drop_inner`

I would expect that the could should do exactly as drop_inner does; i.e. the two-step Release-then-Acquire procedure. If you'd like to use AcqRel, there should be a short explanation that this has only stronger synchronization guarantees than the 2 steps in drop_inner. Either way, some comment should also explicitly point out the importance of some Acquire ordering being involved; in drop_inner that's a 20-line comment. Here, you could probably make it relatively short; e.g. referring the reader to the existing in-code documentation of drop_inner for more context, explaining that “deletion of the data” there, and “mutable access to the data” here is analogous; and perhaps even a mention of https://github.com/Manishearth/triomphe/issues/106 could also be useful.


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 miri tests with different seeds for randomness each time, then such a test can cover more ground in principle.

@tobz
Copy link
Contributor Author

tobz commented Jan 4, 2025

@Manishearth @steffahn I've updated the inline comments around fetch_sub(AcqRel) and their importance, as well as added the additional unit test layout that @steffahn recommended.

Let me know if this addresses your concerns.

@steffahn
Copy link
Contributor

steffahn commented Jan 4, 2025

Looks good to me.

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.

3 participants