Stabilize Arc::{increment,decrement}_strong_count

[yoshuawuyts]

Tracking issue: #71983

Stabilizes Arc::{incr,decr}_strong_count, enabling unsafely incrementing an decrementing the Arc strong count directly with fewer gotchas. This API was first introduced on nightly six months ago, and has not seen any changes since. The initial PR showed two existing pieces of code that would benefit from this API, and included a change inside the stdlib to use this.

Given the small surface area, predictable use, and no changes since introduction, I'd like to propose we stabilize this.

closes #71983
r? @Mark-Simulacrum

Links

• Initial implementation
• Motivation from #68700
• Real world example in an executor

[Mark-Simulacrum]

I am forgetting now -- is there a reason we didn't add this on Rc? I don't quickly see any discussion about that, but it seems like they'd be equally useful there ergonomics wise, though it's somewhat rarer (I suspect) to be stashing away Rc's in atomics or whatever where you're hiding them.

[yoshuawuyts]

@Mark-Simulacrum I don't believe we discussed it, but there's no reason not to as far as I recall — I'm planning to add it to Rc and both Weak variants as a follow-up.

If there are no objections to stabilizing the Arc methods though, it'd be nice if we could stabilize that sooner.

[Mark-Simulacrum]

This stabilizes the following methods on Arc, intended for manipulating the strong reference count without error-prone code required before (e.g., cloning the arc and forgetting it to increment or double-dropping an arc to decrement).

impl<T: ?Sized> Arc<T> {
    pub unsafe fn incr_strong_count(ptr: *const T);
    pub unsafe fn decr_strong_count(ptr: *const T);
}

The only reason these APIs are unsafe is that we take *const T rather than an Arc<T> or &Arc<T>; they are almost always used after Arc::into_raw, so this unsafety is deemed warranted. If we were to take an actual instance of Arc<T> the methods would be less useful as the caller would need to be careful to not re-drop the Arc in the case of &Arc<T> and/or not clone the Arc on passing it in in the owned case. Generally it's expected that this API is easier to use.

r? @dtolnay to kick off libs FCP on this.

[nagisa]

Should incr and decr be expanded to a proper word before stabilizing?

[yoshuawuyts]

@nagisa I find that increment_strong_count and decrement_strong_count feel rather verbose and would prefer we keep the naming as-is. The current names also have a nice benefit that they could also stand for "increase" and "decrease" which may make them easier to memorize.

[dtolnay]

I am on board with these. #70733 has compelling discussion that this functionality is worth exposing in the standard library, and that these signatures involving *const T are the best ones.

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @KodrAus
• @dtolnay
• @m-ou-se
• @sfackler
• @withoutboats

Concerns:

• clone_from_raw instead resolved by Stabilize Arc::{increment,decrement}_strong_count #79285 (comment)
• naming resolved by Stabilize Arc::{increment,decrement}_strong_count #79285 (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]

RE naming: I do have a mild preference for increment/decrement instead of abbreviating them. While these are useful, they are niche, so I don't really buy the verbosity argument. I don't think it's that important to reduce verbosity for infrequently used routines.

RE docs: Would it be possible to add some clarity to the docs explaining specifically why someone might want to use these routines?

[tmiasko]

Personally I found those functions to be rather underwhelming. They cater to relatively niche use-cases, while having a trivial implementation. Furthermore, their own implementation uses a pattern which is much more generally applicable:

• decrement: Arc::from_raw(ptr);
• increment: let _ = ManuallyDrop::new(Arc::from_raw(ptr)).clone();

[Mark-Simulacrum]

I would call neither of those trivial or easy to follow -- if I see those in the wild, I'm definitely going to expect a comment on them explaining them. I also don't think we guarantee that they will work (there's not really a way we could break that even if we wanted to and ignoring stability guarantees); I've always been rather uncertain about the "clone to increment" pattern externally from std, while internally we can of course use it.

[tmiasko]

I also don't think we guarantee that they will work (there's not really a way we could break that even if we wanted to and ignoring stability guarantees); I've always been rather uncertain about the "clone to increment" pattern externally from std, while internally we can of course use it.

I don't think there is anything contentious about Arc::from_raw or clone. The only part that could be controversial is using ManuallyDrop instead of Arc::into_raw. EDIT: Which might be a sign that a Arc::clone_raw might be more appropriate than Arc::incr_strong_count.

[m-ou-se]

@rfcbot concern naming

See #79285 (comment).

I agree not abbreviating the names would be a bit better. Registering it as a concern so we don't forget.

[withoutboats]

I struggled with the pointer aspect of this API a bit. It seemed strange to combine two steps - offseting to get an arc, and then manipulating the strong count - into one function. Especially so because incrementing the strong count is safe. I was worried that these were overly-tailored to implementing a waker.

What won me over to the current API was the drop problem: users who have a raw pointer have to make sure if they get an Arc from it that they know not to drop it. This is really tricky to get right because its all implicit; implementing it properly in std seems like the right choice.

[yoshuawuyts]

It seems everyone on the libs team has ticked their box! -- but the one outstanding concern is naming. So I'd like to ask: does anyone feel strongly against increment_strong_count, decrement_strong_count?

I've already expressed I prefer the short versions, but I don't feel strong enough about them to block this PR on that. So I want to check if anyone objects to the long-form instead. If not, I'll happily switch this PR over to use increment_strong_count, decrement_strong_count so it can be merged. Thanks!

[yoshuawuyts]

I've update the PR to use the {increment,decrement}_strong_count method names. I believe once CI passes this should be ready for FCP.

[m-ou-se]

@rfcbot resolve naming

[rfcbot]

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

[Mark-Simulacrum]

The RawWaker API would need to forget the resulting Arc rather than making use of it in the clone "method". I think most use cases for this API aren't actually creating Arcs directly. The uses I've had historically have also done similar things to the RawWaker API, for example across the FFI boundary where either side wants to increment the counter on a shared resource, due to cloning, but isn't actually going to pass back the Arc (because nothing is going to interact with the Arc directly). That said I don't disagree that the clone_from_raw API is in some cases a bit cleaner; I think I would rather add both though. These methods are largely aliases for 1-3 lines of code, we can expose more than one incrementing method if we want. But I think there are valid uses for both just incrementing and for incrementing and getting a reference.

[m-ou-se]

@SimonSapin I think you have to be on rfcbot's list to add concerns.

@rfcbot concern clone_from_raw instead

#79285 (comment)

[m-ou-se]

@SimonSapin Since this is blocked on your concern, can you respond to Mark's comment? Thanks!

[SimonSapin]

The RawWaker API would need to forget the resulting Arc rather than making use of it in the clone "method". I think most use cases for this API aren't actually creating Arcs directly.

Ok, I didn’t realize that.

This concern can be marked as resolved.

[m-ou-se]

@rfcbot resolve clone_from_raw instead

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[m-ou-se]

@bors r+ rollup

[bors]

📌 Commit fe4ac95 has been approved by m-ou-se