From a534492145409879e51dad61d7a4a0733cb084f5 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Tue, 25 Aug 2020 17:30:46 +0200 Subject: [PATCH 1/5] Implement `Arc::unwrap_or_drop`. Also add documentation and tests for it. This commit has some minor unresolved questions and is intended to be amended. --- library/alloc/src/sync.rs | 56 +++++++++++++++++++++++++++++++++ library/alloc/src/sync/tests.rs | 39 +++++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 73d2fe74826ed..d4ea362c81a75 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -500,6 +500,62 @@ impl Arc { 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. + /// The similar expression `Arc::try_unwrap(this).ok()` does not + /// offer this guarantee. + /// + /// # Examples + /// + /// ``` + /// #![feature(unwrap_or_drop)] + /// + /// use std::sync::Arc; + /// + /// let x = Arc::new(3); + /// let y = Arc::clone(&x); + /// + /// 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(); + /// + /// assert!(matches!( + /// (x_unwrapped_value, y_unwrapped_value), + /// (None, Some(3)) | (Some(3), None) + /// )); + /// ``` + #[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 { + // following the implementation of `drop` (and `drop_slow`) + 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? + + // using `ptr::read` where `drop_slow` was using `ptr::drop_in_place` + let inner = unsafe { ptr::read(Self::get_mut_unchecked(&mut this)) }; + + drop(Weak { ptr: this.ptr }); + + Some(inner) + } } impl Arc<[T]> { diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index d25171716061d..10b05b2fd0f31 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -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()); + 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"); From 8af2a40048e21c6e33d563de0bd484e4dbaeda8d Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 29 Aug 2020 21:28:51 +0200 Subject: [PATCH 2/5] more comments and test, maybe won't want to keep all of them since it's a lot squash me later --- library/alloc/src/sync.rs | 67 ++++++++++++++++++++++++++++++++- library/alloc/src/sync/tests.rs | 35 +++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index d4ea362c81a75..aab721242b28e 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -469,6 +469,17 @@ impl Arc { /// /// 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 /// /// ``` @@ -509,8 +520,11 @@ impl Arc { /// /// 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 this guarantee. + /// offer such a guarantee. See the last example below and the documentation + /// of `try_unwrap`[`Arc::try_unwrap`]. /// /// # Examples /// @@ -522,16 +536,67 @@ impl 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) /// )); + /// + /// + /// + /// // For a somewhat more practical example, + /// // we define a singly linked list using `Arc`: + /// #[derive(Clone)] + /// struct LinkedList(Option>>); + /// struct Node(T, Option>>); + /// + /// // Dropping a long `LinkedList` 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 Drop for LinkedList { + /// 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 LinkedList { + /// /* ... */ + /// # 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)`. + /// { + /// // 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(); + /// } /// ``` #[inline] #[unstable(feature = "unwrap_or_drop", issue = "none")] // FIXME: add issue diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index 10b05b2fd0f31..2a71b82ffd022 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -140,6 +140,41 @@ fn unwrap_or_drop() { assert_eq!(Arc::unwrap_or_drop(x), Some(5)); } +#[test] +fn unwrap_or_drop_linked_list() { + #[derive(Clone)] + struct LinkedList(Option>>); + struct Node(T, Option>>); + + impl Drop for LinkedList { + 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); + } + } + } + + impl LinkedList { + fn push(&mut self, x: T) { + self.0 = Some(Arc::new(Node(x, self.0.take()))); + } + } + + use std::thread; + for _ in 0..25 { + let mut x = LinkedList(None); + for i in 0..100000 { + x.push(i); + } + let y = x.clone(); + let t1 = thread::spawn(|| drop(x)); + let t2 = thread::spawn(|| drop(y)); + t1.join().unwrap(); + t2.join().unwrap(); + } +} + #[test] fn into_from_raw() { let x = Arc::new(box "hello"); From 1ceee61665a85533c63cf32a901a180fd674fc99 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 29 Aug 2020 22:34:16 +0200 Subject: [PATCH 3/5] fix typo, remove superflous test --- library/alloc/src/sync.rs | 2 +- library/alloc/src/sync/tests.rs | 35 --------------------------------- 2 files changed, 1 insertion(+), 36 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index aab721242b28e..53f32098cbee1 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -524,7 +524,7 @@ impl Arc { /// /// 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`]. + /// of [`try_unwrap`][`Arc::try_unwrap`]. /// /// # Examples /// diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index 2a71b82ffd022..10b05b2fd0f31 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -140,41 +140,6 @@ fn unwrap_or_drop() { assert_eq!(Arc::unwrap_or_drop(x), Some(5)); } -#[test] -fn unwrap_or_drop_linked_list() { - #[derive(Clone)] - struct LinkedList(Option>>); - struct Node(T, Option>>); - - impl Drop for LinkedList { - 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); - } - } - } - - impl LinkedList { - fn push(&mut self, x: T) { - self.0 = Some(Arc::new(Node(x, self.0.take()))); - } - } - - use std::thread; - for _ in 0..25 { - let mut x = LinkedList(None); - for i in 0..100000 { - x.push(i); - } - let y = x.clone(); - let t1 = thread::spawn(|| drop(x)); - let t2 = thread::spawn(|| drop(y)); - t1.join().unwrap(); - t2.join().unwrap(); - } -} - #[test] fn into_from_raw() { let x = Arc::new(box "hello"); From 838e5edba7b1cc3a1fc36a4448a501c3333f7705 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 29 Aug 2020 23:06:47 +0200 Subject: [PATCH 4/5] split examples --- library/alloc/src/sync.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 53f32098cbee1..b452a79363e3d 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -528,6 +528,7 @@ impl Arc { /// /// # Examples /// + /// Minimal example demonstrating the guarantee that `unwrap_or_drop` gives. /// ``` /// #![feature(unwrap_or_drop)] /// @@ -548,11 +549,17 @@ impl Arc { /// (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)] /// + /// use std::sync::Arc; /// - /// // For a somewhat more practical example, - /// // we define a singly linked list using `Arc`: + /// // Definition of a simple singly linked list using `Arc`: /// #[derive(Clone)] /// struct LinkedList(Option>>); /// struct Node(T, Option>>); From 08455a631b16b5c4588afcdfd99401d2f83e47fc Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sun, 20 Sep 2020 17:18:04 +0200 Subject: [PATCH 5/5] fix oversight in test where `try_unwrap` was not changed back to `unwrap_or_drop` --- library/alloc/src/sync/tests.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index 10b05b2fd0f31..dffe030e07b00 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -112,11 +112,10 @@ fn unwrap_or_drop() { 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()); + let r_thread = std::thread::spawn(|| Arc::unwrap_or_drop(x)); + let s_thread = std::thread::spawn(|| Arc::unwrap_or_drop(y)); let r = r_thread.join().expect("r_thread panicked"); let s = s_thread.join().expect("s_thread panicked"); assert!(