From ab56b6eb5e967b0c0246dc57c8dc4a88d9d5cbe6 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 1 Mar 2021 14:00:47 -0600 Subject: [PATCH] Add BAR.rebuild() to fix the refcount issue, and to change datatypes. It works by using the new methods allRegistrations() and allSubscriptions() to re-create the data in new data structures. This makes fresh calls to subscribe() and register(). I went this way instead of trying to manually walk the data structures and create them because the logic in those methods is fully tested. --- CHANGES.rst | 3 + src/zope/interface/adapter.py | 120 ++++++++- src/zope/interface/tests/test_adapter.py | 304 +++++++++++++++++++++-- 3 files changed, 406 insertions(+), 21 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 6e1fcd85..ff3254cb 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -20,6 +20,9 @@ solve any already corrupted reference counts. See `issue 227 `_. +- Add the method ``BaseAdapterRegistry.rebuild()``. This can be used + to fix the reference counting issue mentioned above, as well as to + update the data structures when custom data types have changed. 5.2.0 (2020-11-05) ================== diff --git a/src/zope/interface/adapter.py b/src/zope/interface/adapter.py index 4d1d9a84..2576f459 100644 --- a/src/zope/interface/adapter.py +++ b/src/zope/interface/adapter.py @@ -52,19 +52,23 @@ class BaseAdapterRegistry(object): A basic implementation of the data storage and algorithms required for a :class:`zope.interface.interfaces.IAdapterRegistry`. - Subclasses or instances can set the following attributes (before - calling this object's ``__init__`` method) to control how the data + Subclasses can set the following attributes to control how the data is stored; in particular, these hooks can be helpful for ZODB persistence: _sequenceType = list - This is the type used for our top-level "byorder" sequences. - These are usually small (< 10) and are almost always accessed when - using this object, so it is rarely useful to change. + This is the type used for our two top-level "byorder" sequences. + These are usually small (< 10). Although at least one of them is + accessed when performing lookups or queries on this object, the other + is untouched. In many common scenarios, both are only required when + mutating registrations and subscriptions (like what + :meth:`zope.interface.interfaces.IComponents.registerUtility` does). + This use pattern makes it an ideal candidate to be a ``PersistentList``. _leafSequenceType = tuple This is the type used for the leaf sequences of subscribers. It could be set to a ``PersistentList`` to avoid many unnecessary data - loads when subscribers aren't being used. + loads when subscribers aren't being used. See :meth:`_addValueToLeaf` + and :meth:`removeValueFromLeaf` for two methods you'll want to override. _mappingType = dict This is the type used for the keyed mappings. A ``PersistentMapping`` could be used to help reduce the number of data loads when the registry is large @@ -78,6 +82,9 @@ class BaseAdapterRegistry(object): are always integers, so one might choose to use a more optimized data structure such as a ``OIBTree``. The same caveats apply as for ``_mappingType``. + It is possible to also set these on an instance, but because of the need to + potentially also override :meth:`addValueToLeaf` and :meth:`removeValueFromLeaf`, + this may be less useful in a persistent scenario; using a subclass is recommended. .. versionchanged:: 5.3.0 Add support for customizing the way data @@ -264,6 +271,44 @@ def registered(self, required, provided, name=u''): return components.get(name) + @classmethod + def _allKeys(cls, components, i, parent_k=()): + if i == 0: + for k, v in components.items(): + yield parent_k + (k,), v + else: + for k, v in components.items(): + new_parent_k = parent_k + (k,) + for x, y in cls._allKeys(v, i - 1, new_parent_k): + yield x, y + + def _all_entries(self, byorder): + # Locally reference the `byorder` data; it might be replaced while + # this method is running. + for i, components in enumerate(byorder): + # We will have *i* levels of dictionaries to go before + # we get to the leaf. + for key, value in self._allKeys(components, i + 1): + assert len(key) == i + 2 + required = key[:i] + provided = key[-2] + name = key[-1] + yield (required, provided, name, value) + + def allRegistrations(self): + """ + Yields tuples ``(required, provided, name, value)`` for all + the registrations that this object holds. + + These tuples could be passed as the arguments to the + :meth:`register` method on another adapter registry to + duplicate the registrations this object holds. + + .. versionadded:: 5.3.0 + """ + for t in self._all_entries(self._adapters): + yield t + def unregister(self, required, provided, name, value=None): required = tuple([_convert_None_to_Interface(r) for r in required]) order = len(required) @@ -325,7 +370,7 @@ def subscribe(self, required, provided, value): for k in key: d = components.get(k) if d is None: - d = {} + d = self._mappingType() components[k] = d components = d @@ -339,6 +384,21 @@ def subscribe(self, required, provided, value): self.changed(self) + def allSubscriptions(self): + """ + Yields tuples ``(required, provided, value)`` for all the + subscribers that this object holds. + + These tuples could be passed as the arguments to the + :meth:`subscribe` method on another adapter registry to + duplicate the registrations this object holds. + + .. versionadded:: 5.3.0 + """ + for required, provided, _name, value in self._all_entries(self._subscribers): + for v in value: + yield (required, provided, v) + def unsubscribe(self, required, provided, value=None): required = tuple([_convert_None_to_Interface(r) for r in required]) order = len(required) @@ -405,6 +465,52 @@ def unsubscribe(self, required, provided, value=None): self.changed(self) + def rebuild(self): + """ + Rebuild (and replace) all the internal data structures of this + object. + + This is useful, especially for persistent implementations, if + you suspect an issue with reference counts keeping interfaces + alive even though they are no longer used. + + It is also useful if you or a subclass change the data types + (``_mappingType`` and friends) that are to be used. + + This method replaces all internal data structures with new objects; + it specifically does not re-use any storage. + + .. versionadded:: 5.3.0 + """ + + # Grab the iterators, we're about to discard their data. + registrations = self.allRegistrations() + subscriptions = self.allSubscriptions() + + def buffer(it): + # The generator doesn't actually start running until we + # ask for its next(), by which time the attributes will change + # unless we do so before calling __init__. + from itertools import chain + try: + first = next(it) + except StopIteration: + return iter(()) + + return chain((first,), it) + + registrations = buffer(registrations) + subscriptions = buffer(subscriptions) + + + # Replace the base data structures as well as _v_lookup. + self.__init__(self.__bases__) + # Re-register everything previously registered and subscribed. + for args in registrations: + self.register(*args) + for args in subscriptions: + self.subscribe(*args) + # XXX hack to fake out twisted's use of a private api. We need to get them # to use the new registed method. def get(self, _): # pragma: no cover diff --git a/src/zope/interface/tests/test_adapter.py b/src/zope/interface/tests/test_adapter.py index de753eab..a80e4f1f 100644 --- a/src/zope/interface/tests/test_adapter.py +++ b/src/zope/interface/tests/test_adapter.py @@ -47,6 +47,60 @@ class IR1(IR0): return IB0, IB1, IB2, IB3, IB4, IF0, IF1, IR0, IR1 +# Custom types to use as part of the AdapterRegistry data structures. +# Our custom types do strict type checking to make sure +# types propagate through the data tree as expected. +class CustomDataTypeBase(object): + _data = None + def __getitem__(self, name): + return self._data[name] + + def __setitem__(self, name, value): + self._data[name] = value + + def __delitem__(self, name): + del self._data[name] + + def __len__(self): + return len(self._data) + + def __contains__(self, name): + return name in self._data + + def __eq__(self, other): + if other is self: + return True + # pylint:disable=unidiomatic-typecheck + if type(other) != type(self): + return False + return other._data == self._data + + def __repr__(self): + return repr(self._data) + +class CustomMapping(CustomDataTypeBase): + def __init__(self, other=None): + self._data = {} + if other: + self._data.update(other) + self.get = self._data.get + self.items = self._data.items + + +class CustomSequence(CustomDataTypeBase): + def __init__(self, other=None): + self._data = [] + if other: + self._data.extend(other) + self.append = self._data.append + +class CustomLeafSequence(CustomSequence): + pass + +class CustomProvided(CustomMapping): + pass + + class BaseAdapterRegistryTests(unittest.TestCase): maxDiff = None @@ -153,6 +207,115 @@ def test_register(self): IR0: 1 })) + registered = list(registry.allRegistrations()) + self.assertEqual(registered, [( + (IB0,), # required + IR0, # provided + '', # name + 'A1' # value + )]) + + def test_register_multiple_allRegistrations(self): + IB0, IB1, IB2, IB3, IB4, IF0, IF1, IR0, IR1 = _makeInterfaces() # pylint:disable=unused-variable + registry = self._makeOne() + # Use several different depths and several different names + registry.register([], IR0, '', 'A1') + registry.register([], IR0, 'name1', 'A2') + + registry.register([IB0], IR0, '', 'A1') + registry.register([IB0], IR0, 'name2', 'A2') + registry.register([IB0], IR1, '', 'A3') + registry.register([IB0], IR1, 'name3', 'A4') + + registry.register([IB0, IB1], IR0, '', 'A1') + registry.register([IB0, IB2], IR0, 'name2', 'A2') + registry.register([IB0, IB2], IR1, 'name4', 'A4') + registry.register([IB0, IB3], IR1, '', 'A3') + + def build_adapters(L, MT): + return L([ + # 0 + MT({ + IR0: MT({ + '': 'A1', + 'name1': 'A2' + }) + }), + # 1 + MT({ + IB0: MT({ + IR0: MT({ + '': 'A1', + 'name2': 'A2' + }), + IR1: MT({ + '': 'A3', + 'name3': 'A4' + }) + }) + }), + # 3 + MT({ + IB0: MT({ + IB1: MT({ + IR0: MT({'': 'A1'}) + }), + IB2: MT({ + IR0: MT({'name2': 'A2'}), + IR1: MT({'name4': 'A4'}), + }), + IB3: MT({ + IR1: MT({'': 'A3'}) + }) + }), + }), + ]) + + self.assertEqual(registry._adapters, + build_adapters(L=self._getMutableListType(), + MT=self._getMappingType())) + + registered = sorted(registry.allRegistrations()) + self.assertEqual(registered, [ + ((), IR0, '', 'A1'), + ((), IR0, 'name1', 'A2'), + ((IB0,), IR0, '', 'A1'), + ((IB0,), IR0, 'name2', 'A2'), + ((IB0,), IR1, '', 'A3'), + ((IB0,), IR1, 'name3', 'A4'), + ((IB0, IB1), IR0, '', 'A1'), + ((IB0, IB2), IR0, 'name2', 'A2'), + ((IB0, IB2), IR1, 'name4', 'A4'), + ((IB0, IB3), IR1, '', 'A3') + ]) + + # We can duplicate to another object. + registry2 = self._makeOne() + for args in registered: + registry2.register(*args) + + self.assertEqual(registry2._adapters, registry._adapters) + self.assertEqual(registry2._provided, registry._provided) + + # We can change the types and rebuild the data structures. + registry._mappingType = CustomMapping + registry._leafSequenceType = CustomLeafSequence + registry._sequenceType = CustomSequence + registry._providedType = CustomProvided + def addValue(existing, new): + existing = existing if existing is not None else CustomLeafSequence() + existing.append(new) + return existing + registry._addValueToLeaf = addValue + + registry.rebuild() + + self.assertEqual(registry._adapters, + build_adapters( + L=CustomSequence, + MT=CustomMapping + )) + def test_register_with_invalid_name(self): IB0, IB1, IB2, IB3, IB4, IF0, IF1, IR0, IR1 = _makeInterfaces() # pylint:disable=unused-variable registry = self._makeOne() @@ -166,6 +329,8 @@ def test_register_with_value_None_unregisters(self): registry.register([None], IR0, '', None) self.assertEqual(len(registry._adapters), 0) self.assertIsInstance(registry._adapters, self._getMutableListType()) + registered = list(registry.allRegistrations()) + self.assertEqual(registered, []) def test_register_with_same_value(self): from zope.interface import Interface @@ -187,10 +352,20 @@ def test_register_with_same_value(self): ) } )) + registered = list(registry.allRegistrations()) + self.assertEqual(registered, [( + (Interface,), # required + IR0, # provided + '', # name + _value # value + )]) + def test_registered_empty(self): registry = self._makeOne() self.assertEqual(registry.registered([None], None, ''), None) + registered = list(registry.allRegistrations()) + self.assertEqual(registered, []) def test_registered_non_empty_miss(self): IB0, IB1, IB2, IB3, IB4, IF0, IF1, IR0, IR1 = _makeInterfaces() # pylint:disable=unused-variable @@ -509,32 +684,133 @@ def test_unsubscribe_instance_method(self): registry.unsubscribe([IB1], None, self._instance_method_notify_target) self.assertEqual(len(registry._subscribers), 0) + def test_subscribe_multiple_allRegistrations(self): + IB0, IB1, IB2, IB3, IB4, IF0, IF1, IR0, IR1 = _makeInterfaces() # pylint:disable=unused-variable + registry = self._makeOne() + # Use several different depths and several different values + registry.subscribe([], IR0, 'A1') + registry.subscribe([], IR0, 'A2') + + registry.subscribe([IB0], IR0, 'A1') + registry.subscribe([IB0], IR0, 'A2') + registry.subscribe([IB0], IR1, 'A3') + registry.subscribe([IB0], IR1, 'A4') + + registry.subscribe([IB0, IB1], IR0, 'A1') + registry.subscribe([IB0, IB2], IR0, 'A2') + registry.subscribe([IB0, IB2], IR1, 'A4') + registry.subscribe([IB0, IB3], IR1, 'A3') + + + def build_subscribers(L, F, MT): + return L([ + # 0 + MT({ + IR0: MT({ + '': F(['A1', 'A2']) + }) + }), + # 1 + MT({ + IB0: MT({ + IR0: MT({ + '': F(['A1', 'A2']) + }), + IR1: MT({ + '': F(['A3', 'A4']) + }) + }) + }), + # 3 + MT({ + IB0: MT({ + IB1: MT({ + IR0: MT({'': F(['A1'])}) + }), + IB2: MT({ + IR0: MT({'': F(['A2'])}), + IR1: MT({'': F(['A4'])}), + }), + IB3: MT({ + IR1: MT({'': F(['A3'])}) + }) + }), + }), + ]) + + self.assertEqual(registry._subscribers, + build_subscribers( + L=self._getMutableListType(), + F=self._getLeafSequenceType(), + MT=self._getMappingType() + )) + + def build_provided(P): + return P({ + IR0: 6, + IR1: 4, + }) -class CustomTypesBaseAdapterRegistryTests(BaseAdapterRegistryTests): - - class MyMapping(dict): - pass - class MySequence(list): - pass + self.assertEqual(registry._provided, + build_provided(P=self._getProvidedType())) + + registered = sorted(registry.allSubscriptions()) + self.assertEqual(registered, [ + ((), IR0, 'A1'), + ((), IR0, 'A2'), + ((IB0,), IR0, 'A1'), + ((IB0,), IR0, 'A2'), + ((IB0,), IR1, 'A3'), + ((IB0,), IR1, 'A4'), + ((IB0, IB1), IR0, 'A1'), + ((IB0, IB2), IR0, 'A2'), + ((IB0, IB2), IR1, 'A4'), + ((IB0, IB3), IR1, 'A3') + ]) + + # We can duplicate this to another object + registry2 = self._makeOne() + for args in registered: + registry2.subscribe(*args) + + self.assertEqual(registry2._subscribers, registry._subscribers) + self.assertEqual(registry2._provided, registry._provided) + + # We can change the types and rebuild the data structures. + registry._mappingType = CustomMapping + registry._leafSequenceType = CustomLeafSequence + registry._sequenceType = CustomSequence + registry._providedType = CustomProvided + def addValue(existing, new): + existing = existing if existing is not None else CustomLeafSequence() + existing.append(new) + return existing + registry._addValueToLeaf = addValue + + registry.rebuild() + + self.assertEqual(registry._subscribers, + build_subscribers( + L=CustomSequence, + F=CustomLeafSequence, + MT=CustomMapping + )) - class MyLeafSequence(list): - pass - class MyProvided(dict): - pass +class CustomTypesBaseAdapterRegistryTests(BaseAdapterRegistryTests): def _getMappingType(self): - return self.MyMapping + return CustomMapping def _getProvidedType(self): - return self.MyProvided + return CustomProvided def _getMutableListType(self): - return self.MySequence + return CustomSequence def _getLeafSequenceType(self): - return self.MyLeafSequence + return CustomLeafSequence def _getBaseAdapterRegistry(self): from zope.interface.adapter import BaseAdapterRegistry