Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type. #79665

steffahn · 2020-12-03T16:00:09Z

Re-opening #75911 after it was closed due to inactivity (sorry for having been so inactive)

New commits since since previous PR are starting from 5d81f76:
- For now that’s:
  5d81f76
  00d50fe
  ca7066d
  948e402
  7d43bcb
  1e28132
  81623f4
  f57de56
  cd444ca
  ecb85f0
- earlier commits were already present in the previous PR and are only updated due to a rebase to the current master
In particular
- regarding this review, @poliorcetics @CAD97 - 5d81f76 adds a // SAFETY comment
- regarding this review, @LukasKalbertodt - 7d43bcb (and 5d81f76) addresses this
- regarding this review, @LukasKalbertodt - 948e402 addresses this
I’m planning to also implement an Rc::into_inner version for Rc, accordingly. Last previous comment on that topic. Probably being added to this PR very soon.
For now, the only remaining things I’m not 100% sure about yet are:
- Are we are keeping the name be into_inner for now or are there people preferring something different?
- What is the correct way to write documentation in the standard library? I used to have a section in try_unwrap documentation referring to into_inner. I commented that paragraph out for now with a FIXME to make sure it gets re-introduced once into_inner is stabilized; otherwise, it would be confusing if we had a stable function recommended using an unstable one, right? On the other hand the fact that into_inner is not stable doesn’t help the fact that using try_unwrap can introduce bugs... (relevant commit: 00d50fe)
- How do tracking issues work? When are they created, what’s the timeline? I would guess, perhaps, maybe(?), right before merging this PR would be a good time because then we could still include the issue in the #[unstable(..)] attribute. And perhaps also create a page in the Unstable Book? Or is that supposed to be in a seperate PR?

This section comes from the previous PR, #75911:

There was previously some discussion on IRLO.

Motivation

The functionality provided by this new “method” on Arc was previously not archievable with the Arc API. The function into_inner is related to (and hence similarly named similar to) try_unwrap. The expression Arc::into_inner(x) is almost the same as Arc::try_unwrap(x).ok(), however the latter includes two steps, the try_unwrap call and dropping the Arc, whereas into_inner accesses the Arc atomically. Since this is an issue in multi-threaded settings only, a similar function on Rc is not strictly necessary but could be wanted nontheless for ergonomic and API-similarity reasons. (This PR currently only offers the function on Arc, but I could add one for Rc if wanted.) In the IRLO discussion, I also mentioned two more functions that could possibly extend this API.

The function Arc::into_inner(this: Arc<T>) -> Option<T> offers a way to “drop” an Arc without calling the destructor on the contained type. When the Arc provided was the last strong pointer to its target, the target value is returned. Being able to do this is valueable around linear(-ish) types that should not or cannot just be dropped ordinarity, but require extra arguments, or operations that can fail or are async to properly get rid of.

Rendered Documentation

Also add documentation and tests for it. This commit has some minor unresolved questions and is intended to be amended.

…'s a lot squash me later

…rap_or_drop`

…m` path.

rust-highfive · 2020-12-03T16:00:14Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

danielhenrymantilla

I like how this is looking! 👌

library/alloc/src/sync.rs

danielhenrymantilla · 2020-12-03T16:28:48Z

library/alloc/src/sync.rs

+    /// ```
+    ///
+    /// A more practical example demonstrating the need for `Arc::into_inner`:
+    /// ```


Suggested change

/// ```

/// ```no_run

In case a 100_000 iteration loop within a (doc-)test is not deemed desirable.

At least on my machine this doc-test does not run particularly slowly. There seem to be lots of other doc tests that take much longer than this one. The 100_000 iterations are necessary to ensure stack overflows are even possible on enough platforms.

danielhenrymantilla · 2020-12-03T16:31:20Z

library/alloc/src/sync.rs

+    /// It is strongly recommended to use [`Arc::into_inner`] instead if you don't
+    /// want to keep the `Arc` in the [`Err`] case.
+    /// Immediately dropping the [`Err`] payload, like in the expression
+    /// `Arc::try_unwrap(this).ok()`, can still cause the strong count to


Talking for the future here, but if/when this feature gets stabilized, it will be nice to have a (clippy?) lint for this 🙂

I don't know if this has been done before, but adding a FIXME comment about opening an issue for it on clippy would ensure this is not forgotten when this becomes stable.

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

poliorcetics · 2020-12-04T00:13:12Z

It looks very very nice, thanks for sticking with this !

If you want to work on Rc::into_inner too, I suggest adding a checklist in the original comment and marking the PR as WIP: or draft so that a reviewer won't approve it before it's ready. 😄

coolreader18 · 2020-12-04T01:43:54Z

Would it make more sense for the signature to be fn into_inner(Self) -> Result<T, Self>, in order to be able to still use the arc if it's not at refcount==1?

steffahn · 2020-12-04T01:58:09Z

Would it make more sense for the signature to be fn into_inner(Self) -> Result<T, Self>, in order to be able to still use the arc if it's not at refcount==1?

@coolreader18 no, that’s Arc::try_unwrap. The main point of this PR is that Arc::try_unwrap can be problematic in case you don’t want to be able to still use the arc. Hence the new, additional function. I assume you did read the image with the documentation?

I could take this as a hint that the docs might need improvement. On the other hand, in practice this could not be that confusing since the docs of try_unwrap are going to be right above into_inner:

coolreader18 · 2020-12-04T02:01:49Z

Ah, whooooopssss, sorry... I was thinking for some reason that nothing like this existed already, but I think I was thinking of a Arc<T> -> Box<T> conversion function (which wouldn't make too much sense anyway).

JohnCSimon · 2021-02-07T23:49:34Z

Ping from triage - @steffahn
apparently this still needs more work
@rustbot label: -S-waiting-on-review +S-waiting-on-author

JohnCSimon · 2021-03-01T02:16:50Z

@steffahn - Thanks for your contribution but I'm closing this is inactive. If you want to continue feel free to reopen.

kvark · 2022-08-12T16:30:16Z

Very unfortunate to see this being closed. What are the steps missing, exactly? Could someone involved in a discussion make a list of things that need to be addressed for this to proceed? I'm sure somebody (if not me) will pick this up. This functionality is very much needed and missing currently. cc @steffahn

steffahn · 2022-08-12T16:48:23Z

@kvark Thanks for commenting. Reviving this PR was still somewhere deep on my (over-procrastinated) mental TODO list of things I’ll eventually need to get around to. AFAIR, there’s not too much effort necessary to bring this to completion, I’ll have to review the review comments so far, but I don’t remember there being any major problems. Also, this PR is merely about introducing the API behind a feature gate, not stabilizing it.

Thanks for confirming that this functionality is “much needed”; for me personally it was only a technically missing portion of API, but I didn’t have practical need for it personally.

I have also gained more contribution experience in the meantime anyways… let’s see, perhaps I can get this PR back on track fairly quickly… e.g. this weekend, or next week.

kvark · 2022-08-12T17:35:42Z

I see, thanks for the info! I suppose I can't rely on this right now even if it's merged, given than it's behind a feature gate, so I'll search for a workaround.
To expand on the practical use-case, I'm working on a task scheduler - https://github.com/kvark/choir
The neatest part of it is the implementation detail that the current state of a task is expressed in the type system:

if it's Arc<Task> then there are still dependencies not finished
once it's Task (extracted currently with Arc::try_unwrap), it can run

I find this very clean and elegant for not relying on any unsafe functionality. But it will only work if I can do "unwrap or drop" atomically, which is what your PR (fortunately) already does. If you have ideas on the minimal workaround, please share them somewhere (matrix, email, etc, not sure if this PR is really appropriate).

steffahn · 2022-08-12T18:47:51Z

It it gets merged, stabilization can be the next step and needs not take extraordinarily long though (of course if you need a solution right now or quickly, that’s still not enough). Regarding workarounds, you could

use a custom Arc implementation (e.g. by copying the necessary parts from the standard library) that has this functionality added
use some custom, less general, solution using atomics (I haven’t though about the details)
use a destructor that returns a value somehow, e.g. via a thread-local. That’s some overhead, but possibly it’s low enough (after all, e.g. with Futures and executors, there’s a lot of implicit context being passed in thread-locals, too). Here’s an example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8acc3450726f68a75b3efc04477630c1

kvark · 2022-08-14T05:54:17Z

I went the route of implementing my own linear Arc and bumped into a (blocker) problem that constructing Arc<T>::new() for unsized T requires the coercion magic in the compiler. I.e. this can't be a matter of just copying parts of the standard library implementation into userspace, unfortunately.
I admit this is a bit of a different issue, but the point is - Arc in std should either have less magic, or be useful enough that people wouldn't need to re-implement it.

CAD97 · 2022-08-14T06:11:00Z

Arc isn't magic. The coercion Arc has is automatically afforded to any struct S<T: ?Sized>(ptr::NonNull<SInner<T>>);. triomphe::Arc is an example out-of-std Arc.

kvark · 2022-08-14T06:33:09Z

@CAD97 this crate relies on a re-implementation of the coercion mechanism - https://crates.io/crates/unsize
So, yes, possible in principle, but unfortunately deeper than just Arc itself.

kvark · 2022-08-14T06:36:16Z

@steffahn one thing that I believe this PR is missing is a function similar to into_inner but for unsized types. It could return a bool (since it can't return the Option<T>).

CAD97 · 2022-08-14T06:37:10Z

Triomphe only uses the unsize crate for thin arcs, IIRC. But yes, I'm very aware of the exact capabilities and limitations of the current unsizing mechanism.

steffahn · 2023-01-14T13:26:04Z

The code still rebases without problems.

As far as I can tell there aren't actually any blockers to merging this as-is; if I don't implement it now, adding Rc::into_inner can happen later. (The implementation is trivial anyways, it's mostly about finding the right documentation.)

I opened #106854, let's get some progress here, finally.

…structor on the inner type. Mainly rebased and squashed from PR rust-lang#79665, furthermore includes minor changes in comments.

…r=Mark-Simulacrum Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type. ACP: rust-lang/libs-team#162 Reviving rust-lang#79665. I want to get this merged this time; this does not contain changes (apart from very minor changes in comments/docs). See rust-lang#79665 for further description of the PR. The only “unresolved” points that led to that PR being closed, AFAICT, were * The desire to also implement a `Rc::into_inner` function * however, this can very well also happen as a subsequent PR * Possible need for further discussion on the naming “`into_inner`” (?) * `into_inner` seems fine to me; also, this PR introduces unstable API, and names can be changed later, too * ~~I don't know if a tracking issue for the feature flag is supposed to be opened before or after this PR gets merged (if *before*, then I can add the issue number to the `#[unstable…]` attribute)~~ There is a [tracking issue](rust-lang#106894) now. I say “unresolved” in quotation marks because from my point of view, if reviewers agree, the PR can be merged immediately and as-is :-)

…structor on the inner type. Mainly rebased and squashed from PR rust-lang/rust#79665, furthermore includes minor changes in comments.

steffahn added 14 commits December 3, 2020 15:09

Implement Arc::unwrap_or_drop.

6a2e8e7

Also add documentation and tests for it. This commit has some minor unresolved questions and is intended to be amended.

more comments and test, maybe won't want to keep all of them since it…

657b534

…'s a lot squash me later

fix typo, remove superflous test

a0740b8

split examples

fbedcd5

fix oversight in test where try_unwrap was not changed back to `unw…

a7bdb90

…rap_or_drop`

Add SAFETY comment.

5d81f76

Comments of unwrap_or_drop test.

ca7066d

Remove FIXME comment in unwrap_or_drop.

948e402

Capitalize comments.

7d43bcb

Rename unwrap_or_drop to into_inner.

1e28132

Fully qualified associated functions in documentation.

81623f4

Apply fmt.

f57de56

Flatten block in into_inner example.

cd444ca

rust-highfive assigned joshtriplett Dec 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020

steffahn mentioned this pull request Dec 3, 2020

Add Arc::unwrap_or_drop for safely discarding Arcs without calling the destructor on the inner type. #75911

Closed

jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 3, 2020

This comment has been minimized.

Sign in to view

rustbot added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Dec 3, 2020

danielhenrymantilla reviewed Dec 3, 2020

View reviewed changes

into_inner example: prefer "imperative" if let over .map()

ecb85f0

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2021

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2021

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 1, 2021

JohnCSimon closed this Mar 1, 2021

kvark mentioned this pull request Aug 12, 2022

Task dependencies are not fully sound kvark/choir#21

Closed

steffahn mentioned this pull request Jan 14, 2023

Add Arc::into_inner for safely discarding Arcs without calling the destructor on the inner type. #106854

Merged

steffahn mentioned this pull request Jan 14, 2023

Add Arc::into_inner for safely discarding Arcs without calling the destructor on the inner type. rust-lang/libs-team#162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type. #79665

Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type. #79665

steffahn commented Dec 3, 2020 •

edited

Loading

rust-highfive commented Dec 3, 2020

This comment has been minimized.

danielhenrymantilla left a comment

danielhenrymantilla Dec 3, 2020

steffahn Dec 3, 2020

danielhenrymantilla Dec 3, 2020

poliorcetics Dec 4, 2020

poliorcetics commented Dec 4, 2020

coolreader18 commented Dec 4, 2020

steffahn commented Dec 4, 2020

coolreader18 commented Dec 4, 2020 •

edited

Loading

JohnCSimon commented Feb 7, 2021

JohnCSimon commented Mar 1, 2021 •

edited

Loading

kvark commented Aug 12, 2022

steffahn commented Aug 12, 2022

kvark commented Aug 12, 2022

steffahn commented Aug 12, 2022 •

edited

Loading

kvark commented Aug 14, 2022

CAD97 commented Aug 14, 2022

kvark commented Aug 14, 2022

kvark commented Aug 14, 2022

CAD97 commented Aug 14, 2022

steffahn commented Jan 14, 2023

Add Arc::into_inner for safely discarding Arcs without calling the destructor on the inner type. #79665

Add Arc::into_inner for safely discarding Arcs without calling the destructor on the inner type. #79665

Conversation

steffahn commented Dec 3, 2020 • edited Loading

This section comes from the previous PR, #75911:

Motivation

Rendered Documentation

rust-highfive commented Dec 3, 2020

This comment has been minimized.

danielhenrymantilla left a comment

Choose a reason for hiding this comment

danielhenrymantilla Dec 3, 2020

Choose a reason for hiding this comment

steffahn Dec 3, 2020

Choose a reason for hiding this comment

danielhenrymantilla Dec 3, 2020

Choose a reason for hiding this comment

poliorcetics Dec 4, 2020

Choose a reason for hiding this comment

poliorcetics commented Dec 4, 2020

coolreader18 commented Dec 4, 2020

steffahn commented Dec 4, 2020

coolreader18 commented Dec 4, 2020 • edited Loading

JohnCSimon commented Feb 7, 2021

JohnCSimon commented Mar 1, 2021 • edited Loading

kvark commented Aug 12, 2022

steffahn commented Aug 12, 2022

kvark commented Aug 12, 2022

steffahn commented Aug 12, 2022 • edited Loading

kvark commented Aug 14, 2022

CAD97 commented Aug 14, 2022

kvark commented Aug 14, 2022

kvark commented Aug 14, 2022

CAD97 commented Aug 14, 2022

steffahn commented Jan 14, 2023

Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type. #79665

Add `Arc::into_inner` for safely discarding `Arc`s without calling the destructor on the inner type. #79665

steffahn commented Dec 3, 2020 •

edited

Loading

coolreader18 commented Dec 4, 2020 •

edited

Loading

JohnCSimon commented Mar 1, 2021 •

edited

Loading

steffahn commented Aug 12, 2022 •

edited

Loading