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
1=== modified file 'storm/cextensions.c'
2--- storm/cextensions.c 2012-05-31 04:14:32 +0000
3+++ storm/cextensions.c 2019-04-09 11:38:33 +0000
4@@ -112,6 +112,7 @@
5
6 typedef struct {
7 PyObject_HEAD
8+ PyObject *__weakreflist;
9 PyObject *_owner_ref;
10 PyObject *_hooks;
11 } EventSystemObject;
12@@ -267,6 +268,8 @@
13 if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", kwlist, &owner))
14 return -1;
15
16+ self->__weakreflist = NULL;
17+
18 /* self._owner_ref = weakref.ref(owner) */
19 self->_owner_ref = PyWeakref_NewRef(owner, NULL);
20 if (self->_owner_ref) {
21@@ -291,6 +294,8 @@
22 static int
23 EventSystem_clear(EventSystemObject *self)
24 {
25+ if (self->__weakreflist)
26+ PyObject_ClearWeakRefs((PyObject *)self);
27 Py_CLEAR(self->_owner_ref);
28 Py_CLEAR(self->_hooks);
29 return 0;
30@@ -547,7 +552,7 @@
31 (traverseproc)EventSystem_traverse, /*tp_traverse*/
32 (inquiry)EventSystem_clear, /*tp_clear*/
33 0, /*tp_richcompare*/
34- 0, /*tp_weaklistoffset*/
35+ offsetof(EventSystemObject, __weakreflist), /*tp_weaklistoffset*/
36 0, /*tp_iter*/
37 0, /*tp_iternext*/
38 EventSystem_methods, /*tp_methods*/
39@@ -662,10 +667,18 @@
40 Py_INCREF(column);
41 self->column = column;
42
43- /* self.event = event */
44+ /* self.event = weakref.proxy(event) if event is not None else None */
45 Py_DECREF(self->event);
46- Py_INCREF(event);
47- self->event = event;
48+ if (event != Py_None) {
49+ PyObject *event_proxy = PyWeakref_NewProxy(event, NULL);
50+ if (event_proxy)
51+ self->event = event_proxy;
52+ else
53+ goto error;
54+ } else {
55+ Py_INCREF(Py_None);
56+ self->event = Py_None;
57+ }
58
59 return 0;
60
61
62=== modified file 'storm/variables.py'
63--- storm/variables.py 2011-09-13 10:13:46 +0000
64+++ storm/variables.py 2019-04-09 11:38:33 +0000
65@@ -26,6 +26,7 @@
66 import uuid
67 except ImportError:
68 uuid = None
69+import weakref
70
71 from storm.compat import json
72 from storm.exceptions import NoneError
73@@ -153,7 +154,7 @@
74 self._validator_object_factory = validator_object_factory
75 self._validator_attribute = validator_attribute
76 self.column = column
77- self.event = event
78+ self.event = weakref.proxy(event) if event is not None else None
79
80 def get_lazy(self, default=None):
81 """Get the current L{LazyValue} without resolving its value.
82@@ -557,7 +558,7 @@
83 self.event.hook("object-deleted", self._detect_changes_and_stop)
84
85 def _start_tracking(self, obj_info, event_system):
86- self._event_system = event_system
87+ self._event_system = weakref.proxy(event_system)
88 self.event.hook("stop-tracking-changes", self._stop_tracking)
89
90 def _stop_tracking(self, obj_info, event_system):
91
92=== modified file 'tests/store/base.py'
93--- tests/store/base.py 2016-05-13 18:55:24 +0000
94+++ tests/store/base.py 2019-04-09 11:38:33 +0000
95@@ -488,6 +488,35 @@
96 blob = self.store.find(PickleBlob, PickleBlob.id == 4000).one()
97 self.assertEquals(blob.bin, {"k1": "v1", "k": "v"})
98
99+ def test_mutable_variable_no_reference_cycle(self):
100+ """
101+ Mutable variables only hold weak refs to EventSystem, to prevent
102+ leaks.
103+ """
104+ class PickleBlob(Blob):
105+ bin = Pickle()
106+
107+ blob = self.store.get(Blob, 20)
108+ blob.bin = "\x80\x02}q\x01U\x01aK\x01s."
109+ self.store.flush()
110+ del blob
111+
112+ # Get an existing object and make an unflushed change to it so that
113+ # a flush hook for the variable is registered with the event system.
114+ pickle_blob = self.store.get(PickleBlob, 20)
115+ pickle_blob.bin = "foobin"
116+ pickle_blob_ref = weakref.ref(pickle_blob)
117+ del pickle_blob
118+
119+ for store in self.stores:
120+ store.close()
121+ del store
122+ self.store = None
123+ self.stores = []
124+ gc.collect()
125+
126+ self.assertIsNone(pickle_blob_ref())
127+
128 def test_wb_checkpoint_doesnt_override_changed(self):
129 """
130 This test ensures that we don't uselessly checkpoint when getting back
131
132=== modified file 'tests/variables.py'
133--- tests/variables.py 2011-09-13 22:48:50 +0000
134+++ tests/variables.py 2019-04-09 11:38:33 +0000
135@@ -85,7 +85,7 @@
136
137 def test_constructor_event(self):
138 variable = CustomVariable(event=marker)
139- self.assertEquals(variable.event, marker)
140+ self.assertEquals(variable.event, weakref.proxy(marker))
141
142 def test_get_default(self):
143 variable = CustomVariable()

Subscribers

People subscribed via source and target branches

to status/vote changes: