Merge lp:~therve/storm/reference-changed-leak into lp:storm

[Thomas Herve]

[lp:~therve/storm/reference-changed-leak]

340. By Thomas Herve on 2009-11-12

Fix the problem using a weak reference, and test the actual problem observerd

[James Henstridge (jamesh)]

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.

[lp:~therve/storm/reference-changed-leak]

341. By Thomas Herve on 2009-11-20

Use the same trick for "removed" event

[Thomas Herve (therve)]

Thanks a lot for the review.

> [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?

Actually, I think it relies on the fact they have the same hash and are equals, which is made explicit in the documentation.

> [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.

> [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.

[lp:~therve/storm/reference-changed-leak]

342. By Thomas Herve on 2009-11-20

Explicit None check

[Jamu Kakar (jkakar)]

Looks good, tests pass here.

review: Approve

[James Henstridge (jamesh)]

> 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

=== modified file 'storm/references.py'
--- storm/references.py	2009-02-19 16:44:33 +0000
+++ storm/references.py	2009-11-20 20:56:10 +0000
@@ -18,6 +18,8 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
+import weakref
+
 from storm.exceptions import WrongStoreError, NoStoreError, ClassInfoError
 from storm.store import Store, get_where_for_args
 from storm.variables import LazyValue
@@ -620,7 +622,7 @@
                 #local_info.event.hook("removed", self._break_on_local_removed,
                 #                      remote_info)
                 remote_info.event.hook("removed", self._break_on_remote_removed,
-                                       local_info)
+                                       weakref.ref(local_info))
             else:
                 remote_has_changed = False
                 for local_column, remote_column in pairs:
@@ -649,10 +651,10 @@
             local_info.event.hook("changed", self._break_on_local_diverged,
                                   remote_info)
             remote_info.event.hook("changed", self._break_on_remote_diverged,
-                                   local_info)
+                                   weakref.ref(local_info))
             if self.on_remote:
                 remote_info.event.hook("removed", self._break_on_remote_removed,
-                                       local_info)
+                                       weakref.ref(local_info))
 
     def unlink(self, local_info, remote_info, setting=False):
         """Break the relation between the local and remote objects.
@@ -684,11 +686,11 @@
             remote_info.event.unhook("changed", self._track_remote_changes,
                                      local_info)
             remote_info.event.unhook("changed", self._break_on_remote_diverged,
-                                     local_info)
+                                     weakref.ref(local_info))
             remote_info.event.unhook("flushed", self._break_on_remote_flushed,
                                      local_info)
             remote_info.event.unhook("removed", self._break_on_remote_removed,
-                                     local_info)
+                                     weakref.ref(local_info))
 
             if local_store is None:
                 if not self.many or not remote_infos:
@@ -781,13 +783,16 @@
                 self.unlink(local_info, remote_info)
 
     def _break_on_remote_diverged(self, remote_info, remote_variable,
-                                  old_value, new_value, fromdb, local_info):
+                                  old_value, new_value, fromdb, local_info_ref):
         """Break the remote/local relationship on diverging changes.
 
         This hook ensures that if the remote object has an attribute
         changed by hand in a way that diverges from the local object,
         the relationship is undone.
         """
+        local_info = local_info_ref()
+        if local_info is None:
+            return
         local_column = self._get_local_column(local_info.cls_info.cls,
                                               remote_variable.column)
         if local_column is not None:
@@ -803,9 +808,11 @@
         """Break the remote/local relationship on flush."""
         self.unlink(local_info, remote_info)
 
-    def _break_on_remote_removed(self, remote_info, local_info):
+    def _break_on_remote_removed(self, remote_info, local_info_ref):
         """Break the remote relationship when the remote object is removed."""
-        self.unlink(local_info, remote_info)
+        local_info = local_info_ref()
+        if local_info is not None:
+            self.unlink(local_info, remote_info)
 
     def _add_all(self, obj_info, local_info):
         store = Store.of(obj_info)
 
=== modified file 'tests/store/base.py'
--- tests/store/base.py	2009-10-22 02:30:20 +0000
+++ tests/store/base.py	2009-11-20 20:56:10 +0000
@@ -2605,6 +2605,39 @@
         bar.foo_id = SQL("20")
         self.assertEquals(bar.foo.id, 20)
 
+    def test_reference_remote_leak_on_flush_with_changed(self):
+        """
+        "changed" events only hold weak references to remote infos object, thus
+        not creating a leak when unhooked.
+        """
+        self.get_cache(self.store).set_size(0)
+        bar = self.store.get(Bar, 100)
+        bar.foo.title = u"Changed title"
+        bar_ref = weakref.ref(get_obj_info(bar))
+        foo = bar.foo
+        del bar
+        self.store.flush()
+        gc.collect()
+        self.assertEquals(bar_ref(), None)
+
+    def test_reference_remote_leak_on_flush_with_removed(self):
+        """
+        "removed" events only hold weak references to remote infos objects,
+        thus not creating a leak when unhooked.
+        """
+        self.get_cache(self.store).set_size(0)
+        class MyFoo(Foo):
+            bar = Reference(Foo.id, Bar.foo_id, on_remote=True)
+
+        foo = self.store.get(MyFoo, 10)
+        foo.bar.title = u"Changed title"
+        foo_ref = weakref.ref(get_obj_info(foo))
+        bar = foo.bar
+        del foo
+        self.store.flush()
+        gc.collect()
+        self.assertEquals(foo_ref(), None)
+
     def test_reference_break_on_remote_diverged_by_lazy(self):
         class MyBar(Bar):
             pass
@@ -4484,7 +4517,7 @@
         self.store.invalidate()
 
         obj_info = get_obj_info(pickle_blob)
-        variable =  obj_info.variables[PickleBlob.bin]
+        variable = obj_info.variables[PickleBlob.bin]
         var_ref = weakref.ref(variable)
         del variable, blob, pickle_blob, obj_info
         gc.collect()
@@ -4523,7 +4556,7 @@
         self.store.invalidate()
 
         obj_info = get_obj_info(pickle_blob)
-        variable =  obj_info.variables[PickleBlob.bin]
+        variable = obj_info.variables[PickleBlob.bin]
         var_ref = weakref.ref(variable)
         del variable, blob, pickle_blob, obj_info, foo
         gc.collect()