Redo how RelationCounterField handles instance events and states.

Review Request #9760 — Created March 12, 2018 and submitted

Christian Hammond

We've been hitting deadlocks lately, after introducing some changes to
RelationCounterField. This began to occur after introducing weak
reference destroy callbacks that were built to clean up state. As part
of that cleanup process, threading locks would be acquired to ensure
that other threads weren't stomping on the same state.

This ended up having some substantial problems. During thread/process
shutdown, Python begins removing objects from memory, triggering weak
reference destroy callbacks. We have no guarantee of the order, and it's
very common for the destroy callback to hit garbage memory, sometimes
manifesting as AttributeErrors. The thought is that, during this, it's
also possible for a lock to be acquired and then never released,
preventing shutdown and deadlocking. This was made more complicated by
the inter-dependency between model instances, its weak references, and
the InstanceState owning them.

This has all been redone to be far simpler and to move all the important
things out of InstanceState. Some of that was covered in previous
changes, and this change does the rest.

InstanceState is now a subclass of weakref.ref that stores a bit
more state (this is safe and documented in the Python documentation).
None of the state refers to other shared instances. In particular,
fields are no longer stored, just their names, and the signal
connections/handlers are gone. There's also no more destroyed handler.

RelationCounterField now handles the signals for pre-delete and
post-save, looking up InstanceStates as necessary instead of having
those handlers on the states. This means fewer registered signal
connections, fewer manipulations of the signal receiver lists, and less
risk with cleaning up deeply-nested objects.

State is no longer immediately cleaned up when model instances go away.
Instead, it's cleaned up when storing new state, on pre-delete, and when
explicitly checking if there are tracked instances (just used in unit

This means no more attempting to clean up state and grab locks while
memory is being freed, which will hopefully fix any chance of deadlocks.

Unit tests pass.

David Trowbridge
  1. Ship It!
Christian Hammond
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (37c1a51)