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

[Benno Lossin]

On 3/11/24 16:45, Alice Ryhl wrote:
> 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@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.
> 
> 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?

That also looks good to me.

-- 
Cheers,
Benno