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

[Alice Ryhl]

On Mon, Mar 11, 2024 at 4:35 PM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> On Mon, Mar 11, 2024 at 4:15 PM Benno Lossin <benno.lossin@protonme> 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.

After looking at the code, I've changed my mind. I will write it like this:

Some(Pin::from(UniqueArc { inner: ManuallyDrop::into_inner(me) }))

With an INVARIANT comment. Does that make sense for you?

Alice