Merge lp:~therve/storm/reference-changed-leak into lp:storm
Proposed by
Thomas Herve
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | James Henstridge | ||||
Approved revision: | not available | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~therve/storm/reference-changed-leak | ||||
Merge into: | lp:storm | ||||
Diff against target: |
142 lines (+50/-10) 2 files modified
storm/references.py (+15/-8) tests/store/base.py (+35/-2) |
||||
To merge this branch: | bzr merge lp:~therve/storm/reference-changed-leak | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
James Henstridge | Approve | ||
Jamu Kakar (community) | Approve | ||
Review via email: mp+14480@code.launchpad.net |
To post a comment you must log in.
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.