Simply the caching of utility-registration data. In addition to #84
Conversation
simplification, avoids spurious test failures when checking for leaks in tests with persistent registries.
There was a problem hiding this comment.
If the persistent registries in the connection cache aren't having __init__ called on them, then it's not clear to me how moving from a weakref to an instance attribute completely solves the problem.
I suppose its something to do with ghosting of objects, but that seems somewhat fragile to rely on. (It seems the only way it would have been "leaking" before was if the Components object was ghosted, but its .adapters and .utilities weren't.)
An alternative would be to keep the weakref solution and add a zope.testing cleanup that just resets _UtilityRegistrations. (Although I do agree, it's nicer having the Components object manage the cache lifetime by itself.)
CHANGES.rst
Outdated
| @@ -4,6 +4,10 @@ Changes | |||
| 4.4.1 (unreleased) | |||
| ------------------ | |||
|
|
|||
| - Simply the caching of utility-registration data. In addition to | |||
There was a problem hiding this comment.
Gaaa, the spelling checker didn't catch that. Thanks.
src/zope/interface/registry.py
Outdated
| @@ -184,6 +145,9 @@ def __init__(self, name='', bases=()): | |||
| self._init_registrations() | |||
| self.__bases__ = tuple(bases) | |||
|
|
|||
| if hasattr(self, '_v_utility_registrations_cache'): | |||
| del self._v_utility_registrations_cache | |||
There was a problem hiding this comment.
As a reader of this code, I see this and I think, "Why is that being done in __init__? There should be no possible way this object can have that attribute, it isn't set anywhere yet." The secret, of course, is that somewhere else---not even in this project!---there's code that registers a cleanup with zope.testing to call __init__. That's pretty non-obvious; most people don't consider __init__ being called on existing objects multiple times to be "normal". How about just setting that attribute to None? (I know, that voids the AttributeError optimization in the property...but maybe that optimization is premature.) Or have the property have a setter that does the deletion; that makes a certain sense from an internal-logic standpoint.
There was a problem hiding this comment.
I added a comment to address this. It would be good for someone to make the testing interactions across the packages cleaner. That is out of scope of this change.
With the comment, at least there's now something describing the current dual responsibilities of init.
src/zope/interface/registry.py
Outdated
| @@ -184,6 +145,9 @@ def __init__(self, name='', bases=()): | |||
| self._init_registrations() | |||
| self.__bases__ = tuple(bases) | |||
|
|
|||
| if hasattr(self, '_v_utility_registrations_cache'): | |||
There was a problem hiding this comment.
Components is not a subclass of persistent so there's no guarantee that _v attributes will not be written out in pickles. That seems...suboptimal? I know that there are other subclasses of Components besides the ones defined in zope.component (for example, pyramid has class Registry(Components, dict), so just overriding __getstate__ to return self.__dict__ with _v_whatever popped won't work).
I was strongly motivated to keep that from happening in the original PR, although I can't really recall why at this moment. Perhaps this isn't a problem in practice, but it seems worth mentioning.
There was a problem hiding this comment.
I really don't think people pickling these things outside the realm of ZODB is likely. Even if they do, there is no harm, just some extra data.
There was a problem hiding this comment.
OK. If I ever figure out what I was so worried about the first time around we can always come back to this.
|
On Fri, May 12, 2017 at 5:47 PM, Jason Madden ***@***.***> wrote:
***@***.**** commented on this pull request.
If the persistent registries in the connection cache aren't having
__init__ called on them, then it's not clear to me how moving from a
weakref to an instance attribute completely solves the problem.
I suppose its something to do with ghosting of objects, but that seems
somewhat fragile to rely on. (It seems the only way it would have been
"leaking" before was if the Components object was ghosted, but its
.adapters and .utilities weren't.)
The persistent objects are probably being reaped by clearing the cache. If
they weren't, then *they* would be garbage. With this change the cache
isn't introducing a leak where there wasn't one before.
|
But add testing to be sure it doesn't break again, and extra suspenders to ensure the issues that #84 was trying to deal with doesn't show up either.
But add testing to be sure it doesn't break again, and extra suspenders to ensure the issues that #84 was trying to deal with doesn't show up either.
But add testing to be sure it doesn't break again, and extra suspenders to ensure the issues that #84 was trying to deal with doesn't show up either.
simplification, avoids spurious test failures when checking for
leaks in tests with persistent registries.
This fixes #83