HashMap: panic in element destructor causes leaks of unrelated elements

[lukas-code]

When dropping a HashMap (or HashSet) and an element's destructor panics, then all elements that would be dropped after are leaked. This is inconsistent with other std collections (Vec, LinkedList, BTreeMap), where after panic in one destructor the remaining destructors are still called, potentially causing an abort if another one panics.

I tried this code: playground

use std::cell::Cell;
use std::collections::{BTreeMap, HashMap, LinkedList};
use std::panic::{catch_unwind, AssertUnwindSafe};

struct Dropper<'a>(&'a Cell<u32>);

impl Drop for Dropper<'_> {
    fn drop(&mut self) {
        let count = self.0.get();
        self.0.set(count + 1);
        if count == 0 {
            panic!("oh no");
        }
    }
}

fn main() {
    // Vec
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(vec![[Dropper(&count), Dropper(&count)]]);
    }))
    .unwrap_err();
    println!("vec: {}", count.get());

    // LinkedList
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(LinkedList::from([Dropper(&count), Dropper(&count)]));
    }))
    .unwrap_err();
    println!("linked list: {}", count.get());

    // BTreeMap
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(BTreeMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
    }))
    .unwrap_err();
    println!("b-tree map: {}", count.get());

    // HashMap
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(HashMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
    }))
    .unwrap_err();
    println!("hash map: {}", count.get());
}

I expected to see this happen: The drop behavior of Vec, LinkedList, BTreeMap and HashMap should be consistent.

Instead, this happened: HashMap drops only one element if the destructor unwinds, but the others drop both elements.

running Miri on the code shows a memory leak

error: memory leaked: alloc17016 (Rust heap, size: 76, align: 8), allocated here:
  --> /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/alloc.rs:15:15
   |
15 |         match alloc.allocate(layout) {
   |               ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: BACKTRACE:
   = note: inside `hashbrown::raw::alloc::inner::do_alloc::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/alloc.rs:15:15: 15:37
   = note: inside `hashbrown::raw::RawTableInner::new_uninitialized::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:1534:38: 1534:61
   = note: inside `hashbrown::raw::RawTableInner::fallible_with_capacity::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:1572:30: 1572:96
   = note: inside `hashbrown::raw::RawTableInner::prepare_resize::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:2633:13: 2633:94
   = note: inside `hashbrown::raw::RawTableInner::resize_inner::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:2829:29: 2829:86
   = note: inside `hashbrown::raw::RawTableInner::reserve_rehash_inner::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:2719:13: 2725:14
   = note: inside `hashbrown::raw::RawTable::<(i32, Dropper<'_>)>::reserve_rehash::<{closure@hashbrown::map::make_hasher<i32, Dropper<'_>, std::hash::RandomState>::{closure#0}}>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:1045:13: 1056:14
   = note: inside `hashbrown::raw::RawTable::<(i32, Dropper<'_>)>::reserve::<{closure@hashbrown::map::make_hasher<i32, Dropper<'_>, std::hash::RandomState>::{closure#0}}>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:993:20: 994:81
   = note: inside `hashbrown::map::HashMap::<i32, Dropper<'_>, std::hash::RandomState>::reserve` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/map.rs:1102:9: 1103:77
   = note: inside `<hashbrown::map::HashMap<i32, Dropper<'_>, std::hash::RandomState> as std::iter::Extend<(i32, Dropper<'_>)>>::extend::<[(i32, Dropper<'_>); 2]>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/map.rs:4489:9: 4489:30
   = note: inside `<std::collections::HashMap<i32, Dropper<'_>> as std::iter::Extend<(i32, Dropper<'_>)>>::extend::<[(i32, Dropper<'_>); 2]>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:3185:9: 3185:31
   = note: inside `<std::collections::HashMap<i32, Dropper<'_>> as std::iter::FromIterator<(i32, Dropper<'_>)>>::from_iter::<[(i32, Dropper<'_>); 2]>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:3170:9: 3170:25
   = note: inside `<std::collections::HashMap<i32, Dropper<'_>> as std::convert::From<[(i32, Dropper<'_>); 2]>>::from` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:1405:9: 1405:29
note: inside closure
  --> src/main.rs:45:14
   |
45 |         drop(HashMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: inside `<{closure@src/main.rs:44:35: 44:37} as std::ops::FnOnce<()>>::call_once - shim` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
   = note: inside `<std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}> as std::ops::FnOnce<()>>::call_once` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:272:9: 272:19
   = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}>, ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:557:40: 557:43
   = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}>>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:520:19: 520:88
   = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}>, ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:358:14: 358:33
note: inside `main`
  --> src/main.rs:44:5
   |
44 | /     catch_unwind(AssertUnwindSafe(|| {
45 | |         drop(HashMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
46 | |     }))
   | |_______^

Meta

rustc --version --verbose:

rustc 1.84.0-nightly (c1db4dc24 2024-10-25)
binary: rustc
commit-hash: c1db4dc24267a707409c9bf2e67cf3c7323975c8
commit-date: 2024-10-25
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.1

@rustbot label T-libs A-collections A-destructors I-memleak

[RalfJung]

The drop behavior of Vec, LinkedList, BTreeMap and HashMap should be consistent.

AFAIK none of them promises to have this kind of fancy "recovery from panicky drop" behavior. So you shouldn't extrapolate from "some data structures go through the trouble of supporting this" to "all data structure should support this".

Cc @rust-lang/libs-api

[Amanieu]

This is a well known issue for which there is no good solution. There are plenty of places where panicking in a drop can lead to memory leaks. IMO the best solution is to simply force any such panics to abort as proposed in rust-lang/rfcs#3288.

[lukas-code]

there is no good solution

Why wouldn't it be a solution to simply continue dropping the remaining elements (and abort on the second panic), like all other collections already do? I don't see why HashMap has to be special here and, in particular, why we should treat the destructor of a HashMap<T, U> any differently than the destructor of a Box<[T; N]> for example.

IMO the best solution is to simply force any such panics

Sure, that would also fix the issue here. In the meantime, until your RFC gets merged, maybe hashbrown could wrap its calls to drop_in_place in abort_unwind to clearly indicate that unwinding destructors aren't supported?