Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free HasTraits subclasses from hashing/comparing by identity #410

Merged
merged 2 commits into from
Oct 24, 2018

Conversation

rkern
Copy link
Member

@rkern rkern commented Oct 23, 2018

The Traits listener machinery uses weak-key dicts to track some of the bookkeeping for attaching and unattaching dynamic listeners. The intention of the code assumed that HasTraits instances kept the Python 2 default __hash__ and __eq__ semantics, which are based on identity, not value. Python 3 has changed those defaults, and there are some places where we do modify __eq__ to do value comparisons (e.g. codetools.contexts).

Here, I introduce a new WeakIDKeyDict that mostly works like a weakref.WeakKeyDictionary, but the key objects are hashed and compared only by their identity and entirely ignore the __hash__ and __eq__ implementations on the keys.

This also replaces a usage of WeakKeyDictionary in the adaptation framework, which has similar issues, though that part of the framework is deprecated in any case.

@rkern rkern requested a review from mdickinson October 23, 2018 06:05
@mdickinson
Copy link
Member

LGTM at a glance; I'd like to try to find time later to look at it more closely, but not sure when that later might be, so approving anyway for now.

Use of weakref callbacks always worries me, but I don't see an alternative here, and I can't think of any specific situations where they'd pose an issue in this case. The callback itself looks thread safe, so we're okay there. I'm trying to figure out whether there's any possibility of id_dict.data not being defined when the callback is run; that could possibly happen at interpreter shutdown time when things are being collected in uncontrolled orders, but then the exception would get ignored and we don't care. On Python 2, the callback might never get run at all in the presence of awkward cycles, but again I can't see why that would matter (on Python 3, we should be safer in that respect). All that really matters is that the callback does get reliably run whenever an object genuinely goes out of scope, so that we don't end up with bogus ids in the dict.

Sorry, I know you know all this already; I'm mostly just trying to reason it out to myself.

In the WeakIDDict.__iter__ method, what happens if the self.data dictionary changes size during the iteration (due to a weakref callback running at an unexpected moment)? Do we care? Are there any places in the Traits code where we're iterating over a WeakIDKeyDict?

@rkern
Copy link
Member Author

rkern commented Oct 23, 2018

As far as I can tell, we only call __contains__, __getitem__, __setitem__ and pop(); no iteration.

@rkern
Copy link
Member Author

rkern commented Oct 23, 2018

Indeed, WeakKeyDictionary has extra guard logic for handling iteration. I could add similar logic, or just document the intended use.

@mdickinson
Copy link
Member

just document the intended use.

Sounds fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants