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

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

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.

Summary ID
Make registries thread-safe, and add pre/post hooks for operations.
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.
39bec43294eabd7e7c4b7be10c435cf4eb688d1b
Description From Last Updated

This sentence doesn't make sense. I think you meant to say "usage and"? Or take out "usage an" altogether.

maubinmaubin

Should add a "Version Added".

maubinmaubin

Should add a "Version Added".

maubinmaubin

I don't think we need to includes this "Raises" section in the base class?

maubinmaubin

I think this can be taken out now? Per my comment in the previous review

maubinmaubin
chipx86
maubin
  1. 
      
  2. djblets/registries/registry.py (Diff revision 1)
     
     
    Show all issues

    This sentence doesn't make sense. I think you meant to say "usage and"? Or take out "usage an" altogether.

    1. Yeah, should have been "and".

  3. djblets/registries/registry.py (Diff revision 1)
     
     
     
    Show all issues

    Should add a "Version Added".

    1. This is from the original implementation. I'm just moving typing to the body and out of __init__.

  4. djblets/registries/registry.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Should add a "Version Added".

  5. djblets/registries/registry.py (Diff revision 1)
     
     
     
     
    Show all issues

    I don't think we need to includes this "Raises" section in the base class?

    1. I actually think this is useful because it tells people writing subclasses what they should do in their implementation.

    2. Good point. Then we might want to include some other errors here and in the other on_item_* methods like djblets.registries.errors.AlreadyRegisteredError and djblets.registries.errors.ItemLookupError

    3. This is the only place where exceptions are allowed. The other methods aren't permitted to raise exceptions.

      The point of this first one is to allow for some pre-registration validation, but we don't want to disallow unregistration, population, or resetting.

    4. Got it. I just saw that for OrderedRegistry.on_item_unregistered we say in the docs that it could raise a djblets.registries.errors.ItemLookupError, but I see this doesn't apply anymore because we took out the call to super().unregister(item) there. So we should get rid of the "Raises" sections in the docs for OrderedRegistry.on_item_unregistered (and for OrderedRegistry.on_item_registered).

  6. 
      
chipx86
maubin
  1. 
      
  2. djblets/registries/registry.py (Diff revision 2)
     
     
     
     
    Show all issues

    I think this can be taken out now? Per my comment in the previous review

    1. Oops, I thought I got rid of that block.

  3. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.x (3b6426e)