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

Proposed by Colin Watson on 2019-04-07
Status: Merged
Approved by: Adam Collard on 2019-04-09
Approved revision: 487
Merged at revision: 485
Proposed branch: lp:~cjwatson/storm/more-weakrefs
Merge into: lp:storm
Diff against target: 143 lines (+50/-7)
4 files modified
storm/cextensions.c (+17/-4)
storm/variables.py (+3/-2)
tests/store/base.py (+29/-0)
tests/variables.py (+1/-1)
To merge this branch: bzr merge lp:~cjwatson/storm/more-weakrefs
Reviewer Review Type Date Requested Status
Adam Collard (community) 2019-04-07 Approve on 2019-04-09
Tony Simpson (community) Approve on 2019-04-09
Review via email: mp+365639@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
lp:~cjwatson/storm/more-weakrefs updated on 2019-04-08
486. By Colin Watson on 2019-04-08

Simplify C extension changes.

Tony Simpson (tonysimpson) wrote :

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 updated on 2019-04-09
487. By Colin Watson on 2019-04-09

Explain test a little more.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
112112
113typedef struct {113typedef struct {
114 PyObject_HEAD114 PyObject_HEAD
115 PyObject *__weakreflist;
115 PyObject *_owner_ref;116 PyObject *_owner_ref;
116 PyObject *_hooks;117 PyObject *_hooks;
117} EventSystemObject;118} EventSystemObject;
@@ -267,6 +268,8 @@
267 if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", kwlist, &owner))268 if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", kwlist, &owner))
268 return -1;269 return -1;
269270
271 self->__weakreflist = NULL;
272
270 /* self._owner_ref = weakref.ref(owner) */273 /* self._owner_ref = weakref.ref(owner) */
271 self->_owner_ref = PyWeakref_NewRef(owner, NULL);274 self->_owner_ref = PyWeakref_NewRef(owner, NULL);
272 if (self->_owner_ref) {275 if (self->_owner_ref) {
@@ -291,6 +294,8 @@
291static int294static int
292EventSystem_clear(EventSystemObject *self)295EventSystem_clear(EventSystemObject *self)
293{296{
297 if (self->__weakreflist)
298 PyObject_ClearWeakRefs((PyObject *)self);
294 Py_CLEAR(self->_owner_ref);299 Py_CLEAR(self->_owner_ref);
295 Py_CLEAR(self->_hooks);300 Py_CLEAR(self->_hooks);
296 return 0;301 return 0;
@@ -547,7 +552,7 @@
547 (traverseproc)EventSystem_traverse, /*tp_traverse*/552 (traverseproc)EventSystem_traverse, /*tp_traverse*/
548 (inquiry)EventSystem_clear, /*tp_clear*/553 (inquiry)EventSystem_clear, /*tp_clear*/
549 0, /*tp_richcompare*/554 0, /*tp_richcompare*/
550 0, /*tp_weaklistoffset*/555 offsetof(EventSystemObject, __weakreflist), /*tp_weaklistoffset*/
551 0, /*tp_iter*/556 0, /*tp_iter*/
552 0, /*tp_iternext*/557 0, /*tp_iternext*/
553 EventSystem_methods, /*tp_methods*/558 EventSystem_methods, /*tp_methods*/
@@ -662,10 +667,18 @@
662 Py_INCREF(column);667 Py_INCREF(column);
663 self->column = column;668 self->column = column;
664669
665 /* self.event = event */670 /* self.event = weakref.proxy(event) if event is not None else None */
666 Py_DECREF(self->event);671 Py_DECREF(self->event);
667 Py_INCREF(event);672 if (event != Py_None) {
668 self->event = event;673 PyObject *event_proxy = PyWeakref_NewProxy(event, NULL);
674 if (event_proxy)
675 self->event = event_proxy;
676 else
677 goto error;
678 } else {
679 Py_INCREF(Py_None);
680 self->event = Py_None;
681 }
669682
670 return 0;683 return 0;
671684
672685
=== 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 @@
26 import uuid26 import uuid
27except ImportError:27except ImportError:
28 uuid = None28 uuid = None
29import weakref
2930
30from storm.compat import json31from storm.compat import json
31from storm.exceptions import NoneError32from storm.exceptions import NoneError
@@ -153,7 +154,7 @@
153 self._validator_object_factory = validator_object_factory154 self._validator_object_factory = validator_object_factory
154 self._validator_attribute = validator_attribute155 self._validator_attribute = validator_attribute
155 self.column = column156 self.column = column
156 self.event = event157 self.event = weakref.proxy(event) if event is not None else None
157158
158 def get_lazy(self, default=None):159 def get_lazy(self, default=None):
159 """Get the current L{LazyValue} without resolving its value.160 """Get the current L{LazyValue} without resolving its value.
@@ -557,7 +558,7 @@
557 self.event.hook("object-deleted", self._detect_changes_and_stop)558 self.event.hook("object-deleted", self._detect_changes_and_stop)
558559
559 def _start_tracking(self, obj_info, event_system):560 def _start_tracking(self, obj_info, event_system):
560 self._event_system = event_system561 self._event_system = weakref.proxy(event_system)
561 self.event.hook("stop-tracking-changes", self._stop_tracking)562 self.event.hook("stop-tracking-changes", self._stop_tracking)
562563
563 def _stop_tracking(self, obj_info, event_system):564 def _stop_tracking(self, obj_info, event_system):
564565
=== 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 @@
488 blob = self.store.find(PickleBlob, PickleBlob.id == 4000).one()488 blob = self.store.find(PickleBlob, PickleBlob.id == 4000).one()
489 self.assertEquals(blob.bin, {"k1": "v1", "k": "v"})489 self.assertEquals(blob.bin, {"k1": "v1", "k": "v"})
490490
491 def test_mutable_variable_no_reference_cycle(self):
492 """
493 Mutable variables only hold weak refs to EventSystem, to prevent
494 leaks.
495 """
496 class PickleBlob(Blob):
497 bin = Pickle()
498
499 blob = self.store.get(Blob, 20)
500 blob.bin = "\x80\x02}q\x01U\x01aK\x01s."
501 self.store.flush()
502 del blob
503
504 # Get an existing object and make an unflushed change to it so that
505 # a flush hook for the variable is registered with the event system.
506 pickle_blob = self.store.get(PickleBlob, 20)
507 pickle_blob.bin = "foobin"
508 pickle_blob_ref = weakref.ref(pickle_blob)
509 del pickle_blob
510
511 for store in self.stores:
512 store.close()
513 del store
514 self.store = None
515 self.stores = []
516 gc.collect()
517
518 self.assertIsNone(pickle_blob_ref())
519
491 def test_wb_checkpoint_doesnt_override_changed(self):520 def test_wb_checkpoint_doesnt_override_changed(self):
492 """521 """
493 This test ensures that we don't uselessly checkpoint when getting back522 This test ensures that we don't uselessly checkpoint when getting back
494523
=== 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 @@
8585
86 def test_constructor_event(self):86 def test_constructor_event(self):
87 variable = CustomVariable(event=marker)87 variable = CustomVariable(event=marker)
88 self.assertEquals(variable.event, marker)88 self.assertEquals(variable.event, weakref.proxy(marker))
8989
90 def test_get_default(self):90 def test_get_default(self):
91 variable = CustomVariable()91 variable = CustomVariable()

Subscribers

People subscribed via source and target branches

to status/vote changes: