Make registries thread-safe, and add pre/post hooks for operations.

Review Request #13802 — Created April 27, 2024 and submitted — Latest diff uploaded

Information

Djblets
release-5.x

Reviewers

We've had some long-standing problems where, on occasion, two threads
will attempt to populate or manipulate a registry at the same time and
end up with bad results. The common scenario is that two threads would
initiate population of the registry, one would win, and the other would
see 0 items in the registry.

Registries were not built to be thread-safe, but they should have been.
Locks around mutation operations can do a lot to help with that, amd
this change introduces those.

Where this got more complicated is how we handled state management and
registry customization.

Registries were previously in one of two states: Populated, or
unpopulated. When population started, but before any default items were
registered, the registry was put into the Populated state, which would
make any other threads think that it was ready to pull items out of the
registry.

What we needed was a third state: Populating. This is set when
population begins, but before/during default item registration. To
represent this, we now have a RegistryState enum, which represents a
PENDING (initial) state, POPULATING, and READY (populated).

The old populating property now returns True if this state is
anything but PENDING, helping cover existing behavior. This is
deprecated in favor of state.

The other part is registry customization. We had registries that would
subclass populate(), register(), and unregister(), adding
specialized pre/post behavior. However, this wasn't compatible with how
locks and state are now managed. The solution here is to have new hook
methods that subclasses can override to manage state at the right time
and with thread-safety guarantees.

Existing registries will continue to work, but just won't have the full
degree of thread safety until they move to the hooks.

The same reentrant lock is shared for all mutation operations, which
helps ensure that one thread won't be populating while another is
resetting or registering. It's still possible that one thread may expect
an item to be there that won't exist due to another thread's operations,
but now at least there won't be mixed state, and it's still always up to
the caller to handle any exceptions anyway.

Along with this, thread safety has been added to
lazy_import_registry(). Previously, if two threads called this
simultaneously, multiple registries could be created, though only one
would be assigned to the resulting variable. Now there's a lock in
place, ensuring only one is ever created. This was likely never an issue
in practice, but it's worth ensuring we don't have side effects from
redundant registry creation.

Djblets and Review Board unit tests pass.

Ran each of these Djblets Registry threading tests without the locks.
It reproduced the issues we've seen sporadically in production.

This will need real-world testing in production.

Commits

Files