Merge lp:~therve/storm/variable-referenceset-leak into lp:storm

[Thomas Herve]

[Thomas Herve (therve)]

This branch eliminates a leak linked to MutableVariable hook to the store event system: it reverts previous attempt using hook to object-deleted, and instead rely on the store invalidating the object.

[lp:~therve/storm/variable-referenceset-leak]

274. By Thomas Herve on 2008-10-20

No need to re-hook object-deleted

275. By Thomas Herve on 2008-10-20

DIrectly call hook on flush, because we may have already missed
start-tracking-changes the second time.

[Gustavo Niemeyer (niemeyer)]

Only one question:

[1]

I wonder if hooking on flush could happen on an overloaded get() method, instead of the general _start_tracking one. This would mean that if that specific property isn't read or set, the flush event won't even touch this variable.

It looks good in any case!

review: Approve

[Thomas Herve (therve)]

Thanks, done!

[lp:~therve/storm/variable-referenceset-leak]

276. By Thomas Herve on 2008-10-22

Remove hook to flush in start-tracking, and do it in get() instead

[Gustavo Niemeyer (niemeyer)]

A couple of minor details on the last change:

[2]

+ if isinstance(value, LazyValue) and self._event_system is not None:
+ self._event_system.unhook("flush", self._detect_changes)

Would you mind to invert the test above, since the later part is cheaper than the first one?

This is definitely not going to make any visible difference, but since it's so easy to do should be cool.

[3]

+ #elif self._event_system is not None:
+ # self._event_system.hook("flush", self._detect_changes)

Isn't this part necessary? I guess it's a contrieved example, but since it's easy to support we might as well do it.

E.g. something like:

map = {"foo": "bar"}
obj.attr = map
store.flush()
map["bar"] = "baz"
store.flush()

If it is not necessary, we should just remove the commented code. If it is necessary, the conditionals could be organized such as the following to prevent the double test on event system:

if <event system>:
    if <value is lazy>:
    else:

[lp:~therve/storm/variable-referenceset-leak]

277. By Thomas Herve on 2008-10-23

Change the conditionals order, make the test on self._event_system only once.

[Thomas Herve (therve)]

I made the change, but I wonder where you saw commented code? It's not commented in the current version of the branch at least (and it's necessary to make the tests pass).

[Gustavo Niemeyer (niemeyer)]

Sorry, I must have done something bad locally then. Thanks for the change.

[Gustavo Niemeyer (niemeyer)]

Sorry, I must have done something bad locally then. Thanks for the change.

review: Approve

[Jamu Kakar (jkakar)]

Looks great, +1.

review: Approve