Deallocate ThinBox even if the value unwinds on drop

[LegionMammal978]

This makes it match the behavior of an ordinary Box.

[rustbot]

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[m-ou-se]

r? @Amanieu (because this is about panic in Drop)

[Amanieu]

@bors r+

[bors]

📌 Commit ce28b4d has been approved by Amanieu

It is now in the queue for this repository.

[LegionMammal978]

I've noticed that Rc<T> and Arc<T> also leak their allocations (by not decrementing the shared weak count) if drop_in_place() unwinds. Is there some perf or soundness reason behind this, or is it also unintentional?

[Amanieu]

Panics in drop are known to leak memory in all sorts of places since it's so hard to handle them reliably. Hence my attempt in rust-lang/rfcs#3288 to make panics in drop always abort.

[LegionMammal978]

The only place they can leak memory is with types with custom Drop impls. I've looked through all the stdlib heap containers: for the most part, we're very careful about correctly proceeding after a panic. Only the types imported from external crates (mpsc channels, HashMap<K, V, S>, HashSet<T, S>, and related types) fail to drop remaining elements after a panic, and only those as well as ThinBox<T> (before this PR), Rc<T>, and Arc<T> fail to deallocate the backing memory. The unstable DrainFilter types are inconsistent as to whether they continue draining, but they already have issues with panicking closures, so I'm not very surprised. Apart from those, all types use a drop guard and/or delegate to operations like drop_in_place::<[T]>() which correctly handle panics.

(And my position on silently changing the stable behavior of Drop hasn't changed since you proposed it.)

[bors]

⌛ Testing commit ce28b4d with merge df75643...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing df75643 to master...

[rust-timer]

Finished benchmarking commit (df75643): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.