Merge lp:~cjwatson/storm/more-weakrefs into lp:storm

[Colin Watson]

Use weak references from Variable and MutableValueVariable to EventSystem, to avoid GC cycles.

Launchpad's test suite has had occasional problems for years with PostgresConnection leaks (Zope's test runner is careful about checking for GC garbage), but a combination of randomised test order and the fact that it often took hours to reach the failure on development systems even when we did manage to reproduce it meant that we had to work around it rather than fixing it properly; we've found that the problems are generally around dealing with MutableValueVariable instances. A couple of days ago, by chance, I ran across a much smaller subset of the Launchpad test suite that reproduced this bug, and so I was able to figure out a proper fix in Storm.

The problem was that, when change tracking was enabled on a MutableValueVariable, the variable became part of a reference cycle with the store via the event system. Using weak references for the event system appears to fix this, and I've been able to do a full successful Launchpad test run with this patch and with some of our workarounds reverted.

[lp:~cjwatson/storm/more-weakrefs]

486. By Colin Watson on 2019-04-08

Simplify C extension changes.

[Tony Simpson (tonysimpson)]

Looks good.

C changes look good.

Test looks a bit long but I understand it follows convention with other tests. I would add a comment as to why there needs to be an existing object in the store to get the behaviour.

review: Approve

[lp:~cjwatson/storm/more-weakrefs]

487. By Colin Watson on 2019-04-09

Explain test a little more.

[Adam Collard (adam-collard)]

review: Approve

=== modified file 'storm/cextensions.c'
--- storm/cextensions.c	2012-05-31 04:14:32 +0000
+++ storm/cextensions.c	2019-04-09 11:38:33 +0000
@@ -112,6 +112,7 @@
 
 typedef struct {
     PyObject_HEAD
+    PyObject *__weakreflist;
     PyObject *_owner_ref;
     PyObject *_hooks;
 } EventSystemObject;
@@ -267,6 +268,8 @@
     if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", kwlist, &owner))
         return -1;
 
+    self->__weakreflist = NULL;
+
     /* self._owner_ref = weakref.ref(owner) */
     self->_owner_ref = PyWeakref_NewRef(owner, NULL);
     if (self->_owner_ref) {
@@ -291,6 +294,8 @@
 static int
 EventSystem_clear(EventSystemObject *self)
 {
+    if (self->__weakreflist)
+        PyObject_ClearWeakRefs((PyObject *)self);
     Py_CLEAR(self->_owner_ref);
     Py_CLEAR(self->_hooks);
     return 0;
@@ -547,7 +552,7 @@
     (traverseproc)EventSystem_traverse,  /*tp_traverse*/
     (inquiry)EventSystem_clear,          /*tp_clear*/
     0,                      /*tp_richcompare*/
-    0,                      /*tp_weaklistoffset*/
+    offsetof(EventSystemObject, __weakreflist), /*tp_weaklistoffset*/
     0,                      /*tp_iter*/
     0,                      /*tp_iternext*/
     EventSystem_methods,        /*tp_methods*/
@@ -662,10 +667,18 @@
     Py_INCREF(column);
     self->column = column;
 
-    /* self.event = event */
+    /* self.event = weakref.proxy(event) if event is not None else None */
     Py_DECREF(self->event);
-    Py_INCREF(event);
-    self->event = event;
+    if (event != Py_None) {
+        PyObject *event_proxy = PyWeakref_NewProxy(event, NULL);
+        if (event_proxy)
+            self->event = event_proxy;
+        else
+            goto error;
+    } else {
+        Py_INCREF(Py_None);
+        self->event = Py_None;
+    }
 
     return 0;
 

=== modified file 'storm/variables.py'
--- storm/variables.py	2011-09-13 10:13:46 +0000
+++ storm/variables.py	2019-04-09 11:38:33 +0000
@@ -26,6 +26,7 @@
     import uuid
 except ImportError:
     uuid = None
+import weakref
 
 from storm.compat import json
 from storm.exceptions import NoneError
@@ -153,7 +154,7 @@
             self._validator_object_factory = validator_object_factory
             self._validator_attribute = validator_attribute
         self.column = column
-        self.event = event
+        self.event = weakref.proxy(event) if event is not None else None
 
     def get_lazy(self, default=None):
         """Get the current L{LazyValue} without resolving its value.
@@ -557,7 +558,7 @@
             self.event.hook("object-deleted", self._detect_changes_and_stop)
 
     def _start_tracking(self, obj_info, event_system):
-        self._event_system = event_system
+        self._event_system = weakref.proxy(event_system)
         self.event.hook("stop-tracking-changes", self._stop_tracking)
 
     def _stop_tracking(self, obj_info, event_system):

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2016-05-13 18:55:24 +0000
+++ tests/store/base.py	2019-04-09 11:38:33 +0000
@@ -488,6 +488,35 @@
         blob = self.store.find(PickleBlob, PickleBlob.id == 4000).one()
         self.assertEquals(blob.bin, {"k1": "v1", "k": "v"})
 
+    def test_mutable_variable_no_reference_cycle(self):
+        """
+        Mutable variables only hold weak refs to EventSystem, to prevent
+        leaks.
+        """
+        class PickleBlob(Blob):
+            bin = Pickle()
+
+        blob = self.store.get(Blob, 20)
+        blob.bin = "\x80\x02}q\x01U\x01aK\x01s."
+        self.store.flush()
+        del blob
+
+        # Get an existing object and make an unflushed change to it so that
+        # a flush hook for the variable is registered with the event system.
+        pickle_blob = self.store.get(PickleBlob, 20)
+        pickle_blob.bin = "foobin"
+        pickle_blob_ref = weakref.ref(pickle_blob)
+        del pickle_blob
+
+        for store in self.stores:
+            store.close()
+        del store
+        self.store = None
+        self.stores = []
+        gc.collect()
+
+        self.assertIsNone(pickle_blob_ref())
+
     def test_wb_checkpoint_doesnt_override_changed(self):
         """
         This test ensures that we don't uselessly checkpoint when getting back

=== modified file 'tests/variables.py'
--- tests/variables.py	2011-09-13 22:48:50 +0000
+++ tests/variables.py	2019-04-09 11:38:33 +0000
@@ -85,7 +85,7 @@
 
     def test_constructor_event(self):
         variable = CustomVariable(event=marker)
-        self.assertEquals(variable.event, marker)
+        self.assertEquals(variable.event, weakref.proxy(marker))
 
     def test_get_default(self):
         variable = CustomVariable()