Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Alice Ryhl]

On Mon, Mar 11, 2024 at 4:15 PM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>
> On 3/11/24 10:03, Alice Ryhl wrote:
> > On Sat, Mar 9, 2024 at 2:02 PM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
> >>
> >> On 2/28/24 14:00, Alice Ryhl wrote:
> >>> +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
> >>> +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
> >>> +        // are no other arcs, and we can create a `UniqueArc`.
> >>
> >> This comment is not explaining why it is safe to call
> >> `refcount_dec_and_test` on `refcount`.
> >> It dose however explain what you are going to do, so please keep it, but
> >> not as a SAFETY comment.
> >
> > I'll reword.
> >
> >>> +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> >>> +        if is_zero {
> >>> +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> >>> +            // accesses to the refcount.
> >>> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> >>> +
> >>> +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
> >>> +            // since an `Arc` is pinned.
> >>
> >> The `unsafe` block is only needed due to the `new_unchecked` call, which
> >> you could avoid by using `.into()`. The `SAFETY` should also be an
> >> `INVARIANT` comment instead.
> >>
> >>> +            unsafe {
> >>> +                Some(Pin::new_unchecked(UniqueArc {
> >>> +                    inner: Arc::from_inner(me.ptr),
> >>> +                }))
> >>> +            }
> >
> > The from_inner method is also unsafe.
>
> Ah I missed that, might be a good reason to split the block.
> It confused me that the SAFETY comment did not mention why calling
> `new_unchecked` is sound.

I don't mind splitting up the unsafe block into several pieces.

> > I think that using new_unchecked here makes more sense. That method is
> > usually used in the case where something is already pinned, whereas
> > into() is usually used to pin something that was not previously
> > pinned.
>
> I get your argument, but doing it this way avoids an unsafe function
> call. I think it would be fine to use `.into()` in this case.
> Splitting the unsafe block would also be fine with me.

If you are okay with splitting the unsafe block instead, then I prefer
that. I don't think avoiding unsafe blocks is always the best answer;
especially not when you're already using unsafe right next to it.

This reminds me of NonNull::new_unchecked(Box::into_raw(my_box)) vs
NonNull::from(Box::leak(my_box)). The latter is safe, but I don't
necessarily think that makes it the better choice. It's also important
that your code carries the right intent.

Another way to go around it could be to add UniqueArc::from_raw or
from_inner methods, as well as from_raw_pinned and from_inner_pinned,
and then use those here.

Alice