Messages in this thread | | | From | Alice Ryhl <> | Date | Mon, 11 Mar 2024 16:45:24 +0100 | Subject | Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop` |
| |
On Mon, Mar 11, 2024 at 4:35 PM Alice Ryhl <aliceryhl@google.com> 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@proton.me> 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
| |