diff options
author | Jason Madden <jamadden@gmail.com> | 2020-09-30 11:54:09 -0500 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-09-30 11:54:09 -0500 |
commit | 8d3c802ba585a68631742f80b8c1f51783b3b341 (patch) | |
tree | 51b270702e835b41d020e3f4bb923bd6b99ad44b | |
parent | 255db9d3c0ea7a5016e3c87644c23fca6eda9a7d (diff) | |
parent | 8cf31ebd6be86332860294cb541f8978d138d3b4 (diff) | |
download | zope-interface-8d3c802ba585a68631742f80b8c1f51783b3b341.tar.gz |
Merge pull request #217 from zopefoundation/issue216
C optimizations: Spec_clear and Spec_traverse need to include Spec->_bases
-rw-r--r-- | CHANGES.rst | 27 | ||||
-rw-r--r-- | docs/api/specifications.rst | 64 | ||||
-rw-r--r-- | src/zope/interface/_zope_interface_coptimizations.c | 2 | ||||
-rw-r--r-- | src/zope/interface/tests/test_declarations.py | 29 |
4 files changed, 122 insertions, 0 deletions
diff --git a/CHANGES.rst b/CHANGES.rst index a22876d..ab01fd6 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,6 +12,33 @@ that argument. See `issue 208 <https://github.com/zopefoundation/zope.interface/issues/208>`_. +- Fix a potential reference leak in the C optimizations. Previously, + applications that dynamically created unique ``Specification`` + objects (e.g., used ``@implementer`` on dynamic classes) could + notice a growth of small objects over time leading to increased + garbage collection times. See `issue 216 + <https://github.com/zopefoundation/zope.interface/issues/216>`_. + + .. caution:: + + This leak could prevent interfaces used as the bases of + other interfaces from being garbage collected. Those interfaces + will now be collected. + + One way in which this would manifest was that ``weakref.ref`` + objects (and things built upon them, like + ``Weak[Key|Value]Dictionary``) would continue to have access to + the original object even if there were no other visible + references to Python and the original object *should* have been + collected. This could be especially problematic for the + ``WeakKeyDictionary`` when combined with dynamic or local + (created in the scope of a function) interfaces, since interfaces + are hashed based just on their name and module name. See the + linked issue for an example of a resulting ``KeyError``. + + Note that such potential errors are not new, they are just once + again a possibility. + 5.1.0 (2020-04-08) ================== diff --git a/docs/api/specifications.rst b/docs/api/specifications.rst index 357e361..45cf5e1 100644 --- a/docs/api/specifications.rst +++ b/docs/api/specifications.rst @@ -161,6 +161,70 @@ Exmples for :meth:`.Specification.extends`: >>> I2.extends(I2, strict=False) True +Equality, Hashing, and Comparisons +---------------------------------- + +Specifications (including their notable subclass `Interface`), are +hashed and compared based solely on their ``__name__`` and +``__module__``, not including any information about their enclosing +scope, if any (e.g., their ``__qualname__``). This means that any two +objects created with the same name and module are considered equal and +map to the same value in a dictionary. + +.. doctest:: + + >>> from zope.interface import Interface + >>> class I1(Interface): pass + >>> orig_I1 = I1 + >>> class I1(Interface): pass + >>> I1 is orig_I1 + False + >>> I1 == orig_I1 + True + >>> d = {I1: 42} + >>> d[orig_I1] + 42 + >>> def make_nested(): + ... class I1(Interface): pass + ... return I1 + >>> nested_I1 = make_nested() + >>> I1 == orig_I1 == nested_I1 + True + +Because weak references hash the same as their underlying object, +this can lead to surprising results when weak references are involved, +especially if there are cycles involved or if the garbage collector is +not based on reference counting (e.g., PyPy). For example, if you +redefine an interface named the same as an interface being used in a +``WeakKeyDictionary``, you can get a ``KeyError``, even if you put the +new interface into the dictionary. + + +.. doctest:: + + >>> from zope.interface import Interface + >>> import gc + >>> from weakref import WeakKeyDictionary + >>> wr_dict = WeakKeyDictionary() + >>> class I1(Interface): pass + >>> wr_dict[I1] = 42 + >>> orig_I1 = I1 # Make sure it stays alive + >>> class I1(Interface): pass + >>> wr_dict[I1] = 2020 + >>> del orig_I1 + >>> _ = gc.collect() # Sometime later, gc runs and makes sure the original is gone + >>> wr_dict[I1] # Cleaning up the original weakref removed the new one + Traceback (most recent call last): + ... + KeyError: ... + +This is mostly likely a problem in test cases where it is tempting to +use the same named interfaces in different test methods. If references +to them escape, especially if they are used as the bases of other +interfaces, you may find surprising ``KeyError`` exceptions. For this +reason, it is best to use distinct names for local interfaces within +the same test module. + Interface ========= diff --git a/src/zope/interface/_zope_interface_coptimizations.c b/src/zope/interface/_zope_interface_coptimizations.c index 346e7f2..374311e 100644 --- a/src/zope/interface/_zope_interface_coptimizations.c +++ b/src/zope/interface/_zope_interface_coptimizations.c @@ -350,6 +350,7 @@ Spec_traverse(Spec* self, visitproc visit, void* arg) { Py_VISIT(self->_implied); Py_VISIT(self->_dependents); + Py_VISIT(self->_bases); Py_VISIT(self->_v_attrs); Py_VISIT(self->__iro__); Py_VISIT(self->__sro__); @@ -361,6 +362,7 @@ Spec_clear(Spec* self) { Py_CLEAR(self->_implied); Py_CLEAR(self->_dependents); + Py_CLEAR(self->_bases); Py_CLEAR(self->_v_attrs); Py_CLEAR(self->__iro__); Py_CLEAR(self->__sro__); diff --git a/src/zope/interface/tests/test_declarations.py b/src/zope/interface/tests/test_declarations.py index c8678b5..9ef192a 100644 --- a/src/zope/interface/tests/test_declarations.py +++ b/src/zope/interface/tests/test_declarations.py @@ -1078,6 +1078,35 @@ class Test_implementer(Test_classImplements): self.assertIsNone(spec.inherit,) self.assertIs(foo.__implemented__, spec) # pylint:disable=no-member + def test_does_not_leak_on_unique_classes(self): + # Make sure nothing is hanging on to the class or Implements + # object after they go out of scope. There was briefly a bug + # in 5.x that caused SpecificationBase._bases (in C) to not be + # traversed or cleared. + # https://github.com/zopefoundation/zope.interface/issues/216 + import gc + from zope.interface.interface import InterfaceClass + IFoo = InterfaceClass('IFoo') + + begin_count = len(gc.get_objects()) + + for _ in range(1900): + class TestClass(object): + pass + + self._callFUT(TestClass, IFoo) + + gc.collect() + + end_count = len(gc.get_objects()) + + # How many new objects might still be around? In all currently + # tested interpreters, there aren't any, so our counts should + # match exactly. When the bug existed, in a steady state, the loop + # would grow by two objects each iteration + fudge_factor = 0 + self.assertLessEqual(end_count, begin_count + fudge_factor) + class Test_implementer_only(Test_classImplementsOnly): |