From 3f9cdc52cddec6ae3b303901d4260e03e964c559 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 25 Mar 2018 23:20:52 +0200 Subject: [PATCH 1/4] TEST: Add test that we drop all elements even if one of them panics --- tests/tests.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/tests.rs b/tests/tests.rs index de3507a..7e3c4c7 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -107,6 +107,65 @@ fn test_drop() { } +#[test] +fn test_drop_panics() { + use std::cell::Cell; + use std::panic::catch_unwind; + use std::panic::AssertUnwindSafe; + + let flag = &Cell::new(0); + + struct Bump<'a>(&'a Cell); + + // Panic in the first drop + impl<'a> Drop for Bump<'a> { + fn drop(&mut self) { + let n = self.0.get(); + self.0.set(n + 1); + if n == 0 { + panic!("Panic in Bump's drop"); + } + } + } + + { + let mut array = ArrayVec::<[Bump; 128]>::new(); + array.push(Bump(flag)); + array.push(Bump(flag)); + array.push(Bump(flag)); + + let res = catch_unwind(AssertUnwindSafe(|| { + drop(array); + })); + assert!(res.is_err()); + } + // Check that all the elements drop, even if the first drop panics. + assert_eq!(flag.get(), 3); + + + flag.set(0); + { + let mut array = ArrayVec::<[Bump; 16]>::new(); + array.push(Bump(flag)); + array.push(Bump(flag)); + array.push(Bump(flag)); + array.push(Bump(flag)); + array.push(Bump(flag)); + + let i = 2; + let tail_len = array.len() - i; + + let res = catch_unwind(AssertUnwindSafe(|| { + array.truncate(i); + })); + assert!(res.is_err()); + // Check that all the tail elements drop, even if the first drop panics. + assert_eq!(flag.get(), tail_len as i32); + } + + +} + #[test] fn test_extend() { let mut range = 0..10; From 602e55dc67b04e97dd96f45f9b80e0b0daa13060 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 25 Mar 2018 23:21:18 +0200 Subject: [PATCH 2/4] FEAT: Use drop_in_place for truncate and clear (and drop) This should perform better in both release and debug mode. NOTE: This is significant because it changes the drop order of the elements, and how we handle panicking destructors. If just one of the destructors panic during ArrayVec drop, clear or truncate, the rest of the elements should still drop. If we encounter another panic during that process, however, Rust will abort as usual for panic during unwinding. --- src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 96e7a24..3f16fc9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -459,12 +459,18 @@ impl ArrayVec { /// assert_eq!(&array[..], &[1, 2, 3]); /// ``` pub fn truncate(&mut self, len: usize) { - while self.len() > len { self.pop(); } + unsafe { + if len < self.len() { + let tail: *mut [_] = &mut self[len..]; + self.set_len(len); + ptr::drop_in_place(tail); + } + } } /// Remove all elements in the vector. pub fn clear(&mut self) { - while let Some(_) = self.pop() { } + self.truncate(0) } /// Retains only the elements specified by the predicate. From 9db64d5948039ddce5d7ffefc561c61513d9abca Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 25 Mar 2018 23:23:32 +0200 Subject: [PATCH 3/4] FIX: Fix a typo in a comment --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 3f16fc9..22743c3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -543,7 +543,7 @@ impl ArrayVec { // Memory safety // // When the Drain is first created, it shortens the length of - // the source vector to make sure no uninitalized or moved-from elements + // the source vector to make sure no uninitialized or moved-from elements // are accessible at all if the Drain's destructor never gets to run. // // Drain will ptr::read out the values to remove. From ac61ce748b3f1509f7e09b1021ac7dbb431f4259 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 25 Mar 2018 23:49:42 +0200 Subject: [PATCH 4/4] TEST: Fix drop tests for older Rust In the test, test with Vec first to see how Rust implements panic recovery while elements drop. If Vec drops all the elements, then we test that arrayvec does too. --- tests/tests.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/tests.rs b/tests/tests.rs index 7e3c4c7..5b0f3aa 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -127,7 +127,22 @@ fn test_drop_panics() { } } } + // check if rust is new enough + flag.set(0); + { + let array = vec![Bump(flag), Bump(flag)]; + let res = catch_unwind(AssertUnwindSafe(|| { + drop(array); + })); + assert!(res.is_err()); + } + if flag.get() != 2 { + println!("test_drop_panics: skip, this version of Rust doesn't continue in drop_in_place"); + return; + } + + flag.set(0); { let mut array = ArrayVec::<[Bump; 128]>::new(); array.push(Bump(flag));