Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for arc_new_cyclic #75861

Closed
4 tasks done
KodrAus opened this issue Aug 24, 2020 · 17 comments · Fixed by #90666
Closed
4 tasks done

Tracking Issue for arc_new_cyclic #75861

KodrAus opened this issue Aug 24, 2020 · 17 comments · Fixed by #90666
Labels
A-concurrency Area: Concurrency C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@KodrAus
Copy link
Contributor

KodrAus commented Aug 24, 2020

The feature gate for the issue is #![feature(arc_new_cyclic)].

Tracking the following API:

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

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

Steps

Unresolved Questions

@KodrAus KodrAus added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Aug 24, 2020
@KodrAus KodrAus added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 24, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 24, 2020
@mental32
Copy link
Contributor

mental32 commented Aug 24, 2020

@Dylan-DPC @KodrAus allow me a day to implement arc_new_cyclic on Rc :)

matklad added a commit to matklad/rust that referenced this issue Sep 4, 2020
matklad added a commit to matklad/rust that referenced this issue Sep 4, 2020
matklad added a commit to matklad/rust that referenced this issue Sep 4, 2020
matklad added a commit to matklad/rust that referenced this issue Sep 4, 2020
matklad added a commit to matklad/rust that referenced this issue Sep 4, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 4, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 4, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 5, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 5, 2020
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Sep 10, 2020
@KodrAus KodrAus added the A-concurrency Area: Concurrency label Sep 22, 2020
@eduardosm
Copy link
Contributor

I think "Implement Rc::new_cyclic" can be marked as done.

@ObsidianMinor
Copy link
Contributor

Is there a specific reason why the definitions of Arc::new_cyclic and Rc::new_cyclic use impl FnOnce instead of an explicit generic type?

@Thomasdezeeuw
Copy link
Contributor

I need a version of Arc::new_cyclic where the creation function can fail, would such a function be acceptable? Or should I open another issue for that?

The API would look something like the following:

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

@philip-peterson
Copy link

philip-peterson commented May 21, 2021

It would be nice to have multiple arities of this function, i.e.

fn new_cyclic_2<A, B>(data_fn: impl FnOnce(&Weak<A>, &Weak<B>) -> (A, B)) -> (Arc<A>, Arc<B>) {
 ...
}

and ditto for 3, etc. The use case would be for when your type B also contains a Weak<A> and/or vice-versa.

@Johannesd3

This comment has been minimized.

@philip-peterson
Copy link

@Johannesd3 Thanks for that, it's a lot cleaner than what I got from experimenting myself.

One thing I noticed when testing it though is that there is a bug with arc_new_cyclic as far as I can tell. The docs state:

Attempting to upgrade the weak reference before this function returns will result in a None value.

To me this implies that after the function returns, it will be safe. (I know that's not technically what it says, but...) also:

the weak reference may be cloned freely and stored for use at a later time.

When putting this to test, the upgrade fails:

#[test]
fn arc_new_cyclic_deferred_upgrades_succeed() {
    let mut foo = None;
    Arc::new_cyclic(|weak_a| {
        foo = Some(weak_a.clone());

        (1,)
    });
    assert_eq!(*foo.unwrap().upgrade().unwrap(), (1,));
}
failures:

---- tests::arc_new_cyclic_deferred_upgrades_succeed stdout ----
thread 'tests::arc_new_cyclic_deferred_upgrades_succeed' panicked at 'called `Option::unwrap()` on a `None` value', src/lib.rs:17:44
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@ogoffart
Copy link
Contributor

@philip-peterson That's because you did not keep the Arc alive, so it is destroyed after the function call. fix it by assigning the resulting Arc to some value: let arc = Arc::new_cyclic( ...

@philip-peterson
Copy link

philip-peterson commented May 22, 2021

@ogoffart ah, thank you!!

I created a third party crate to house the multi-arity versions of new_cyclic at https://github.com/philip-peterson/arc_new_cyclic_n -- @Johannesd3 thank you for your help on this, please let me know if you have any disagreements with this crate existing.

@tmandry
Copy link
Member

tmandry commented Jul 26, 2021

Are additional docs required for this feature? The documentation in std looks sufficient. The rustc guide says the reference must be updated, but I'm pretty sure that doesn't apply to library APIs.

The unstable book has a stub entry, that might need a blurb on the feature but I'm not sure it matters for stabilization.

Are there any other blockers for stabilization?

@chris-laplante
Copy link

I need a version of Arc::new_cyclic where the creation function can fail, would such a function be acceptable? Or should I open another issue for that?

With assistance from the Forum, I found that implementing try_new_cyclic appears pretty simple: https://users.rust-lang.org/t/implementing-an-rc-try-new-cyclic/62945. To echo @Thomasdezeeuw, should I submit a new issue / pull request for it?

@bdbai
Copy link
Contributor

bdbai commented Nov 9, 2021

@chris-laplante try_* functions in Arc are related to allocators. Shall we pick another name?

@chris-laplante
Copy link

@chris-laplante try_* functions in Arc are related to allocators. Shall we pick another name?

How about new_cyclic_fallible?

@dtolnay
Copy link
Member

dtolnay commented Nov 26, 2021

What's a real world use case for try_new_cyclic where the fallible work can't be done before the new_cyclic call, possibly putting Weak::new() placeholders into the object? That way new_cyclic only needs to wire in the self references into an exclusively owned object.

let mut obj = try_obj()?;
let rc = Rc::new_cyclic(move |this| {
    obj.this = Weak::clone(this);
    obj
});

@Thomasdezeeuw
Copy link
Contributor

@dtolnay I have a use case where a fallible Arc::new_cyclic would help, although I'm currently working around with an additional type. Basically I need to make a system call (the fallible part) to create a special fd (epoll in this case), which I then put in a shared structure (using an Arc). But this shared structure contains a type (waker_id, see code below) which is created using an Arc reference to the shared structure itself. But like I said I'm working around with an additional type, so it's a livable workaround.

The code is available here:
https://github.com/Thomasdezeeuw/heph/blob/aa6ccf79de85c0abccb6a26f920c7d9d405e80cf/src/rt/coordinator.rs#L103-L107

With the call to setup:
https://github.com/Thomasdezeeuw/heph/blob/aa6ccf79de85c0abccb6a26f920c7d9d405e80cf/src/rt/shared/mod.rs#L108-L113

And the intermediate type:
https://github.com/Thomasdezeeuw/heph/blob/aa6ccf79de85c0abccb6a26f920c7d9d405e80cf/src/rt/shared/mod.rs#L33-L46

@ckaran
Copy link

ckaran commented Jan 14, 2022

How close is this to stabilization?

@m-ou-se
Copy link
Member

m-ou-se commented Jan 22, 2022

How close is this to stabilization?

Very close! The FCP just finished and I approved the stabilization PR just now. It'll be stable in Rust 1.60.0.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2022
Stabilize arc_new_cyclic

This stabilizes feature `arc_new_cyclic` as the implementation has been merged for one year and there is no unresolved questions. The FCP is not started yet.

Closes rust-lang#75861 .

`@rustbot` label +T-libs-api
@bors bors closed this as completed in 59d9ad9 Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.