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

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

> Thanks a lot for the review.
>
> > [1]
> Actually, I think it relies on the fact they have the same hash and are
> equals, which is made explicit in the documentation.

Gar. I missed that in the docs. I noticed that weakref.ref() returned the same reference object for an instance and was wondering if that was what was causing equality and hashing to work. Never mind that issue then.

> > [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?
>
> Nice catch, I fixed it.

Fix looks good.

> > [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.
>
> The thing is that we want (for now) to keep the objects alive if they have a
> direct reference. So, no, I don't think it's a problem, because the objects
> will stay alive only if at least one of both objects are referenced directly.

Okay. If it is an issue, we can look at it later. This patch certainly won't introduce a problem of this type.

review: Approve

« Back to merge proposal