Tracking issue for Rc/Arc stabilization

[aturon]

There are a number of unstable features related to Rc and Arc, including both weak references and uniqueness. An RFC to remove weak pointers failed, and the current consensus seems to be to stabilize these APIs after nailing down more precise semantics for uniqueness.

cc @gankro

[abonander]

Please consider #24789. Dunno if it belongs here or in the RFCs repo since it's a semantics change that wouldn't break much.

[Gankra]

Current plan:

• Stabilize weak pointers (Do we want it as a normal method? General convention is static methods -- but it's symmetric to clone)
• Stabilize try_unwrap only looking at strong count (but properly considering weak count to clean up)
• Stabilize make_unique which clones if strong != 1 || weak != 0
• Stabilize get_mut which only succeeds if strong == 1 && weak == 0
• don't stabilize is_unique because there are two notions of uniqueness (as seen above) -- can consider exposing both notions, but they're both racey to act on in Arc, I think?
• maybe stabilize strong_count and weak_count? They're harmless to support. However they're inherently racey to act on in Arc, which makes them a bit questionable.

Basically: stabilize the actually useful functionality, don't stabilize introspection pending more work.

[Gankra]

CC @SimonSapin

[Gankra]

Oh and maybe bikeshed naming? They seem perfectly cromulant to me, though make_unique has the annoying notion of uniqueness. make_mut is a potential alternative, hinting at the CoW semantics.

[Gankra]

I am currently drafting a PR to this affect.

[jethrogb]

Is alloc::arc::strong_count part of this issue or #27700?

However they're inherently racey to act on in Arc, which makes them a bit questionable.

It's not racy if you protect the Arc with a Mutex. I have a use case for this in namedlock-rs.

[Gankra]

@jethrogb this issue -- Arc is defined in liballoc but it's intended to be used inside std::sync

That issue largely concerns the actual alloc::heap API

[jethrogb]

Thanks for the clarifcation. I mention this since using it currently requires the alloc feature. Nevermind, there is the arc_counts feature as of 1.2.0.

[Gankra]

@jethrogb it only needs that feature because you're accessing the functionality through the alloc crate instead of std. The whole alloc crate is unstable to directly access.

[SimonSapin]

Of things that is still unstable but not deprecated in #27760:

• I think arc_counts methods should be deprecated since they are racy.

• Rc::is_unique with its current meaning (strong == 1 && weak == 0) can be emulated with Rc::get_mut(x).is_some(), so I don’t mind deprecating it.

• Rc::try_unwrap(x).is_ok(), however, would be destructive. I have a use case for testing Rc::strong_count(x) == 1: I want to know whether Node will be dropped when a given Rc<Node> is dropped so I can start deconstructing it to make Drop non-recursive. I could use Rc::try_unwrap, but keeping (a stack of) Rc<Node> around is much cheaper than moving Node with Rc::try_unwrap and keeping (a stack of) Node around, since Node is much larger than Rc<Node> (which is pointer-sized).

  Therefore, I don’t mind deprecating Rc::strong_count and Rc::weak_count as long as there is some way to test Rc::strong_count(x) == 1 non-destructively. Perhaps a dedicated method? But I don’t have a good name in mind, since is_unique is somewhat ambiguous.

[Gankra]

@SimonSapin sounds like all you need is would_unwrap?

[alexcrichton]

This feature is now entering its final comment period for stabilization.

I believe that @gankro will post a PR soon with various tweaks to do the final prep for stabilization

[Gankra]

Building now and dealing with fallout. Changes proposed:

• rc.make_unique -> Rc::make_mut(&mut rc)
• add Arc::make_mut
• deprecate strong_count and weak_count
• deprecate is_unique
• add Rc::would_unwrap(&Rc) to evaluate whether try_unwrap will succeed
• don't add Arc::would_unwrap because it's racy
• rc.downgrade() -> Rc::downgrade(&Rc) -- note that there cannot be a deprecation grace period due to UFCS naming conflicts!
• arc.downgrade() -> Arc::downgrade(&Arc) -- ditto!

[alexcrichton]

@gankro that sounds good to me, but I'd consider Rc::would_unwrap separately for stabilization from the rest of these features.

[jethrogb]

• rc.make_unique -> Rc::make_mut(&mut rc)
• add Arc::make_mut

What's happening with Arc::make_unique?

• don't add Arc::would_unwrap because it's racy

What about Arc::try_unwrap?

• deprecate strong_count and weak_count

I don't think strong_count/weak_count should be deprecated. Consider a data structure that opaquely owns some data (say, keys in a HashMap). You store an Rc/Arc in it. Now, if you also have a copy of that Rc/Arc, there is no way (after removing the count accessors) to tell whether you have the last copy besides the one in the data structure. Since they are racy for Arc, I propose declaring those unsafe instead, signaling to users that they need to manually synchronize the use.

[Gankra]

@jethrogb Oh whoops, I got make_unique and try_unwrap a bit mixed up.

I'm not a fan of that argument -- if you want to intropsect uniqueness you can use try_unwrap/would_unwrap and get_mut.

[jethrogb]

if you want to intropsect uniqueness you can use try_unwrap/would_unwrap and get_mut.

Except I just told you that those functions are not sufficient.

[Gankra]

@jethrogb Ah yes, I misread the example.

Still, this seems extraordinarily niche.

Also: that's not what unsafe is for. It's impossible to introduce memory safety with the counts -- the information they yield just borders on useless (you need to do proper CAS-loop stuff, which only the guts of Arc is prepared to do).

[jethrogb]

Code example

#![feature(arc_unique,arc_counts)]

use std::sync::Arc;
use std::hash::{Hash,Hasher};
use std::collections::HashMap;

#[derive(Clone)]
pub struct ArcHashKey<T>(pub Arc<T>);
impl<T: PartialEq> PartialEq for ArcHashKey<T> {
    fn eq(&self, other: &ArcHashKey<T>) -> bool { self.0.eq(&other.0) }
}
impl<T: Eq> Eq for ArcHashKey<T> { }
impl<T: Hash> Hash for ArcHashKey<T> {
    fn hash<H: Hasher>(&self, state: &mut H) { self.0.hash(state) }
}

fn main() {
    let mut map=HashMap::new();
    let mut key=ArcHashKey(Arc::new("key"));
    map.insert(key.clone(),"value");
    // hand out some references to key, thread some things
    if Arc::strong_count(&key.0)==2 {
        map.remove(&key);
        println!("all threads are done with the arc: {:?}",Arc::get_mut(&mut key.0));
    }
}

[SimonSapin]

@gankro Yes, Rc::would_unwrap(&Rc<T>) -> bool works for my use case described above.

[eefriedman]

One method that hasn't been proposed is a variant of get_mut() which only considers the number of strong references (and moves the contents to a new allocation if necessary); would that be useful?

[Gankra]

@eefriedman you need to consider the weak references in general because otherwise they can promote themselves to a strong reference and then access the supposedly unaliased &mut. Or am I misunderstanding?

[SimonSapin]

I think @eefriedman means something equivalent to if Rc::would_unwrap(x) { Some(Rc::make_mut(x)) } else { None }.

[SimonSapin]

With #27837 landed this is looking pretty good to me. What’s the next step in the process? I think we can stabilize the rc_weak, rc_unique, arc_weak, and arc_unique features as-is. I’d like to stabilize rc_would_unwrap as well, but I’m the only one who asked for it :) This would only leave rc_counts and arc_counts unstable.

[Gankra]

Stabilize *_weak and *_unique is my proposal as well.

rc_would_unwrap definitely needs more time to bake as unstable

I'd like to deprecated *_counts, though there is some opposition to this (even arc_counts can arguably be used correctly if you're very careful).

[alexcrichton]

Nominating for discussion to move into FCP

[alexcrichton]

The libs team decided today to move this into FCP, specifically:

• std::rc::Weak
• std::rc::Weak::upgrade
• std::rc::Rc::downgrade
• std::rc::Rc::try_unwrap
• std::rc::Rc::get_mut
• std::rc::Rc::make_mut
• std::sync::Weak
• std::sync::Weak::upgrade
• std::sync::Arc::downgrade
• std::sync::Arc::try_unwrap
• std::sync::Arc::make_mut
• std::sync::Arc::get_mut

[jethrogb]

I'm not quite sure about the process here. You don't mention the count methods. Does that mean that they'll be deprecated, or that no decision is being made just yet?

[alexcrichton]

Ah yes, sorry, all other methods will remain unstable for now, and follow-up issues will be opened to track their unstable progress.