Can a destructor ever be called twice automatically?

I have a Arc<MyItem> which is tracked by a HashMap. The HashMap has Arc::Weak references to all the MyItem instances.
Creating a MyItem puts a weak reference to it in the map. Dropping a MyItem results in Drop being called, which removes the weak reference from the map. Multiple threads are creating and deleting MyItem items. 100% safe Rust code. No explicit calls to destructors.

On rare occasions, the same item is removed from the map twice, which causes a panic in my code. So I'm looking for a race condition.

General questions:

  • Is Drop ever called automatically more than once on the same item?

  • Is there anything that can go wrong when Arc and Weak are used on something with a Drop defined?

  • Is item.strong_count() > 0 always equivalent to item.upgrade().is_some()?

  • If an Arc<Item> is being dropped, what is the strong count on the Arc inside the destructor? Does the count decrease before or after the destructor runs?

In sound code, the drop glue is only ever run for a value at most once. Running drop glue multiple times can trivially cause UB, e.g. double-free of a Box.

When the last strong reference of Arc is dropped, this drops the contained value. If the value can reach a Weak of itself, it will see a strong count of 0. This is necessary because once the value is being destroyed, it can't be possible to cancel this by creating a new strong reference. [playground demo]

I suspect the race is likely to be TOCTOU — e.g. that you're checking .strong_count() > 0 and then doing something that assumes a strong reference exists, but the last strong ref is dropped between the Time Of Check and Time Of Use.

7 Likes

I suspect you're right. There's a mutex around the accesses to the hash, but not around the accesses to the Arc. My code needs a rethink.

If you have no unsafe code yourself, one of your dependencies has a soundness bug (could be std). You can try running your code with miri to find the offending piece of code.

Often, it should be sufficient to use .upgrade() and then hold that upgrade for the duration that you expect to still have access to the value instead of checking .strong_count() > 0.

The other possibility is that there's some sort of collision involved, such that a lookup is trying to upgrade a weak reference to an item that is in the process of being deleted. It could even be an insert growing the map and asking for the hash of the key again, without any real collision.

Check wherever the backtrace indicates the panic is coming from and see if there's a reasonable fallback behavior for the situation which is causing a panic. The probability that the situation can be legitimately reached and isn't actually a double remove is decent. (e.g. two items get deleted concurrently, and thus removing the first sees two zombie entries; the second sees whatever the first left behind.)

6 Likes

A way to think about this is that soon as item.strong_count() returns, the actual strong count may be different. You can't even say that item.strong_count() > 0 is always equivalent to an immediately subsequent item.strong_count() > 0, so it can't always be equivalent to anything else either.

For this reason, it is pretty rare in my experience to see code using these counts for application business logic, or correctness checks, etc. They're good for debugging and tracing to get a sense of things, though.

Personally, I think the docs for both Weak::strong_count and Weak::weak_count are incomplete in this respect. They should both simply state that the value returned is only guaranteed to be correct for some point in the past, but may be off by an arbitrary amount if other threads are manipulating Arc or Weak pointers to the same object. The docs for weak_count suggest that the error, if any, must be by a delta of 1 or -1, which is not true, and the absence of a warning for strong_count could be taken to suggest that there will never be a problem.

4 Likes

Agreed. Looking at the Arc counts is unsound.

The item the Arc points to is immutable, which is why it's just an Arc, not an Arc<Mutex<>>. So there's no flag for "present in index" within the item.

During creation, and inside Drop, there's mutable access to the otherwise-immutable item.
So adding an "is_in_index" bool is possible. Set that at creation, clear it when removed from the index at Drop. Check that flag when it's necessary to know if it is still indexed.

Maybe. Want to think about this some more.

1 Like

Could your problem be your Hash implementation? Because Weak does not implement it.

If you are doing the hash and equality based on the value of the weak this will have changed before the removal from the map and broken the HashMap

The Arc::Weak is the value, not the key. Not a problem.

Right now, I'm testing a new approach that doesn't look at strong or weak counts. The "am I in the index" flag is inside the object, not inferred from the Arc, and it's only accessed when the index is locked. That has better soundness.

I'll have to run it overnight to get enough test time to have a good chance of hitting this rare race condition.

I did put a flag inside the struct, where it's only accessed when the struct is locked. No more looking at strong_count() for anything other than debug. That eliminated the race condition. The program then ran overnight without crashing. Thanks.

4 Likes