Stabilize arc_new_cyclic by bdbai · Pull Request #90666 · rust-lang/rust · GitHu...

Copy link

Contributor

rust-highfive commented on Nov 7, 2021

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

dtolnay commented on Nov 26, 2021

I like these!

@rust-lang/libs-api
@rfcbot fcp merge

impl<T> Rc<T> {
    pub fn new_cyclic(f: impl FnOnce(&Weak<T>) -> T) -> Rc<T>;
}

impl<T> Arc<T> {
    pub fn new_cyclic(f: impl FnOnce(&Weak<T>) -> T) -> Arc<T>;
}

rfcbot commented on Nov 26, 2021 •

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

• clarify doc example resolved by #90666 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

BurntSushi commented on Dec 2, 2021

I think the docs for this API leave a lot to be desired. I can't figure out what this API is really supposed to do or, more importantly, why or when I should (or shouldn't) use it. Here are some points of confusion that I have after looking at this for a few minutes:

• The name of the API includes the word "cyclic," but nothing in the docs mention it.
• I'm grateful for the existence of the example, but the example doesn't really help me understand what this routine does or why I would want to use it. It kind of seems like a "trivial" example to me, which is fine in some cases, but this API feels like it deserves something better.
• The docs mention a None value, but I don't quite grok this. I don't see any Option<T> anywhere, so it's not clear to me where this None value comes from.
• What happens if data_fn panics? Does a leak occur? Something worse? Should we specify the behavior of this routine if data_fn panics?

My suspicion is that this is a fine API to add, but I'd like to see the docs improved here a bit before signing off.

yaahc commented on Dec 2, 2021

• The docs mention a None value, but I don't quite grok this. I don't see any Option<T> anywhere, so it's not clear to me where this None value comes from.

It looks like the None is referring to calling upgrade on the Weak argument inside of the provided closure. Seems easy to clarify by turning the upgrade reference into a link to the relevant function on Weak.

yaahc commented on Dec 2, 2021 •

• I'm grateful for the existence of the example, but the example doesn't really help me understand what this routine does or why I would want to use it. It kind of seems like a "trivial" example to me, which is fine in some cases, but this API feels like it deserves something better.

I did some digging in the history of this API to see if I couldn't find any better explanations and found this comment which explicitly mentions being able to give out reference counted pointers to ones self. Perhaps the example just needs to additionally include a simple method on Foo, naming notwithstanding.

use std::sync::{Arc, Weak};

struct Foo {
    me: Weak<Foo>,
}

impl Foo {
    /// Construct a reference counted Foo.
    fn new() -> Arc<Self> {
        Arc::new_cyclic(|me| Foo {
            me: me.clone(),
        })
    }

    /// Return a reference counted pointer to Self.
    fn me(&self) -> Arc<Self> {
        self.me.upgrade()
    }
}

yaahc commented on Dec 2, 2021

@rfcbot reviewed
@rfcbot concern clarify doc example

yaahc commented on Dec 3, 2021 •

Fair point, we should definitely also make sure all the docs on Rc::new_cyclic and Arc::new_cyclic are consistent.

The necessary changes as I see them:

• Update Arc::new_cyclic docs to add the methods mentioned above
• Rename the Foo type in the Arc example to Gadget
• Add the second method from the methods mentioned above to the Rc example
• Update the description for both Arc and Rc so that the mention of upgrade becomes a link to Weak::upgrade

bdbai commented on Dec 4, 2021 •

Regarding the doc example, I would like to propose the following alternative:

use std::sync::{Arc, Weak};

struct Root {
    left: Leaf,
    right: Leaf,
}

struct Leaf {
    root: Weak<Root>,
}

let tree = Arc::new_cyclic(|me| {
    assert!(root.upgrade().is_none());
    Root {
        left: Leaf { root: me.clone() },
        right: Leaf { root: me.clone() },
    }
});

assert!(tree.left.root.upgrade().is_some());
assert!(Arc::ptr_eq(&tree.right.root.upgrade().unwrap(), &tree));

I noticed there was a section Breaking cycles with Weak in the documentation of Arc<T>, where a tree-like data structure was introduced as an example. I guess the doc example here is the right place to reflect that.

bdbai commented on Dec 4, 2021 •

The description was amended as follows:

/// Constructs a new Arc<T> using a closure data_fn that has access to
/// a weak reference to the constructing Arc<T>.
///
/// Generally, a structure circularly referencing itself, either directly or
/// indirectly, should not hold a strong reference to prevent a memory leak.
/// In data_fn, initialization of T can make use of the weak reference
/// by cloning and storing it inside T for use at a later time.
///
/// Since the new Arc<T> is not fully-constructed until
/// Arc<T>::new_cyclic returns, calling [upgrade] on the weak
/// reference inside data_fn will fail and result in a None value.
///
/// # Panics
/// If data_fn panics, the panic is propagated to the caller, and the
/// temporary [Weak<T>] is dropped normally.
///
/// # Example
/// ...
/// [upgrade]: Weak::upgrade

Since I am not a native English speaker, I would appreciate it if any suggestions could be made in order to make readers feel more idiomatic.

yaahc commented on Dec 6, 2021 •

@bdbai I edited a couple of the verb tenses in your updated doc comment but otherwise everything in your example looked correct grammatically. Am I correct in understanding that you've not made the mentioned changes in the PR itself?

As for the example, I have a slight preference for the example that I suggested earlier since it seems closer to real world examples while still being relatively brief, where as I feel like the tree example you linked is a simplified form of what a real world tree structure would look like and is still a much longer example. That said, I'm fine with either example, so I'll leave it up to other libs team members to voice a stronger preference.

I would still like to see the Rc and Arc docs updated to match regardless of which example we go with.

bdbai commented on Dec 7, 2021

@yaahc

I edited a couple of the verb tenses in your updated doc comment but otherwise everything in your example looked correct grammatically.

Thanks!

Am I correct in understanding that you've not made the mentioned changes in the PR itself?

I failed to mention that the changes of the text description would be committed into the PR once it got corrected and approved. Sorry about that.

I would still like to see the Rc and Arc docs updated to match regardless of which example we go with.

Sure. I will update both while changing the codebase. For the sake of brevity, I would like to mention only one of them during discussion since the both does not differ in terms of this API.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

bdbai commented on Dec 13, 2021

@yaahc The documentation in Arc::new_cyclic and Rc::new_cyclic has been updated.

btw why did rustbot add the T-compiler tag?

yaahc commented on Dec 14, 2021

@bdbai not sure what's up with the T-compiler label but the updates look great!

@rfcbot resolve clarify doc example

veber-alex commented 22 days ago

Can we change the signatures of those methods to use "normal generics" instead of impl Trait ?
It seems to be more consistent with the rest of the standard library.

bdbai commented 21 days ago

@veber-alex fixed.

rfcbot commented 8 days ago

This is now entering its final comment period, as per the review above.

wx-csy commented 6 days ago

Can't wait for the stabilization to build data structures with circular references!

Currently, this function takes a closure to initialize the arc, but it operates badly with async programming. I think we can offer an API that returns a pair of (Initializer, Weak<T>), where the non-clonable Initializer can be used to initialize the arc and after that, the weak pointer can be promoted to a strong pointer. In this case, I can pass the initializer to an async block then do the initialization asynchronously.

bdbai commented 6 days ago •

@wx-csy good point!
I suggest this feature may be generalized as ArcBuilder (or TempArc?) with the following API interface:

struct ArcBuilder<T> { ... }
impl<T> ArcBuilder<T> {
    fn new() -> Self;
    fn get_weak(&self) -> &Weak<T>;
    fn with_value(self, value: T) -> Arc<T>;
}

Once implemented, it will also address the need of fallible cyclic Arc creation mentioned here. Furthermore, interoperability with MaybeUninit can be added to it.

Since the pair of new_cyclic functions have been in the standard library for a while, reworking this feature may introduce breaking changes. @yaahc what do you think?

m-ou-se commented 6 days ago

That sounds like a discussion to be had on the tracking issue, before stabilizing this: #75861

wx-csy commented 4 days ago

@wx-csy good point! I suggest this feature may be generalized as ArcBuilder (or TempArc?) with the following API interface:

struct ArcBuilder<T> { ... }
impl<T> ArcBuilder<T> {
    fn new() -> Self;
    fn get_weak(&self) -> &Weak<T>;
    fn with_value(self, value: T) -> Arc<T>;
}

Once implemented, it will also address the need of fallible cyclic Arc creation mentioned here. Furthermore, interoperability with MaybeUninit can be added to it.

Since the pair of new_cyclic functions have been in the standard library for a while, reworking this feature may introduce breaking changes. @yaahc what do you think?

Well, I think it's fine to move forward stabilizing new-cyclic and just add ArcBuilder later. After that, we may mark new-cyclic as a shorthand for ArcBuilder.

Btw, we may additionally return an ArcBuilder when unwrapping the last strong pointer (e.g., in Arc::try_unwrap), so that we may reinitialize the arc within exactly the same allocation, though I've not yet found a real-world use case of this :)