Implementing an Rc::try_new_cyclic

In the tracking issue for Arc/Rc new_cyclic, there is a suggestion for also implementing a try_new_cyclic, which would look like this:

impl<T> Rc<T> {
    pub fn try_new_cyclic<F, E>(data_fn: F) -> T) -> Result<Rc<T>, E>
    where
        F: FnOnce(&Weak<T>) -> Result<T, E>,
    {
        // ...
    }
}

I wanted to try implementing it myself.

Here is the implementation of new_cyclic on master currently:

pub fn new_cyclic(data_fn: impl FnOnce(&Weak<T>) -> T) -> Rc<T> {
    // Construct the inner in the "uninitialized" state with a single
    // weak reference.
    let uninit_ptr: NonNull<_> = Box::leak(box RcBox {
        strong: Cell::new(0),
        weak: Cell::new(1),
        value: mem::MaybeUninit::<T>::uninit(),
    })
    .into();

    let init_ptr: NonNull<RcBox<T>> = uninit_ptr.cast();

    let weak = Weak { ptr: init_ptr };

    // It's important we don't give up ownership of the weak pointer, or
    // else the memory might be freed by the time `data_fn` returns. If
    // we really wanted to pass ownership, we could create an additional
    // weak pointer for ourselves, but this would result in additional
    // updates to the weak reference count which might not be necessary
    // otherwise.
    let data = data_fn(&weak);

    unsafe {
        let inner = init_ptr.as_ptr();
        ptr::write(ptr::addr_of_mut!((*inner).value), data);

        let prev_value = (*inner).strong.get();
        debug_assert_eq!(prev_value, 0, "No prior strong references should exist");
        (*inner).strong.set(1);
    }

    let strong = Rc::from_inner(init_ptr);

    // Strong references should collectively own a shared weak reference,
    // so don't run the destructor for our old weak reference.
    mem::forget(weak);
    strong
}

Without knowing much about unsafe code, my naïve attempt to implement try_new_cyclic would be (starting with new_cyclic as a base):

  1. Change this line:
    let data = data_fn(&weak);
    to:
    let data = data_fn(&weak)?;
  2. Change the return from:
    strong
    to:
    Ok(strong)

I downloaded the rust repo and made these modifications and it seems to work. However, I think that in the Err case the Box contents will be leaked. So perhaps something like this is required?

    let data = match data_fn(&weak) {
        Ok(val) => val,
        Err(err) => {
            unsafe {
                drop(Box::from_raw(uninit_ptr.as_ptr()));
            }
            return Err(err);
        }
    };

P.S. I know about Miri but don't really know how to run it against the rust library that I built myself.

I don't think there is a leak, since that's the role of the weak (which kind of acts as a drop guard in that regard); I'd personally have changed a tiny bit the code for more readability:

+   // Transfer ownership of the allocation to this `weak`
    let weak = Weak { ptr: init_ptr };

    ...

-   let strong = Rc::from_inner(init_ptr);

-   // Strong references should collectively own a shared weak reference,
-   // so don't run the destructor for our old weak reference.
-   mem::forget(weak);
+   // Directly upgrade the weak into a fully owned `Rc`, transferring ownership
+   let strong = Rc::from_inner(ManuallyDrop::new(weak).ptr);

It thus looks good to me :slightly_smiling_face:

1 Like

Oh right, I missed that.