Code review comment for lp:~therve/storm/reference-changed-leak

Revision history for this message
James Henstridge (jamesh) wrote :

This looks pretty good. I just have a few comments:

[1]
This seems to be relying on the fact that weakref.ref() always returns the same reference object given the same input. Looking at the weakref.__new__ implementation this is indeed true, but I don't see it spelled out in the Python documentation.

The source code makes it sound like an optimisation rather than a language feature, so I wonder if it is a good idea to rely on it?

[2]
For on_remote references, an additional "removed" event handler is hooked in the remote -> local direction. Won't this cause the same circular reference problem that the remote -> local "changed" handler does?

[3]
If we have references on both the local and remote objects (one forward, one backward) and access them both, will that generate the same kind of cycle? While each ref in isolation doesn't form a cycle, as a pair they do.

« Back to merge proposal