From 6a2e8e7633bf40f45db82a27fe021916705ba7cf Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Tue, 25 Aug 2020 17:30:46 +0200 Subject: [PATCH 01/15] 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 5ab930a520884..c2b1a0bfd5942 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -516,6 +516,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 77f328d48f94d..b523530793e38 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 657b5345cbb03e0f040797bc9eba88ee44514f56 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 29 Aug 2020 21:28:51 +0200 Subject: [PATCH 02/15] 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 c2b1a0bfd5942..2e7624573edc8 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -485,6 +485,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 /// /// ``` @@ -525,8 +536,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 /// @@ -538,16 +552,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 b523530793e38..518620da26961 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 a0740b8c445f8a500eeae0704fb18ca302418718 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 29 Aug 2020 22:34:16 +0200 Subject: [PATCH 03/15] 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 2e7624573edc8..45ed9c209f5e9 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -540,7 +540,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 518620da26961..b523530793e38 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 fbedcd57498a9190de813450f197cd951f80ffd6 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sat, 29 Aug 2020 23:06:47 +0200 Subject: [PATCH 04/15] 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 45ed9c209f5e9..dc8682e890057 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -544,6 +544,7 @@ impl Arc { /// /// # Examples /// + /// Minimal example demonstrating the guarantee that `unwrap_or_drop` gives. /// ``` /// #![feature(unwrap_or_drop)] /// @@ -564,11 +565,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 a7bdb905ef646c9bc35e2f4d74b7e60123f3f568 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Sun, 20 Sep 2020 17:18:04 +0200 Subject: [PATCH 05/15] 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 b523530793e38..271d9b720c1cb 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!( From 5d81f76712ea2b9a06d3814ec3d553b51ceedbbe Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 13:25:43 +0100 Subject: [PATCH 06/15] Add SAFETY comment. --- library/alloc/src/sync.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index dc8682e890057..508ad630bcb6c 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -623,11 +623,11 @@ impl Arc { /// ``` #[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`) + // Make sure that the ordinary `Drop` implementation isn’t called as well let mut this = core::mem::ManuallyDrop::new(this); + // Following the implementation of `drop` and `drop_slow` if this.inner().strong.fetch_sub(1, Release) != 1 { return None; } @@ -637,7 +637,13 @@ impl Arc { // 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` + // SAFETY: This mirrors the line + // + // unsafe { ptr::drop_in_place(Self::get_mut_unchecked(self)) }; + // + // in `drop_slow`. Instead of dropping the value behind the pointer + // it is read and eventually returned; `ptr::read` has the same + // safety conditions as `ptr::drop_in_place`. let inner = unsafe { ptr::read(Self::get_mut_unchecked(&mut this)) }; drop(Weak { ptr: this.ptr }); From 00d50fe0288a3caeb65787a0ca8a146e9a39c7da Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 13:45:55 +0100 Subject: [PATCH 07/15] Hide recommendations for using nightly-only API; clean up a `core::mem` path. --- library/alloc/src/sync.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 508ad630bcb6c..0d341bbac6b93 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -485,6 +485,8 @@ impl Arc { /// /// This will succeed even if there are outstanding weak references. /// + // FIXME: when `Arc::unwrap_or_drop` is stabilized, add this paragraph: + /* /// 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 @@ -496,6 +498,7 @@ impl Arc { /// both drop their `Arc` in the call to [`ok`][`Result::ok`], /// taking the strong count from two down to zero. /// + */ /// # Examples /// /// ``` @@ -621,11 +624,14 @@ impl Arc { /// t2.join().unwrap(); /// } /// ``` + + // FIXME: when `Arc::unwrap_or_drop` is stabilized, adjust the documentation of + // `Arc::try_unwrap` according to the `FIXME` presented there. #[inline] #[unstable(feature = "unwrap_or_drop", issue = "none")] // FIXME: add issue pub fn unwrap_or_drop(this: Self) -> Option { // Make sure that the ordinary `Drop` implementation isn’t called as well - let mut this = core::mem::ManuallyDrop::new(this); + let mut this = mem::ManuallyDrop::new(this); // Following the implementation of `drop` and `drop_slow` if this.inner().strong.fetch_sub(1, Release) != 1 { From ca7066d5d959cea81c33738572f365728e85ac2b Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 13:50:56 +0100 Subject: [PATCH 08/15] Comments of unwrap_or_drop test. --- library/alloc/src/sync/tests.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index 271d9b720c1cb..5004f92bff095 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -103,14 +103,8 @@ fn try_unwrap() { #[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 + // ^ increase chances of hitting potential race conditions { let x = Arc::new(3); let y = Arc::clone(&x); From 948e4020e35fc898f7d253ca7b61f4317b067b4c Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 14:06:42 +0100 Subject: [PATCH 09/15] Remove FIXME comment in unwrap_or_drop. --- library/alloc/src/sync.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 0d341bbac6b93..53c6f92f4430a 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -640,9 +640,6 @@ impl Arc { 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? - // SAFETY: This mirrors the line // // unsafe { ptr::drop_in_place(Self::get_mut_unchecked(self)) }; From 7d43bcbde582afc44185a1c72a4b18c4916aa9c0 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 14:21:38 +0100 Subject: [PATCH 10/15] Capitalize comments. --- library/alloc/src/sync.rs | 8 ++++---- library/alloc/src/sync/tests.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 53c6f92f4430a..8b872c6812f87 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -595,7 +595,7 @@ impl Arc { /// } /// } /// - /// // implementation of `new` and `push` omitted + /// // Implementation of `new` and `push` omitted /// impl LinkedList { /// /* ... */ /// # fn new() -> Self { @@ -610,14 +610,14 @@ impl Arc { /// // 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 + /// // 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 + /// x.push(i); // Adds i to the front of x /// } /// let y = x.clone(); /// - /// // drop the clones in parallel + /// // Drop the clones in parallel /// let t1 = std::thread::spawn(|| drop(x)); /// let t2 = std::thread::spawn(|| drop(y)); /// t1.join().unwrap(); diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index 5004f92bff095..b4568e2cf0a7c 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -104,7 +104,7 @@ fn try_unwrap() { #[test] fn unwrap_or_drop() { for _ in 0..100 - // ^ increase chances of hitting potential race conditions + // ^ Increase chances of hitting potential race conditions { let x = Arc::new(3); let y = Arc::clone(&x); From 1e28132c733546c2cc9b8dde7b2462deab35e18c Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 14:40:30 +0100 Subject: [PATCH 11/15] Rename unwrap_or_drop to into_inner. --- library/alloc/src/sync.rs | 30 +++++++++++++++--------------- library/alloc/src/sync/tests.rs | 14 +++++++------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 8b872c6812f87..afa4df9a95018 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -485,9 +485,9 @@ impl Arc { /// /// This will succeed even if there are outstanding weak references. /// - // FIXME: when `Arc::unwrap_or_drop` is stabilized, add this paragraph: + // FIXME: when `Arc::into_inner` is stabilized, add this paragraph: /* - /// It is strongly recommended to use [`Arc::unwrap_or_drop`] instead if you don't + /// 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 @@ -537,7 +537,7 @@ impl Arc { /// /// This will succeed even if there are outstanding weak references. /// - /// If `unwrap_or_drop` is called on every clone of this `Arc`, + /// If `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. /// @@ -547,18 +547,18 @@ impl Arc { /// /// # Examples /// - /// Minimal example demonstrating the guarantee that `unwrap_or_drop` gives. + /// Minimal example demonstrating the guarantee that `into_inner` gives. /// ``` - /// #![feature(unwrap_or_drop)] + /// #![feature(arc_into_inner)] /// /// 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)); + /// // Two threads calling `into_inner` on both clones of an `Arc`: + /// let x_unwrap_thread = std::thread::spawn(|| Arc::into_inner(x)); + /// let y_unwrap_thread = std::thread::spawn(|| Arc::into_inner(y)); /// /// let x_unwrapped_value = x_unwrap_thread.join().unwrap(); /// let y_unwrapped_value = y_unwrap_thread.join().unwrap(); @@ -572,9 +572,9 @@ impl Arc { /// // `Arc::try_unwrap(x).ok()` and `Arc::try_unwrap(y).ok()` instead. /// ``` /// - /// A more practical example demonstrating the need for `unwrap_or_drop`: + /// A more practical example demonstrating the need for `into_inner`: /// ``` - /// #![feature(unwrap_or_drop)] + /// #![feature(arc_into_inner)] /// /// use std::sync::Arc; /// @@ -590,7 +590,7 @@ impl Arc { /// 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); + /// Arc::into_inner(arc).map(|node| x = node.1); /// } /// } /// } @@ -608,7 +608,7 @@ impl Arc { /// /// // 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)`. + /// // `Arc::try_unwrap(arc).ok()` instead of `Arc::into_inner(arc)`. /// { /// // Create a long list and clone it /// let mut x = LinkedList::new(); @@ -625,11 +625,11 @@ impl Arc { /// } /// ``` - // FIXME: when `Arc::unwrap_or_drop` is stabilized, adjust the documentation of + // FIXME: when `Arc::into_inner` is stabilized, adjust the documentation of // `Arc::try_unwrap` according to the `FIXME` presented there. #[inline] - #[unstable(feature = "unwrap_or_drop", issue = "none")] // FIXME: add issue - pub fn unwrap_or_drop(this: Self) -> Option { + #[unstable(feature = "arc_into_inner", issue = "none")] // FIXME: create and add issue + pub fn into_inner(this: Self) -> Option { // Make sure that the ordinary `Drop` implementation isn’t called as well let mut this = mem::ManuallyDrop::new(this); diff --git a/library/alloc/src/sync/tests.rs b/library/alloc/src/sync/tests.rs index b4568e2cf0a7c..3c93d59ff8ac8 100644 --- a/library/alloc/src/sync/tests.rs +++ b/library/alloc/src/sync/tests.rs @@ -102,14 +102,14 @@ fn try_unwrap() { } #[test] -fn unwrap_or_drop() { +fn into_inner() { for _ in 0..100 // ^ Increase chances of hitting potential race conditions { let x = Arc::new(3); let y = Arc::clone(&x); - let r_thread = std::thread::spawn(|| Arc::unwrap_or_drop(x)); - let s_thread = std::thread::spawn(|| Arc::unwrap_or_drop(y)); + let r_thread = std::thread::spawn(|| Arc::into_inner(x)); + let s_thread = std::thread::spawn(|| Arc::into_inner(y)); let r = r_thread.join().expect("r_thread panicked"); let s = s_thread.join().expect("s_thread panicked"); assert!( @@ -121,16 +121,16 @@ fn unwrap_or_drop() { } let x = Arc::new(3); - assert_eq!(Arc::unwrap_or_drop(x), Some(3)); + assert_eq!(Arc::into_inner(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)); + assert_eq!(Arc::into_inner(x), None); + assert_eq!(Arc::into_inner(y), Some(4)); let x = Arc::new(5); let _w = Arc::downgrade(&x); - assert_eq!(Arc::unwrap_or_drop(x), Some(5)); + assert_eq!(Arc::into_inner(x), Some(5)); } #[test] From 81623f4452c7d9468fb75005bdd53cd9313d9294 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 14:54:44 +0100 Subject: [PATCH 12/15] Fully qualified associated functions in documentation. --- library/alloc/src/sync.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index afa4df9a95018..ae92b7fa372bf 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -494,7 +494,7 @@ impl Arc { /// 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 + /// have the last clone of their `Arc` via `Arc::try_unwrap`, and then /// both drop their `Arc` in the call to [`ok`][`Result::ok`], /// taking the strong count from two down to zero. /// @@ -537,17 +537,17 @@ impl Arc { /// /// This will succeed even if there are outstanding weak references. /// - /// If `into_inner` is called on every clone of this `Arc`, + /// 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. /// /// 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 [`Arc::try_unwrap`]. /// /// # Examples /// - /// Minimal example demonstrating the guarantee that `into_inner` gives. + /// Minimal example demonstrating the guarantee that `Arc::into_inner` gives. /// ``` /// #![feature(arc_into_inner)] /// @@ -556,7 +556,7 @@ impl Arc { /// let x = Arc::new(3); /// let y = Arc::clone(&x); /// - /// // Two threads calling `into_inner` on both clones of an `Arc`: + /// // Two threads calling `Arc::into_inner` on both clones of an `Arc`: /// let x_unwrap_thread = std::thread::spawn(|| Arc::into_inner(x)); /// let y_unwrap_thread = std::thread::spawn(|| Arc::into_inner(y)); /// @@ -572,7 +572,7 @@ impl Arc { /// // `Arc::try_unwrap(x).ok()` and `Arc::try_unwrap(y).ok()` instead. /// ``` /// - /// A more practical example demonstrating the need for `into_inner`: + /// A more practical example demonstrating the need for `Arc::into_inner`: /// ``` /// #![feature(arc_into_inner)] /// From f57de565adf027312a0c9632be97aa0a71e3e7af Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 15:23:32 +0100 Subject: [PATCH 13/15] Apply fmt. --- library/alloc/src/sync.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index ae92b7fa372bf..1d83acdb404f8 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -498,7 +498,7 @@ impl Arc { /// both drop their `Arc` in the call to [`ok`][`Result::ok`], /// taking the strong count from two down to zero. /// - */ + */ /// # Examples /// /// ``` From cd444ca6f65264062dbb873cd2e38a75a8b34dfe Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 16:10:49 +0100 Subject: [PATCH 14/15] Flatten block in into_inner example. --- library/alloc/src/sync.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 1d83acdb404f8..1ecf9606c85d7 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -609,20 +609,19 @@ impl Arc { /// // 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::into_inner(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(); + /// // 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(); /// ``` // FIXME: when `Arc::into_inner` is stabilized, adjust the documentation of From ecb85f03bac849c13f8af371dee816ba9a82d595 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 3 Dec 2020 18:56:49 +0100 Subject: [PATCH 15/15] into_inner example: prefer "imperative" `if let` over `.map()` Co-authored-by: Daniel Henry-Mantilla --- library/alloc/src/sync.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 1ecf9606c85d7..1e86b28cfd74a 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -588,9 +588,11 @@ impl Arc { /// // 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::into_inner(arc).map(|node| x = node.1); + /// let mut link = self.0.take(); + /// while let Some(arc_node) = link.take() { + /// if let Some(Node(_value, next)) = Arc::into_inner(arc_node) { + /// link = next; + /// } /// } /// } /// }