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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,17 @@ impl<T> Arc<T> {
///
/// This will succeed even if there are outstanding weak references.
///
/// It is strongly recommended to use [`Arc::unwrap_or_drop`] 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
/// drop to zero and the inner value of the `Arc` to be dropped:
/// For instance if two threads execute this expression in parallel, then
/// there is a race condition. The threads could first both check whether they
/// have the last clone of their `Arc` via `try_unwrap`, and then
/// both drop their `Arc` in the call to [`ok`][`Result::ok`],
/// taking the strong count from two down to zero.
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -500,6 +511,123 @@ impl<T> Arc<T> {
Ok(elem)
}
}

/// Returns the inner value, if the `Arc` has exactly one strong reference.
///
/// Otherwise, [`None`] is returned and the `Arc` is dropped.
///
/// This will succeed even if there are outstanding weak references.
///
/// If `unwrap_or_drop` 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.
///
/// The similar expression `Arc::try_unwrap(this).ok()` does not
/// offer such a guarantee. See the last example below and the documentation
/// of [`try_unwrap`][`Arc::try_unwrap`].
///
/// # Examples
///
/// Minimal example demonstrating the guarantee that `unwrap_or_drop` gives.
/// ```
/// #![feature(unwrap_or_drop)]
///
/// use std::sync::Arc;
///
/// let x = Arc::new(3);
/// let y = Arc::clone(&x);
///
/// // Two threads calling `unwrap_or_drop` on both clones of an `Arc`:
/// let x_unwrap_thread = std::thread::spawn(|| Arc::unwrap_or_drop(x));
/// let y_unwrap_thread = std::thread::spawn(|| Arc::unwrap_or_drop(y));
///
/// let x_unwrapped_value = x_unwrap_thread.join().unwrap();
/// let y_unwrapped_value = y_unwrap_thread.join().unwrap();
///
/// // One of the threads is guaranteed to receive the inner value:
/// assert!(matches!(
/// (x_unwrapped_value, y_unwrapped_value),
/// (None, Some(3)) | (Some(3), None)
/// ));
/// // The result could also be `(None, None)` if the threads called
/// // `Arc::try_unwrap(x).ok()` and `Arc::try_unwrap(y).ok()` instead.
/// ```
///
/// A more practical example demonstrating the need for `unwrap_or_drop`:
/// ```
/// #![feature(unwrap_or_drop)]
///
steffahn marked this conversation as resolved.
Show resolved Hide resolved
/// use std::sync::Arc;
///
/// // Definition of a simple singly linked list using `Arc`:
/// #[derive(Clone)]
/// struct LinkedList<T>(Option<Arc<Node<T>>>);
/// struct Node<T>(T, Option<Arc<Node<T>>>);
///
/// // Dropping a long `LinkedList<T>` relying on the destructor of `Arc`
/// // can cause a stack overflow. To prevent this, we can provide a
/// // manual `Drop` implementation that does the destruction in a loop:
/// impl<T> Drop for LinkedList<T> {
/// fn drop(&mut self) {
/// let mut x = self.0.take();
/// while let Some(arc) = x.take() {
/// Arc::unwrap_or_drop(arc).map(|node| x = node.1);
/// }
/// }
/// }
///
/// // implementation of `new` and `push` omitted
/// impl<T> LinkedList<T> {
/// /* ... */
/// # fn new() -> Self {
/// # LinkedList(None)
/// # }
/// # fn push(&mut self, x: T) {
/// # self.0 = Some(Arc::new(Node(x, self.0.take())));
/// # }
/// }
///
/// // 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)`.
/// {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

/// // create a long list and clone it
/// let mut x = LinkedList::new();
/// for i in 0..100000 {
/// x.push(i); // adds i to the front of x
/// }
/// let y = x.clone();
///
/// // drop the clones in parallel
/// let t1 = std::thread::spawn(|| drop(x));
/// let t2 = std::thread::spawn(|| drop(y));
/// t1.join().unwrap();
/// t2.join().unwrap();
/// }
/// ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ^_^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#[inline]
#[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`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut this = core::mem::ManuallyDrop::new(this);

if this.inner().strong.fetch_sub(1, Release) != 1 {
return None;
}

acquire!(this.inner().strong);

// FIXME: should the part below this be moved into a seperate #[inline(never)]
// function, like it's done with drop_slow in drop?
Comment on lines +621 to +622
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// using `ptr::read` where `drop_slow` was using `ptr::drop_in_place`
let inner = unsafe { ptr::read(Self::get_mut_unchecked(&mut this)) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@steffahn steffahn Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?”

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


drop(Weak { ptr: this.ptr });

Some(inner)
}
}

impl<T> Arc<[T]> {
Expand Down
39 changes: 39 additions & 0 deletions library/alloc/src/sync/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,45 @@ fn try_unwrap() {
assert_eq!(Arc::try_unwrap(x), Ok(5));
}

#[test]
fn unwrap_or_drop() {
// FIXME: Is doing this kind of loop reasonable? I tested `Arc::try_unwrap(x).ok()`
// and it makes this kind of assertion fail in roughly every second run somewhere
// between 1000 and 5000 iterations; I feel like doing a single iteration is too
// unlikely to catch anything interesting but doing too many is way too slow
// for a test that wouldn't ever fail for any reasonable implementation

for _ in 0..100
// ^ increase chances of hitting uncommon race conditions
{
use std::sync::Arc;
let x = Arc::new(3);
let y = Arc::clone(&x);
let r_thread = std::thread::spawn(|| Arc::try_unwrap(x).ok());
let s_thread = std::thread::spawn(|| Arc::try_unwrap(y).ok());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let r = r_thread.join().expect("r_thread panicked");
let s = s_thread.join().expect("s_thread panicked");
assert!(
matches!((r, s), (None, Some(3)) | (Some(3), None)),
"assertion failed: unexpected result `{:?}`\
\n expected `(None, Some(3))` or `(Some(3), None)`",
(r, s),
);
}

let x = Arc::new(3);
assert_eq!(Arc::unwrap_or_drop(x), Some(3));

let x = Arc::new(4);
let y = Arc::clone(&x);
assert_eq!(Arc::unwrap_or_drop(x), None);
assert_eq!(Arc::unwrap_or_drop(y), Some(4));

let x = Arc::new(5);
let _w = Arc::downgrade(&x);
assert_eq!(Arc::unwrap_or_drop(x), Some(5));
}

#[test]
fn into_from_raw() {
let x = Arc::new(box "hello");
Expand Down