8000 Issue #28427: old keys should not remove new values from · python/cpython@e10ca3a · GitHub
[go: up one dir, main page]
Skip to content

Commit e10ca3a

Browse files
pitroupitrou
committed
Issue #28427: old keys should not remove new values from
WeakValueDictionary when collecting from another thread.
1 parent 1fee515 commit e10ca3a

File tree

7 files changed

+174
-30
lines changed

7 files changed

+174
-30
lines changed

‎Include/dictobject.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,8 @@ PyAPI_FUNC(int) PyDict_DelItem(PyObject *mp, PyObject *key);
7575
#ifndef Py_LIMITED_API
7676
PyAPI_FUNC(int) _PyDict_DelItem_KnownHash(PyObject *mp, PyObject *key,
7777
Py_hash_t hash);
78+
PyAPI_FUNC(int) _PyDict_DelItemIf(PyObject *mp, PyObject *key,
79+
int (*predicate)(PyObject *value));
7880
#endif
7981
PyAPI_FUNC(void) PyDict_Clear(PyObject *mp);
8082
PyAPI_FUNC(int) PyDict_Next(

‎Lib/test/test_weakref.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,6 +1673,18 @@ def test_threaded_weak_valued_pop(self):
16731673
x = d.pop(10, 10)
16741674
self.assertIsNot(x, None) # we never put None in there!
16751675

1676+
def test_threaded_weak_valued_consistency(self):
1677+
# Issue #28427: old keys should not remove new values from
1678+
# WeakValueDictionary when collecting from another thread.
1679+
d = weakref.WeakValueDictionary()
1680+
with collect_in_thread():
1681+
for i in range(200000):
1682+
o = RefCycle()
1683+
d[10] = o
1684+
# o is still alive, so the dict can't be empty
1685+
self.assertEqual(len(d), 1)
1686+
o = None # lose ref
1687+
16761688

16771689
from test import mapping_tests
16781690

‎Lib/weakref.py

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@
1616
proxy,
1717
CallableProxyType,
1818
ProxyType,
19-
ReferenceType)
19+
ReferenceType,
20+
_remove_dead_weakref)
2021

2122
from _weakrefset import WeakSet, _IterationGuard
2223

@@ -111,7 +112,9 @@ def remove(wr, selfref=ref(self)):
111112
if self._iterating:
112113
self._pending_removals.append(wr.key)
113114
else:
114-
del self.data[wr.key]
115+
# Atomic removal is necessary since this function
116+
# can be called asynchronously by the GC
117+
_remove_dead_weakref(d, wr.key)
115118
self._remove = remove
116119
# A list of keys to be removed
117120
self._pending_removals = []
@@ -125,9 +128,12 @@ def _commit_removals(self):
125128
# We shouldn't encounter any KeyError, because this method should
126129
# always be called *before* mutating the dict.
127130
while l:
128-
del d[l.pop()]
131+
key = l.pop()
132+
_remove_dead_weakref(d, key)
129133

130134
def __getitem__(self, key):
135+
if self._pending_removals:
136+
self._commit_removals()
131137
o = self.data[key]()
132138
if o is None:
133139
raise KeyError(key)
@@ -140,9 +146,13 @@ def __delitem__(self, key):
140146
del self.data[key]
141147

142148
def __len__(self):
143-
return len(self.data) - len(self._pending_removals)
149+
if self._pending_removals:
150+
self._commit_removals()
151+
return len(self.data)
144152

145153
def __contains__(self, key):
154+
if self._pending_removals:
155+
self._commit_removals()
146156
try:
147157
o = self.data[key]()
148158
except KeyError:
@@ -158,6 +168,8 @@ def __setitem__(self, key, value):
158168
self.data[key] = KeyedRef(value, self._remove, key)
159169

160170
def copy(self):
171+
if self._pending_removals:
172+
self._commit_removals()
161173
new = WeakValueDictionary()
162174
for key, wr in self.data.items():
163175
o = wr()
@@ -169,6 +181,8 @@ def copy(self):
169181

170182
def __deepcopy__(self, memo):
171183
from copy import deepcopy
184+
if self._pending_removals:
185+
self._commit_removals()
172186
new = self.__class__()
173187
for key, wr in self.data.items():
174188
o = wr()
@@ -177,6 +191,8 @@ def __deepcopy__(self, memo):
177191
return new
178192

179193
def get(self, key, default=None):
194+
if self._pending_removals:
195+
self._commit_removals()
180196
try:
181197
wr = self.data[key]
182198
except KeyError:
@@ -190,13 +206,17 @@ def get(self, key, default=None):
190206
return o
191207

192208
def items(self):
209+
if self._pending_removals:
210+
self._commit_removals()
193211
with _IterationGuard(self):
194212
for k, wr in self.data.items():
195213
v = wr()
196214
if v is not None:
197215
yield k, v
198216

199217
def keys(self):
218+
if self._pending_removals:
219+
self._commit_removals()
200220
with _IterationGuard(self):
201221
for k, wr in self.data.items():
202222
if wr() is not None:
@@ -214,10 +234,14 @@ def itervaluerefs(self):
214234
keep the values around longer than needed.
215235
216236
"""
237+
if self._pending_removals:
238+
self._commit_removals()
217239
with _IterationGuard(self):
218240
yield from self.data.values()
219241

220242
def values(self):
243+
if self._pending_removals:
244+
self._commit_removals()
221245
with _IterationGuard(self):
222246
for wr in self.data.values():
223247
obj = wr()
@@ -290,6 +314,8 @@ def valuerefs(self):
290314
keep the values around longer than needed.
291315
292316
"""
317+
if self._pending_removals:
318+
self._commit_removals()
293319
return list(self.data.values())
294320

295321

‎Misc/NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,9 @@ Core and Builtins
138138
Library
139139
-------
140140

141+
- Issue #28427: old keys should not remove new values from
142+
WeakValueDictionary when collecting from another thread.
143+
141144
- Issue 28923: Remove editor artifacts from Tix.py.
142145

143146
- Issue #28871: Fixed a crash when deallocate deep ElementTree.

‎Modules/_weakref.c

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,46 @@ _weakref_getweakrefcount_impl(PyObject *module, PyObject *object)
3535
}
3636

3737

38+
static int
39+
is_dead_weakref(PyObject *value)
40+
{
41+
if (!PyWeakref_Check(value)) {
42+
PyErr_SetString(PyExc_TypeError, "not a weakref");
43+
return -1;
44+
}
45+
return PyWeakref_GET_OBJECT(value) == Py_None;
46+
}
47+
48+
/*[clinic input]
49+
50+
_weakref._remove_dead_weakref -> object
51+
52+
dct: object(subclass_of='&PyDict_Type')
53+
key: object
54+
/
55+
56+
Atomically remove key from dict if it points to a dead weakref.
57+
[clinic start generated code]*/
58+
59+
static PyObject *
60+
_weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct,
61+
PyObject *key)
62+
/*[clinic end generated code: output=d9ff53061fcb875c input=19fc91f257f96a1d]*/
63+
{
64+
if (_PyDict_DelItemIf(dct, key, is_dead_weakref) < 0) {
65+
if (PyErr_ExceptionMatches(PyExc_KeyError))
66+
/* This function is meant to allow safe weak-value dicts
67+
with GC in another thread (see issue #28427), so it's
68+
ok if the key doesn't exist anymore.
69+
*/
70+
PyErr_Clear();
71+
else
72+
return NULL;
73+
}
74+
Py_RETURN_NONE;
75+
}
76+
77+
3878
PyDoc_STRVAR(weakref_getweakrefs__doc__,
3979
"getweakrefs(object) -- return a list of all weak reference objects\n"
4080
"that point to 'object'.");
@@ -88,6 +128,7 @@ weakref_proxy(PyObject *self, PyObject *args)
88128
static PyMethodDef
89129
weakref_functions[] = {
90130
_WEAKREF_GETWEAKREFCOUNT_METHODDEF
131+
_WEAKREF__REMOVE_DEAD_WEAKREF_METHODDEF
91132
{"getweakrefs", weakref_getweakrefs, METH_O,
92133
weakref_getweakrefs__doc__},
93134
{"proxy", weakref_proxy, METH_VARARGS,

‎Modules/clinic/_weakref.c.h

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,33 @@ _weakref_getweakrefcount(PyObject *module, PyObject *object)
2828
exit:
2929
return return_value;
3030
}
31-
/*[clinic end generated code: output=d9086c8576d46933 input=a9049054013a1b77]*/
31+
32+
PyDoc_STRVAR(_weakref__remove_dead_weakref__doc__,
33+
"_remove_dead_weakref($module, dct, key, /)\n"
34+
"--\n"
35+
"\n"
36+
"Atomically remove key from dict if it points to a dead weakref.");
37+
38+
#define _WEAKREF__REMOVE_DEAD_WEAKREF_METHODDEF \
39+
{"_remove_dead_weakref", (PyCFunction)_weakref__remove_dead_weakref, METH_VARARGS, _weakref__remove_dead_weakref__doc__},
40+
41+
static PyObject *
42+
_weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct,
43+
PyObject *key);
44+
45+
static PyObject *
46+
_weakref__remove_dead_weakref(PyObject *module, PyObject *args)
47+
{
48+
PyObject *return_value = NULL;
49+
PyObject *dct;
50+
PyObject *key;
51+
52+
if (!PyArg_ParseTuple(args, "O!O:_remove_dead_weakref",
53+
&PyDict_Type, &dct, &key))
54+
goto exit;
55+
return_value = _weakref__remove_dead_weakref_impl(module, dct, key);
56+
57+
exit:
58+
return return_value;
59+
}
60+
/*[clinic end generated code: output=5764cb64a6f66ffd input=a9049054013a1b77]*/

‎Objects/dictobject.c

Lines changed: 56 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1246,13 +1246,31 @@ _PyDict_SetItem_KnownHash(PyObject *op, PyObject *key, PyObject *value,
12461246
return insertdict(mp, key, hash, value);
12471247
}
12481248

1249+
static int
1250+
delitem_common(PyDictObject *mp, PyDictKeyEntry *ep, PyObject **value_addr)
1251+
{
1252+
PyObject *old_key, *old_value;
1253+
1254+
old_value = *value_addr;
1255+
*value_addr = NULL;
1256+
mp->ma_used--;
1257+
if (!_PyDict_HasSplitTable(mp)) {
1258+
ENSURE_ALLOWS_DELETIONS(mp);
1259+
old_key = ep->me_key;
1260+
Py_INCREF(dummy);
1261+
ep->me_key = dummy;
1262+
Py_DECREF(old_key);
1263+
}
1264+
Py_DECREF(old_value);
1265+
return 0;
1266+
}
1267+
12491268
int
12501269
PyDict_DelItem(PyObject *op, PyObject *key)
12511270
{
12521271
PyDictObject *mp;
12531272
Py_hash_t hash;
12541273
PyDictKeyEntry *ep;
1255-
PyObject *old_key, *old_value;
12561274
PyObject **value_addr;
12571275

12581276
if (!PyDict_Check(op)) {
@@ -1274,26 +1292,14 @@ PyDict_DelItem(PyObject *op, PyObject *key)
12741292
_PyErr_SetKeyError(key);
12751293
return -1;
12761294
}
1277-
old_value = *value_addr;
1278-
*value_addr = NULL;
1279-
mp->ma_used--;
1280-
if (!_PyDict_HasSplitTable(mp)) {
1281-
ENSURE_ALLOWS_DELETIONS(mp);
1282-
old_key = ep->me_key;
1283-
Py_INCREF(dummy);
1284-
ep->me_key = dummy;
1285-
Py_DECREF(old_key);
1286-
}
1287-
Py_DECREF(old_value);
1288-
return 0;
1295+
return delitem_common(mp, ep, value_addr);
12891296
}
12901297

12911298
int
12921299
_PyDict_DelItem_KnownHash(PyObject *op, PyObject *key, Py_hash_t hash)
12931300
{
12941301
PyDictObject *mp;
12951302
PyDictKeyEntry *ep;
1296-
PyObject *old_key, *old_value;
12971303
PyObject **value_addr;
12981304

12991305
if (!PyDict_Check(op)) {
@@ -1310,20 +1316,45 @@ _PyDict_DelItem_KnownHash(PyObject *op, PyObject *key, Py_hash_t hash)
13101316
_PyErr_SetKeyError(key);
13111317
return -1;
13121318
}
1313-
old_value = *value_addr;
1314-
*value_addr = NULL;
1315-
mp->ma_used--;
1316-
if (!_PyDict_HasSplitTable(mp)) {
1317-
ENSURE_ALLOWS_DELETIONS(mp);
1318-
old_key = ep->me_key;
1319-
Py_INCREF(dummy);
1320-
ep->me_key = dummy;
1321-
Py_DECREF(old_key);
1319+
return delitem_common(mp, ep, value_addr);
1320+
}
1321+
1322+
int
1323+
_PyDict_DelItemIf(PyObject *op, PyObject *key,
1324+
int (*predicate)(PyObject *value))
1325+
{
1326+
PyDictObject *mp;
1327+
Py_hash_t hash;
1328+
PyDictKeyEntry *ep;
1329+
PyObject **value_addr;
1330+
int res;
1331+
1332+
if (!PyDict_Check(op)) {
1333+
PyErr_BadInternalCall();
1334+
return -1;
13221335
}
1323-
Py_DECREF(old_value);
1324-
return 0;
1336+
assert(key);
1337+
hash = PyObject_Hash(key);
1338+
if (hash == -1)
1339+
return -1;
1340+
mp = (PyDictObject *)op;
1341+
ep = (mp->ma_keys->dk_lookup)(mp, key, hash, &value_addr);
1342+
if (ep == NULL)
1343+
return -1;
1344+
if (*value_addr == NULL) {
1345+
_PyErr_SetKeyError(key);
1346+
return -1;
1347+
}
1348+
res = predicate(*value_addr);
1349+
if (res == -1)
1350+
return -1;
1351+
if (res > 0)
1352+
return delitem_common(mp, ep, value_addr);
1353+
else
1354+
return 0;
13251355
}
13261356

1357+
13271358
void
13281359
PyDict_Clear(PyObject *op)
13291360
{

0 commit comments

Comments
 (0)
0