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

steffahn · 2020-08-25T16:12:10Z

This is my first contribution. The commit includes tests and documentation. I have marked a few stylistic or technical questions with FIXME. In particular, I don’t know how the tracking issues for new unstable standard library functions work.

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 unwrap_or_drop is related to (and hence similarly named similar to) try_unwrap. The expression Arc::unwrap_or_drop(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 unwrap_or_drop 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::unwrap_or_drop(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.

Further Remarks

The current documentation is adapted from and compares this function to try_unwrap so there’s no mention of the motivation (dropping Arc without calling a destructor). I don’t know if this should be added.

The names try_unwrap and unwrap_or_drop are a bit unfortunate since these operations seem quite different from the unwrap methods on Option or Result. This functionality could be renamed around into_inner, for example as try_into_inner (instead of try_unwrap, which would be deprecated) and into_inner (instead of unwrap_or_drop). Some people favored this kind of naming scheme in the IRLO discussion. On the other hand, into_inner is usually more straightforward and deterministic than what unwrap_or_drop offers.

Rendered Documentation

poliorcetics

Thanks for the contribution ! I'm not an official Rust team member but I still left some style comments and small nits. :)

library/alloc/src/sync.rs

library/alloc/src/sync/tests.rs

poliorcetics · 2020-08-26T16:14:46Z

@pietroalbini highfive didn't appear, is it down ? (I think you are one of the person in charge of it, sorry if I'm wrong)

pietroalbini · 2020-08-26T16:30:50Z

Highfive failed to assign someone here. I'll add more logging so we'll discover what's causing this in the future.

When something like this happens again, please ping the whole infra team. Thanks :)

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

poliorcetics · 2020-08-26T20:52:24Z

library/alloc/src/sync.rs

+        // function, like it's done with drop_slow in drop?
+
+        // using `ptr::read` where `drop_slow` was using `ptr::drop_in_place`
+        let inner = unsafe { ptr::read(Self::get_mut_unchecked(&mut this)) };


I forgot: Please write a // SAFETY: comment for why this is safe here. It doesn't have to be very long, just explains why the invariants are checked. :) Here is an example from core.

The situation is a bit more complex. It is safe because it is doing the same things that the drop implementation does, with the difference of doing ptr::read instead of ptr::drop_in_place, but those two operations are applicable in pretty much exactly the same situations. The reason why every step of drop is safe also includes some really really really long comments in drop about ordering of atomic operations, hence my general question, as stated in the FIXME in line 538: “should I copy [...] the comments from drop?”

What I've done before is say something along the lines of "this is safe for the same reason the drop impl is safe; see that for more info".

Sorry I didn't answer earlier, I didn't get the notification for this comment.

The comments in the drop implementation should not change very much, except for improving them so referencing them is probably okay.

You can add a line about the differences between ptr::read and ptr::drop_in_place and why the former must be used instead of the later below and it should be good.

jyn514 · 2020-08-26T21:07:31Z

r? @LukasKalbertodt

steffahn · 2020-08-29T09:29:45Z

I’ve found another use-case while reading: this book about linked lists in rust.

The author presents code like this

impl<T> Drop for List<T> {
    fn drop(&mut self) {
        let mut head = self.head.take();
        while let Some(node) = head {
            if let Ok(mut node) = Rc::try_unwrap(node) {
                head = node.next.take();
            } else {
                break;
            }
        }
    }
}

to avoid stack overflows on drop. This code does use try_unwrap and drop the Rc in the Err case. On the next page they state “In order to get thread safety, we have to use Arc. [...] All we need to do [...] is replace every reference to Rc with std::sync::Arc. That's it. We're thread safe. Done!”

At least they acknowledge that in Rust, “[...] it's impossible to cause data races, (not to be mistaken with the more general issue of race conditions).” They do not seem to notice that by replacing Rc with Arc, they are introducing a race condition themselves that can cause a stack overflow.

This example can of course be solved by unwrap_or_drop. Since it comes from starting out with Rc and only migrating the code to Arc afterwards, it convinces me that Rc should get a version of unwrap_or_drop, too.

jyn514 · 2020-08-29T12:36:09Z

The current documentation is adapted from and compares this function to try_unwrap so there’s no mention of the motivation (dropping Arc without calling a destructor). I don’t know if this should be added.

This would be nice to have; just by reading the type signature it wasn't clear why you'd want this over arc.try_unwrap().ok().

steffahn · 2020-08-29T19:14:49Z

The current documentation is adapted from and compares this function to try_unwrap so there’s no mention of the motivation (dropping Arc without calling a destructor). I don’t know if this should be added.

This would be nice to have; just by reading the type signature it wasn't clear why you'd want this over arc.try_unwrap().ok().

I’ve been working on the documentation a bit. I also added a paragraph to try_unwrap and a second example to unwrap_or_drop. This might be overkill though:

Edit: Updated screenshot

I mean, in case you like it, I can commit and push it so that people can correct any typos, etc.

poliorcetics · 2020-08-29T19:31:19Z

Don't hesitate to push, at worst you'll be asked to squash before the PR is accepted. :)

Just avoid using force pushing when possible while reviews are going on, it makes following changes harder

…'s a lot squash me later

library/alloc/src/sync.rs

library/alloc/src/sync/tests.rs

danielhenrymantilla · 2020-09-02T15:22:45Z

Concern: naming

After reading this thread a bunch of days later, remembering a vague idea of the topic at hand, I've realised that or_drop is actually misleading. The whole point of doing this "atomically" is to ensure that the or case will just decrease ref_count / release a non-owning handle, so that the pointee will not be dropped in that case. Calling that branch drop seems misleading.

I thus think that the name ought to be changed (other than that, it does seem to me like a good API for this very specific but not far-fetched use case 👍).

My personal suggestions regarding the naming:

.unwrap_or_relinquish_ownership(). Quite mouthful, but for such specific use cases it seems to be worth it.
Shorter version: .unwrap_or_release(), but Rust does not have the habit of using the retain/ release terminology.
.unwrap_or_decr_strong_count(), based on .decr_strong_count(). This goes against abstraction, by exposing a very low-level detail of the implementation, but, again, given how specific the use case is, among all these three options, this one looks like the best to me.

CAD97 · 2020-09-02T15:31:38Z

Note that this is always called as Arc::unwrap_or_xxx(handle), so it should be possible to infer from that, that Arc::xxx_or_drop means drop the Arc, not the T.

That said, I agree that there is an opening for misinterpretation (since, as you point out, the point of this is that the pointee will not be dropped), but I disagree that any of your proposed options are strictly better (except maybe unwrap_or_release, but I agree that doesn't fit with existing naming).

poliorcetics · 2020-09-02T20:49:51Z

The method is defined as taking this: Self so it will always be called as Arc::unwrap_or_drop(my_value). IMO this is enough to show what's going on. The fact it feats the current naming style is a nice bonus.

Now that I think about it: @steffahn, what do you think about modifying the doc for Arc::try_unwrap to give a pointer to Arc::unwrap_or_drop about the Arc::try_unwrap(x).ok() case ? Or maybe only adding a FIXME in the new method for later, when it is eventually stabilised ?

steffahn · 2020-09-02T21:05:00Z

I already modified the docs of try_unwrap in one of my newer commits.

poliorcetics · 2020-09-02T21:06:01Z

I already modified the docs of try_unwrap in one of my newer commits.

Oh sorry, I checked quickly but didn't see it !

steffahn · 2020-09-02T21:10:04Z

@poliorcetics also see the updated screenshot in my comment further up for a rendered version of that documentation

poliorcetics · 2020-09-02T21:18:52Z

library/alloc/src/sync.rs

+    /// // The following code could still cause a stack overflow
+    /// // despite the manual `Drop` impl if that `Drop` impl used
+    /// // `Arc::try_unwrap(arc).ok()` instead of `Arc::unwrap_or_drop(arc)`.
+    /// {


I don't think the extra-indentation and block are necessary here, you should be able to put everything at the same level.

It's not necessary, I know. I felt like it looked better this way, especially making clear that the comment is referencing the entire following code. I also wrote this when the two examples where not split yet. I have not reevaluated if it looks a bit less confusing to have this block on top level now that the examples are split.

You can put a vertical space between the overarching comment and the rest:

/// // The following code could still cause a stack overflow /// // despite the manual `Drop` impl if that `Drop` impl used /// // `Arc::try_unwrap(arc).ok()` instead of `Arc::unwrap_or_drop(arc)`. /// other comments / code

The example end with this block so there should be no confusion about it. (I hope)

crlf0710 · 2020-09-18T11:42:53Z

Triage: I guess this is still waiting on review. Somehow it doesn't has a S-* tag.

danielhenrymantilla · 2020-09-18T12:57:15Z

⚠️ Note that I am still concerned about the naming.

I've found a way to better phrase the problem. We have:

Arc: unwrap [the payload] or drop [the handle]

As you can see, there is a double elision of the object of the action, and contrary to what natural languages do, at least, w.r.t. English (and French and Spanish), when multiple elisions happen it's only because they all refer to the same elided entity.

This is factually not the case here, so I insist that such a niche use case should favor being less terse and thus potentially confusing, and on the contrary, lean on the side of explicit-ness. Be it by using unwrap_or_release, or whatever other name somebody else can come up with 🙂

EDIT: the suggested .into_inner(), for instance, LGTM 👍

That is: although some people may not be confused by the current naming (good for them), do all of you honestly think that nobody out there will? What's the harm in having a slightly longer / more explicit method name?

I hope I have, this time, managed to better convey my feeling, which is only a question of being potentially overly cautious, rather than the opposite 😉. And if I haven't, so be it, I won't insist anymore 🙃

LukasKalbertodt

Thanks a lot for the PR. Sorry for my late review.

The reasoning for adding this method makes sense and I'm on board merging it unstably. However, I strongly agree that the name still has to change. As @danielhenrymantilla said here, it's confusing that "unwrap" and "drop" in the name don't refer to the same thing. I think into_inner is a pretty good name. Whether we rename/deprecate try_unwrap is another question.

Regarding the implementation: I checked it briefly and couldn't find any problems. However, this is code using relaxed atomics and I don't feel comfortable approving this kind of addition. I don't have experience with it and generally, it's a hard topic and the standard library was always very conservative with these kinds of changes. However, here, this is mostly just a copy of existing code and it seems to make sense to me. (And yes, I might or might not have spent the last 4 hours trying to understand Arc::drop.) So before merging this, I will probably ping the team to have a few more eyes on this.

Lastly, I left a few inline comments.

LukasKalbertodt · 2020-09-20T10:28:45Z

library/alloc/src/sync.rs

+    ///     t1.join().unwrap();
+    ///     t2.join().unwrap();
+    /// }
+    /// ```


This second long example feels slightly overkill for this kind of highly specialized method. I would assume that people reaching for this method don't need a motivating real world example anymore. However, now that it's here already, you don't need to remove it. Maybe it helps someone after all ^_^

Yeah I know, I called it overkill myself. Nontheless, more examples aren’t going to hurt, probably.

LukasKalbertodt · 2020-09-20T10:36:33Z

library/alloc/src/sync.rs

+    #[unstable(feature = "unwrap_or_drop", issue = "none")] // FIXME: add issue
+    // FIXME: should this copy all/some of the comments from drop and drop_slow?
+    pub fn unwrap_or_drop(this: Self) -> Option<T> {
+        // following the implementation of `drop` (and `drop_slow`)


Comments in std usually start uppercase. This also applies to other comments that I won't individually comment on.

Suggested change

// following the implementation of `drop` (and `drop_slow`)

// Following the implementation of `drop` (and `drop_slow`)

I’ll still have to rework the comments a bit anyways, as suggested above. But thanks for the info anyways.

LukasKalbertodt · 2020-09-20T10:38:59Z

library/alloc/src/sync.rs

+        // FIXME: should the part below this be moved into a seperate #[inline(never)]
+        // function, like it's done with drop_slow in drop?


I don't think this is necessary for now. We can still improve this later, if it seems useful.

I was mostly just curious what the reasoning for the separation in the drop implementation is good for. Possibly improved performance by reducing code size in the common case; but someone ought to have none some benchmarking determining that this is worth it somewhere, right? Perhaps I should look at the git blame and track down the original author and PR to find out...
I was just thinking that since unwrap_or_drop is pretty much also just a destructor for Arc, all the same performance considerations should apply there, too. On the other hand, try_unwrap does not do anything like this.

LukasKalbertodt · 2020-09-20T14:20:04Z

library/alloc/src/sync/tests.rs

+        let r_thread = std::thread::spawn(|| Arc::try_unwrap(x).ok());
+        let s_thread = std::thread::spawn(|| Arc::try_unwrap(y).ok());


Spawning a thread takes a while, so I would almost expect both try_unwraps to never run at the same time. Maybe try spawning both threads but then immediately blocking them to synchronize them. Then after both threads are spawned, in the main thread, you somehow signal to both threads that they may start now. I am not sure how best to do that, though. RwLock or rendezvous channel maybe?

But yeah, in any case, that's a really tricky test to write. Not sure what's best here.

I tested and the test does indeed fail more often than one would think (on my machine) if try_unwrap(x).ok() is used (at least on my machine). Which makes me notice... I totally forgot replacing it with unwrap_or_drop again after testing that these failures can happen!! Damn... this means also means that it didn’t fail once on the CI. But the hope was basically only that if unwrap_or_drop changed in the future and became broken (i.e. lost its guarantees of being atomic) then the test would fail for a lot of people (even if not for everyone) and the error would be noticed eventually.

…rap_or_drop`

steffahn · 2020-09-20T16:13:30Z

Thanks a lot for the PR. Sorry for my late review.

The reasoning for adding this method makes sense and I'm on board merging it unstably. However, I strongly agree that the name still has to change. As @danielhenrymantilla said here, it's confusing that "unwrap" and "drop" in the name don't refer to the same thing. I think into_inner is a pretty good name. Whether we rename/deprecate try_unwrap is another question.

Regarding the implementation: I checked it briefly and couldn't find any problems. However, this is code using relaxed atomics and I don't feel comfortable approving this kind of addition. I don't have experience with it and generally, it's a hard topic and the standard library was always very conservative with these kinds of changes. However, here, this is mostly just a copy of existing code and it seems to make sense to me. (And yes, I might or might not have spent the last 4 hours trying to understand Arc::drop.) So before merging this, I will probably ping the team to have a few more eyes on this.

Lastly, I left a few inline comments.

Thanks for the review so far. I’m thinking into_inner is pretty good myself, too. I’m not sure if renaming try_unwrap is even necessary; I think I was thinking about that these functions must be named similarly for reasons like discoverability and cleanness. But super clean names are hard/almost impossibly anyways and discoverability is good now that I added a link to unwrap_or_drop to try_unwrap’s documentation. So I guess we could just live with try_unwrap + into_inner for now. What I also like about into_inner is that it is a shorter and more familiar name, suggesting that this operation isn’t obscure but instead exactly what you canonically want to do when getting rid of an Arc whilst caring about getting the inner value (if possible). And also there is no useful operation of type Arc<T> -> T (the common type of an into_inner method) that I can think of.

I would be curious to get your opinion on this, too:

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.)

[go to comment]

and

“In order to get thread safety, we have to use Arc. [...] All we need to do [...] is replace every reference to Rc with std::sync::Arc. That's it. We're thread safe. Done!”

[…] They do not seem to notice that by replacing Rc with Arc, they are introducing a race condition themselves that can cause a stack overflow.

This example can of course be solved by unwrap_or_drop. Since it comes from starting out with Rc and only migrating the code to Arc afterwards, it convinces me that Rc should get a version of unwrap_or_drop, too.

[go to comment]

LukasKalbertodt · 2020-10-04T10:10:42Z

Mhhhh... good question regarding Rc::into_inner.

This example can of course be solved by unwrap_or_drop. Since it comes from starting out with Rc and only migrating the code to Arc afterwards, it convinces me that Rc should get a version of unwrap_or_drop, too.

That's a good point, but just adding Rc::into_inner won't solve the problem completely. Maybe the docs should have a section about "transitioning from Rc to Arc". (Not saying it should be included in this PR.) I don't know if there are other cases where just replacing Rc with Arc can lead to some problems.

I'm fine with adding Rc::into_inner in any case. It is a somewhat useful API addition either way. And unlike Arc::into_inner, Rc::into_inner will be trivial to implement (right?) and thus has only a tiny maintenance cost. We can still figure out if we actually want it before stabilization.

Diggsey · 2020-10-14T22:45:25Z

Bikeshed: I like atomic_unwrap() or unwrap_atomic() - from the name alone I had no idea what this did differently from try_unwrap. Upon reading further, it seems the important difference is that it's atomic.

steffahn · 2020-10-14T23:44:19Z

In case anyone is wondering, sorry for not making any progress here at the moment, I'll have time for this again in about a week.

Dylan-DPC-zz · 2020-11-01T15:57:35Z

@steffahn thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks

steffahn · 2020-12-03T16:02:45Z

opened #79665

steffahn force-pushed the drop_linear_arc branch 2 times, most recently from a6f80e6 to 8155a9a Compare August 25, 2020 19:06

poliorcetics reviewed Aug 26, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 26, 2020

Implement Arc::unwrap_or_drop.

a534492

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

steffahn force-pushed the drop_linear_arc branch from 8155a9a to a534492 Compare August 26, 2020 16:58

poliorcetics reviewed Aug 26, 2020

View reviewed changes

rust-highfive assigned LukasKalbertodt Aug 26, 2020

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

8af2a40

…'s a lot squash me later

poliorcetics reviewed Aug 29, 2020

View reviewed changes

library/alloc/src/sync.rs Outdated Show resolved Hide resolved

poliorcetics reviewed Aug 29, 2020

View reviewed changes

library/alloc/src/sync.rs Show resolved Hide resolved

poliorcetics reviewed Aug 29, 2020

View reviewed changes

library/alloc/src/sync/tests.rs Outdated Show resolved Hide resolved

poliorcetics reviewed Aug 29, 2020

View reviewed changes

library/alloc/src/sync/tests.rs Outdated Show resolved Hide resolved

steffahn added 2 commits August 29, 2020 22:34

fix typo, remove superflous test

1ceee61

split examples

838e5ed

poliorcetics reviewed Sep 2, 2020

View reviewed changes

crlf0710 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2020

LukasKalbertodt suggested changes Sep 20, 2020

View reviewed changes

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

08455a6

…rap_or_drop`

steffahn force-pushed the drop_linear_arc branch from c38f4b7 to 08455a6 Compare September 20, 2020 15:20

jyn514 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 Oct 30, 2020

Dylan-DPC-zz closed this Nov 1, 2020

Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2020

steffahn mentioned this pull request Dec 3, 2020

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

Closed

steffahn mentioned this pull request Mar 31, 2021

Fix documentation of conversion from String to OsString #83700

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

steffahn mentioned this pull request May 6, 2023

Race condition in Arc-ified ”Persistent Stack“, and the new Arc::into_inner API rust-unofficial/too-many-lists#271

Open

	// following the implementation of `drop` (and `drop_slow`)
	// Following the implementation of `drop` (and `drop_slow`)

		// FIXME: should the part below this be moved into a seperate #[inline(never)]
		// function, like it's done with drop_slow in drop?

		let r_thread = std::thread::spawn(\|\| Arc::try_unwrap(x).ok());
		let s_thread = std::thread::spawn(\|\| Arc::try_unwrap(y).ok());

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

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

Conversation

steffahn commented Aug 25, 2020 • edited Loading

Motivation

Further Remarks

Rendered Documentation

poliorcetics left a comment

Choose a reason for hiding this comment

poliorcetics commented Aug 26, 2020

This comment has been minimized.

pietroalbini commented Aug 26, 2020

Choose a reason for hiding this comment

steffahn Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Aug 26, 2020

steffahn commented Aug 29, 2020 • edited Loading

jyn514 commented Aug 29, 2020

steffahn commented Aug 29, 2020 • edited Loading

poliorcetics commented Aug 29, 2020

danielhenrymantilla commented Sep 2, 2020 • edited Loading

Concern: naming

CAD97 commented Sep 2, 2020

poliorcetics commented Sep 2, 2020

steffahn commented Sep 2, 2020 via email • edited Loading

poliorcetics commented Sep 2, 2020

steffahn commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crlf0710 commented Sep 18, 2020

danielhenrymantilla commented Sep 18, 2020 • edited Loading

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffahn commented Sep 20, 2020

LukasKalbertodt commented Oct 4, 2020

Diggsey commented Oct 14, 2020

steffahn commented Oct 14, 2020

Dylan-DPC-zz commented Nov 1, 2020

steffahn commented Dec 3, 2020

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

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

steffahn commented Aug 25, 2020 •

edited

Loading

steffahn Aug 26, 2020 •

edited

Loading

steffahn commented Aug 29, 2020 •

edited

Loading

steffahn commented Aug 29, 2020 •

edited

Loading

danielhenrymantilla commented Sep 2, 2020 •

edited

Loading

steffahn commented Sep 2, 2020 via email •

edited

Loading

danielhenrymantilla commented Sep 18, 2020 •

edited

Loading