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, Rc}::into_inner #106894

Closed
6 tasks done
steffahn opened this issue Jan 15, 2023 · 12 comments · Fixed by #109504
Closed
6 tasks done

Tracking Issue for {Arc, Rc}::into_inner #106894

steffahn opened this issue Jan 15, 2023 · 12 comments · Fixed by #109504
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Jan 15, 2023

Feature gates: #![feature(arc_into_inner)], #![feature(rc_into_inner)]

This is a tracking issue for the Arc::into_inner method, and (for API-consistency) also an Rc::into_inner analogue.

The function Arc::into_inner(x) serves as an atomic race-condition-free alternative to Arc::try_unwrap(x).ok(). See the ACP for more details.

Public API

// alloc::sync

impl<T> Arc<T> {
    /// Returns the inner value, if the `Arc` has exactly one strong reference.
    /// Otherwise, `None` is returned and the `Arc` is dropped.
    pub fn into_inner(this: Arc<T>) -> Option<T>;
}

and the extension to Rc

// alloc::rc

impl<T> Rc<T> {
    /// Returns the inner value, if the `Rc` has exactly one strong reference.
    /// Otherwise, `None` is returned and the `Rc` is dropped.
    pub fn into_inner(this: Rc<T>) -> Option<T>;
}

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@steffahn steffahn added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 15, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jan 23, 2023
…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 :-)
@ryoqun
Copy link
Contributor

ryoqun commented Mar 1, 2023

Hey, I've stumbled on this. I wonder how soon this can be stabilized? My usecase is complex. but in a nut shell, I want this to conditionally and cleanly restart bunch of interconnected threads of mixed roles, all sharing Arc<GlobalContext>, depending on which thread did actually dropped the GlobalContext.

if sharing helps, i can write the usecase in detail...

Of course, I can resort to sync::Weak or tls. but it incurs overhead and/or further complexities. or mem::transmute like this to forcibly bring ::into_inner into stable rustc: https://rust.godbolt.org/z/r5rbsYYcT, in which i have to use unsafe... Also, due to already-existing wide-spread use of Arc<GlobalContext> in our codebase, switching to different Arc impl altogether isn't viable as well. That precondition also precludes more saner cleaning code arrangement in the first place. ;)

@joshtriplett
Copy link
Member

Submitted the Rc::into_inner implementation as #109026

@joshtriplett
Copy link
Member

I think Arc::into_inner makes sense to stabilize at this point. And Rc::into_inner is trivial.

@rust-lang/libs-api, shall we stabilize these?

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 11, 2023

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

No concerns currently listed.

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 11, 2023
@rfcbot
Copy link

rfcbot commented Mar 12, 2023

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 12, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 22, 2023
@rfcbot
Copy link

rfcbot commented Mar 22, 2023

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.

This will be merged soon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 23, 2023
…, r=joshtriplett

Stabilize `arc_into_inner` and `rc_into_inner`.

Stabilize the `arc_into_inner` and `rc_into_inner` library features and thus close rust-lang#106894.

The changes in this PR also resolve the FIXMEs for adjusting the documentation upon stabilization, and I’ve additionally included some very minor documentation improvements.

`@rustbot` label +T-libs-api -T-libs
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 23, 2023
…, r=joshtriplett

Stabilize `arc_into_inner` and `rc_into_inner`.

Stabilize the `arc_into_inner` and `rc_into_inner` library features and thus close rust-lang#106894.

The changes in this PR also resolve the FIXMEs for adjusting the documentation upon stabilization, and I’ve additionally included some very minor documentation improvements.

``@rustbot`` label +T-libs-api -T-libs
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 23, 2023
@bors bors closed this as completed in e445648 Mar 23, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
…mulacrum

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

ACP: rust-lang/libs-team#162

Reviving #79665.

I want to get this merged this time; this does not contain changes (apart from very minor changes in comments/docs).

See #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/rust#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 :-)
@thynson

This comment was marked as resolved.

@steffahn
Copy link
Member Author

steffahn commented Jan 23, 2024

@thynson Feel free to open an issue if you have concrete ideas how the documentation of Arc::into_inner can be improved to convince users to actually look at the Arc::try_unwrap function. Of course, it's already mentioned there multiple times (it's also mentioned on this tracking issue) but apparently not in a way that convinced you to actually look at that method.

Maybe also a doc aliastry_into_inner” for try_unwrap to further help discoverability 🤔 (though I'm not sure off the top of my head, how well those work for associated items; and also that would assume people use the search bar, anyways)

@thynson
Copy link

thynson commented Jan 23, 2024

Umm, it's my bad that Arc::try_unwrap already mentioned in the doc of Arc::into_inner, which is exactly what I'm looking for, through the doc make me feel it's discouraged to be used

    /// ...
    ///
    /// The similar expression `Arc::try_unwrap(this).ok()` does not
    /// offer such a guarantee. See the last example below
    /// and the documentation of [`Arc::try_unwrap`].
    ///
    /// ...

Actually they're designed for different scenarios. Probably better to document it like:

    /// ...
    ///
    /// `Arc::try_unwrap` offers similar functionality but it's designed 
    /// for difference scenarios, while this function is suitable for dropping
    /// a clone of an `Arc`, obtaining the wrapped `T` when the last clone of an
    ///  `Arc` was dropped.
    ///
    /// ...

@steffahn
Copy link
Member Author

steffahn commented Jan 23, 2024

Hmm, no try_unwrap is only meant to be discouraged in the case where you ignore the Arc<T> you get back in the Err case. That’s clearly documented, too… but it’s describing the details of when and how to use try_unwrap, and hence not documented in into_inner’s documentation.

On the other hand, I wasn’t aware that someone could try to combine into_inner with a Arc::strong_count(…) == 1 check.

Anyways… maybe a sentence that introduces Arc::try_unwrap before immediately calling out an expression using it could be sufficiently useful, after all.

If Arc::into_inner is called on every clone of this Arc, it is guaranteed that exactly one of the calls returns the inner value. This means in particular that the inner value is not dropped.

Arc::try_unwrap offers similar functionality to this function, and might seem simply like a more general/powerful API than Arc::into_inner; but it has in fact different use-cases. It you did use it to replace Arc::into_inner, such as with the expression Arc::try_unwrap(this).ok(), then you would not get the guarantee described in the previous paragraph. For more information, see the last example below and read the documentation of Arc::try_unwrap.

^^^ Maybe like this?

@thynson
Copy link

thynson commented Jan 23, 2024

It you did use it to replace Arc::into_inner, ...

If I understand it right, it looks like a typos of

If you did use it to replace Arc::into_inner, ...

The modified version should be clear enough for me.
And thanks for your quick response. 🥇

@steffahn
Copy link
Member Author

@thynson I’ve changed the wording a bit more, feel free to take a look #120266

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 27, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 27, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 27, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 28, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 28, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
fmease added a commit to fmease/rust that referenced this issue Jan 29, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jan 29, 2024
…ark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 29, 2024
Rollup merge of rust-lang#120266 - steffahn:a_rc_into_inner_docs, r=Mark-Simulacrum

Improve documentation for [A]Rc::into_inner

General improvements, and also aims to better encourage the reader to actually check out Arc::try_unwrap.

This addresses concerns from rust-lang#106894 (comment).

Rendered:

![Screenshot_20240123_114436](https://github.com/rust-lang/rust/assets/3986214/68896d62-13e0-4f3a-8073-91d8e77c5554)
![Screenshot_20240123_114455](https://github.com/rust-lang/rust/assets/3986214/dc58e4bd-dd7f-40b1-bc50-fd6200dde593)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.

6 participants