cpython: e5ce7bdf9e99 (original) (raw)

Mercurial > cpython

changeset 105853:e5ce7bdf9e99

Issue #28427: old keys should not remove new values from WeakValueDictionary when collecting from another thread. [#28427]

Antoine Pitrou solipsis@pitrou.net
date	Tue, 27 Dec 2016 14:34:54 +0100
parents	77f5f31bf699(current diff)97d6616b2d22(diff)
children	3a2595f82447
files	Include/dictobject.h Misc/NEWS Objects/dictobject.c
diffstat	7 files changed, 194 insertions(+), 22 deletions(-)[+] [-] Include/dictobject.h 2 Lib/test/test_weakref.py 12 Lib/weakref.py 34 Misc/NEWS 3 Modules/_weakref.c 41 Modules/clinic/_weakref.c.h 32 Objects/dictobject.c 92

line wrap: on

line diff

--- a/Include/dictobject.h +++ b/Include/dictobject.h @@ -88,6 +88,8 @@ PyAPI_FUNC(int) PyDict_DelItem(PyObject #ifndef Py_LIMITED_API PyAPI_FUNC(int) _PyDict_DelItem_KnownHash(PyObject *mp, PyObject *key, Py_hash_t hash); +PyAPI_FUNC(int) _PyDict_DelItemIf(PyObject *mp, PyObject *key,

                             int (*predicate)(PyObject *value));[](#l1.8)

#endif PyAPI_FUNC(void) PyDict_Clear(PyObject *mp); PyAPI_FUNC(int) PyDict_Next(

--- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -1676,6 +1676,18 @@ class MappingTestCase(TestBase): x = d.pop(10, 10) self.assertIsNot(x, None) # we never put None in there

def test_threaded_weak_valued_consistency(self):

   # Issue #28427: old keys should not remove new values from[](#l2.8)

   # WeakValueDictionary when collecting from another thread.[](#l2.9)

   d = weakref.WeakValueDictionary()[](#l2.10)

```
   with collect_in_thread():[](#l2.11)
```

       for i in range(200000):[](#l2.12)

```
           o = RefCycle()[](#l2.13)
```
```
           d[10] = o[](#l2.14)
```

           # o is still alive, so the dict can't be empty[](#l2.15)

           self.assertEqual(len(d), 1)[](#l2.16)

           o = None  # lose ref[](#l2.17)

+ from test import mapping_tests

--- a/Lib/weakref.py +++ b/Lib/weakref.py @@ -16,7 +16,8 @@ from _weakref import ( proxy, CallableProxyType, ProxyType,

```
ReferenceType)[](#l3.7)
```

```
ReferenceType,[](#l3.8)
```
```
_remove_dead_weakref)[](#l3.9)
```

from _weakrefset import WeakSet, _IterationGuard @@ -111,7 +112,9 @@ class WeakValueDictionary(collections.Mu if self._iterating: self._pending_removals.append(wr.key) else:

               del self.data[wr.key][](#l3.17)

               # Atomic removal is necessary since this function[](#l3.18)

               # can be called asynchronously by the GC[](#l3.19)

               _remove_dead_weakref(d, wr.key)[](#l3.20)
   self._remove = remove[](#l3.21)
   # A list of keys to be removed[](#l3.22)
   self._pending_removals = [][](#l3.23)

@@ -125,9 +128,12 @@ class WeakValueDictionary(collections.Mu # We shouldn't encounter any KeyError, because this method should # always be called before mutating the dict. while l:

```
       del d[l.pop()][](#l3.28)
```

```
       key = l.pop()[](#l3.29)
```

       _remove_dead_weakref(d, key)[](#l3.30)

def getitem(self, key):

```
   if self._pending_removals:[](#l3.33)
```

       self._commit_removals()[](#l3.34)
   o = self.data[key]()[](#l3.35)
   if o is None:[](#l3.36)
       raise KeyError(key)[](#l3.37)

@@ -140,9 +146,13 @@ class WeakValueDictionary(collections.Mu del self.data[key] def len(self):

   return len(self.data) - len(self._pending_removals)[](#l3.42)

```
   if self._pending_removals:[](#l3.43)
```

       self._commit_removals()[](#l3.44)

```
   return len(self.data)[](#l3.45)
```

def contains(self, key):

```
   if self._pending_removals:[](#l3.48)
```

       self._commit_removals()[](#l3.49)
   try:[](#l3.50)
       o = self.data[key]()[](#l3.51)
   except KeyError:[](#l3.52)

@@ -158,6 +168,8 @@ class WeakValueDictionary(collections.Mu self.data[key] = KeyedRef(value, self._remove, key) def copy(self):

```
   if self._pending_removals:[](#l3.57)
```

       self._commit_removals()[](#l3.58)
   new = WeakValueDictionary()[](#l3.59)
   for key, wr in self.data.items():[](#l3.60)
       o = wr()[](#l3.61)

@@ -169,6 +181,8 @@ class WeakValueDictionary(collections.Mu def deepcopy(self, memo): from copy import deepcopy

```
   if self._pending_removals:[](#l3.66)
```

       self._commit_removals()[](#l3.67)
   new = self.__class__()[](#l3.68)
   for key, wr in self.data.items():[](#l3.69)
       o = wr()[](#l3.70)

@@ -177,6 +191,8 @@ class WeakValueDictionary(collections.Mu return new def get(self, key, default=None):

```
   if self._pending_removals:[](#l3.75)
```

       self._commit_removals()[](#l3.76)
   try:[](#l3.77)
       wr = self.data[key][](#l3.78)
   except KeyError:[](#l3.79)

@@ -190,6 +206,8 @@ class WeakValueDictionary(collections.Mu return o def items(self):

```
   if self._pending_removals:[](#l3.84)
```

       self._commit_removals()[](#l3.85)
   with _IterationGuard(self):[](#l3.86)
       for k, wr in self.data.items():[](#l3.87)
           v = wr()[](#l3.88)

@@ -197,6 +215,8 @@ class WeakValueDictionary(collections.Mu yield k, v def keys(self):

```
   if self._pending_removals:[](#l3.93)
```

       self._commit_removals()[](#l3.94)
   with _IterationGuard(self):[](#l3.95)
       for k, wr in self.data.items():[](#l3.96)
           if wr() is not None:[](#l3.97)

@@ -214,10 +234,14 @@ class WeakValueDictionary(collections.Mu keep the values around longer than needed. """

   if self._pending_removals:[](#l3.102)

       self._commit_removals()[](#l3.103)
   with _IterationGuard(self):[](#l3.104)
       yield from self.data.values()[](#l3.105)

def values(self):

   if self._pending_removals:[](#l3.108)

       self._commit_removals()[](#l3.109)
   with _IterationGuard(self):[](#l3.110)
       for wr in self.data.values():[](#l3.111)
           obj = wr()[](#l3.112)

@@ -290,6 +314,8 @@ class WeakValueDictionary(collections.Mu keep the values around longer than needed. """

   if self._pending_removals:[](#l3.117)

       self._commit_removals()[](#l3.118)
   return list(self.data.values())[](#l3.119)

--- a/Misc/NEWS +++ b/Misc/NEWS @@ -208,6 +208,9 @@ Core and Builtins Library ------- +- Issue #28427: old keys should not remove new values from

WeakValueDictionary when collecting from another thread. +

Issue 28923: Remove editor artifacts from Tix.py.
Issue #28871: Fixed a crash when deallocate deep ElementTree.

--- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -35,6 +35,46 @@ static Py_ssize_t } +static int +is_dead_weakref(PyObject *value) +{

if (!PyWeakref_Check(value)) {

   PyErr_SetString(PyExc_TypeError, "not a weakref");[](#l5.11)

```
   return -1;[](#l5.12)
```
}
return PyWeakref_GET_OBJECT(value) == Py_None;

+} + +/*[clinic input] + +_weakref._remove_dead_weakref -> object +

dct: object(subclass_of='&PyDict_Type')
key: object
/ + +Atomically remove key from dict if it points to a dead weakref. +[clinic start generated code]*/ + +static PyObject * +_weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct,

                              PyObject *key)[](#l5.30)

+/[clinic end generated code: output=d9ff53061fcb875c input=19fc91f257f96a1d]/ +{

if (_PyDict_DelItemIf(dct, key, is_dead_weakref) < 0) {

   if (PyErr_ExceptionMatches(PyExc_KeyError))[](#l5.34)

       /* This function is meant to allow safe weak-value dicts[](#l5.35)

          with GC in another thread (see issue #28427), so it's[](#l5.36)

          ok if the key doesn't exist anymore.[](#l5.37)

```
          */[](#l5.38)
```
```
       PyErr_Clear();[](#l5.39)
```
```
   else[](#l5.40)
```
```
       return NULL;[](#l5.41)
```
}
Py_RETURN_NONE;

+} + + PyDoc_STRVAR(weakref_getweakrefs__doc__, "getweakrefs(object) -- return a list of all weak reference objects\n" "that point to 'object'."); @@ -88,6 +128,7 @@ weakref_proxy(PyObject *self, PyObject * static PyMethodDef weakref_functions[] = { _WEAKREF_GETWEAKREFCOUNT_METHODDEF

WEAKREF__REMOVE_DEAD_WEAKREF_METHODDEF {"getweakrefs", weakref_getweakrefs, METH_O, weakref_getweakrefs__doc_}, {"proxy", weakref_proxy, METH_VARARGS,

--- a/Modules/clinic/_weakref.c.h +++ b/Modules/clinic/weakref.c.h @@ -29,4 +29,34 @@ static PyObject exit: return return_value; } -/[clinic end generated code: output=e1ad587147323e19 input=a9049054013a1b77]*/ + +PyDoc_STRVAR(weakref__remove_dead_weakref__doc, +"_remove_dead_weakref($module, dct, key, /)\n" +"--\n" +"\n" +"Atomically remove key from dict if it points to a dead weakref."); + +#define _WEAKREF__REMOVE_DEAD_WEAKREF_METHODDEF [](#l6.15)

{"_remove_dead_weakref", (PyCFunction)weakref__remove_dead_weakref, METH_VARARGS, weakref__remove_dead_weakref__doc},

+ +static PyObject * +_weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct,

                              PyObject *key);[](#l6.20)

+ +static PyObject * +_weakref__remove_dead_weakref(PyObject *module, PyObject *args) +{

PyObject *return_value = NULL;
PyObject *dct;
PyObject *key;

if (!PyArg_ParseTuple(args, "O!O:_remove_dead_weakref",

   &PyDict_Type, &dct, &key)) {[](#l6.30)

```
   goto exit;[](#l6.31)
```
}
return_value = _weakref__remove_dead_weakref_impl(module, dct, key);

+ +exit:

return return_value;

+} +/[clinic end generated code: output=e860dd818a44bc9b input=a9049054013a1b77]/

--- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1583,11 +1583,32 @@ int return insertdict(mp, key, hash, value); } +static int +delitem_common(PyDictObject *mp, Py_ssize_t hashpos, Py_ssize_t ix,

```
          PyObject *old_value)[](#l7.9)
```

PyObject *old_key;
PyDictKeyEntry *ep;

mp->ma_used--;
mp->ma_version_tag = DICT_NEXT_VERSION();
ep = &DK_ENTRIES(mp->ma_keys)[ix];
dk_set_index(mp->ma_keys, hashpos, DKIX_DUMMY);
ENSURE_ALLOWS_DELETIONS(mp);
old_key = ep->me_key;
ep->me_key = NULL;
ep->me_value = NULL;
Py_DECREF(old_key);
Py_DECREF(old_value);

assert(_PyDict_CheckConsistency(mp));
return 0;

+} + int PyDict_DelItem(PyObject *op, PyObject *key) { Py_hash_t hash; - assert(key); if (!PyUnicode_CheckExact(key) || (hash = ((PyASCIIObject *) key)->hash) == -1) { @@ -1604,8 +1625,7 @@ int { Py_ssize_t hashpos, ix; PyDictObject *mp;

PyDictKeyEntry *ep;
PyObject *old_key, *old_value;

PyObject *old_value;

if (!PyDict_Check(op)) { PyErr_BadInternalCall(); @@ -1632,22 +1652,60 @@ int assert(ix >= 0); }

assert(old_value != NULL);
mp->ma_used--;
mp->ma_version_tag = DICT_NEXT_VERSION();
ep = &DK_ENTRIES(mp->ma_keys)[ix];
dk_set_index(mp->ma_keys, hashpos, DKIX_DUMMY);
ENSURE_ALLOWS_DELETIONS(mp);
old_key = ep->me_key;
ep->me_key = NULL;
ep->me_value = NULL;
Py_DECREF(old_key);
Py_DECREF(old_value);

assert(_PyDict_CheckConsistency(mp));
return 0;

return delitem_common(mp, hashpos, ix, old_value);

} +/* This function promises that the predicate -> deletion sequence is atomic

- (i.e. protected by the GIL), assuming the predicate itself doesn't
- release the GIL.
*/ +int +_PyDict_DelItemIf(PyObject *op, PyObject *key,

             int (*predicate)(PyObject *value))[](#l7.74)

Py_ssize_t hashpos, ix;
PyDictObject *mp;
Py_hash_t hash;
PyObject *old_value;
int res;

if (!PyDict_Check(op)) {
```
   PyErr_BadInternalCall();[](#l7.83)
```
```
   return -1;[](#l7.84)
```
}
assert(key);
hash = PyObject_Hash(key);
if (hash == -1)
```
   return -1;[](#l7.89)
```
mp = (PyDictObject *)op;
ix = (mp->ma_keys->dk_lookup)(mp, key, hash, &old_value, &hashpos);
if (ix == DKIX_ERROR)
```
   return -1;[](#l7.93)
```
if (ix == DKIX_EMPTY || old_value == NULL) {
```
   _PyErr_SetKeyError(key);[](#l7.95)
```
```
   return -1;[](#l7.96)
```
}
assert(dk_get_index(mp->ma_keys, hashpos) == ix);

// Split table doesn't allow deletion. Combine it.
if (_PyDict_HasSplitTable(mp)) {

   if (dictresize(mp, DK_SIZE(mp->ma_keys))) {[](#l7.102)

```
       return -1;[](#l7.103)
```
```
   }[](#l7.104)
```

   ix = (mp->ma_keys->dk_lookup)(mp, key, hash, &old_value, &hashpos);[](#l7.105)

```
   assert(ix >= 0);[](#l7.106)
```
}

res = predicate(old_value);
if (res == -1)
```
   return -1;[](#l7.111)
```
if (res > 0)

   return delitem_common(mp, hashpos, ix, old_value);[](#l7.113)

else
```
   return 0;[](#l7.115)
```

+} + + void PyDict_Clear(PyObject *op) {