Issue 19542: WeakValueDictionary bug in setdefault()&pop() (original) (raw)
Created on 2013-11-10 08:51 by arigo, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (15)
Author: Armin Rigo (arigo) * 
Date: 2013-11-10 08:51
WeakValueDictionary.setdefault() contains a bug that shows up in multithreaded situations using reference cycles. Attached a test case: it is possible for 'setdefault(key, default)' to return None, although None is never put as a value in the dictionary. (Actually, being a WeakValueDictionary, None is not allowed as a value.)
The reason is that the code in setdefault() ends in "return wr()", but the weakref "wr" might have gone invalid between the time it was fetched from "self.data" and the "wr()" itself, thus returning None.
A similar problem occurs in pop(), leading it to possibly raise KeyError even if it is called with a default argument.
Author: Antoine Pitrou (pitrou) *
Date: 2013-12-08 20:30
I think the underlying question is: are weak dicts otherwise MT-safe?
Author: Armin Rigo (arigo) *
Date: 2013-12-08 21:57
As you can see in x.py, the underlying question is rather: are weakdicts usable in a single thread of a multithreaded program? I believe that this question cannot reasonably be answered No, independently on the answer you want to give to your own question.
Author: Antoine Pitrou (pitrou) *
Date: 2013-12-08 22:19
Ah, you're right. We just need to cook up a patch then.
Author: Tim Peters (tim.peters) *
Date: 2013-12-09 03:55
The weakref.slice fix looks solid to me, although it appears to be specific to 2.7 (the methods are fancier on the current default branch, fiddling with self._pending_removals too).
Does anyone know why the signature of pop is:
def pop(self, key, *args)? It doesn't make sense to me to accept any number of arguments following key. Perhaps it's just an obscure way to avoid doing:
_noarg = object() # unique sentinel
def pop(self, key, default=_noarg):
?
Author: Antoine Pitrou (pitrou) *
Date: 2013-12-09 10:41
Changeset ea70032a24b1 is where the pop(*args) thing comes from.
Author: Armin Rigo (arigo) *
Date: 2015-02-02 23:42
Converted the test into an official test, which fails; and wrote the patch for CPython "3.trunk" and for CPython 2.7. Please review and commit.
Author: Tin Tvrtković (tinchester) *
Date: 2015-06-18 15:23
We're actually getting bitten by this in production through the Riak Python client, so this isn't a strictly theoretical problem.
Author: Antoine Pitrou (pitrou) *
Date: 2015-06-18 15:24
Yes, we need to fix this. Sorry for being a bit slow.
Author: Armin Rigo (arigo) *
Date: 2016-10-25 20:39
ping
Author: Roundup Robot (python-dev) 
Date: 2016-12-19 10:01
New changeset 5a542a2bca08 by Antoine Pitrou in branch '3.5': Issue #19542: Fix bugs in WeakValueDictionary.setdefault() and WeakValueDictionary.pop() https://hg.python.org/cpython/rev/5a542a2bca08
New changeset f3706a9430da by Antoine Pitrou in branch '3.6': Issue #19542: Fix bugs in WeakValueDictionary.setdefault() and WeakValueDictionary.pop() https://hg.python.org/cpython/rev/f3706a9430da
New changeset ac2715d04119 by Antoine Pitrou in branch 'default': Issue #19542: Fix bugs in WeakValueDictionary.setdefault() and WeakValueDictionary.pop() https://hg.python.org/cpython/rev/ac2715d04119
Author: Roundup Robot (python-dev)
Date: 2016-12-19 10:14
New changeset bcb1f0698401 by Antoine Pitrou in branch '2.7': Issue #19542: Fix bugs in WeakValueDictionary.setdefault() and WeakValueDictionary.pop() https://hg.python.org/cpython/rev/bcb1f0698401
Author: Antoine Pitrou (pitrou) *
Date: 2016-12-19 10:14
This is finally fixed!
Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) *
Date: 2016-12-26 20:06
Newly added test_threaded_weak_valued_pop() triggers ignored KeyError exceptions (on all branches):
$ python3.7 -m test -v test_weakref ... 0:00:00 [1/1] test_weakref ... test_make_weak_keyed_dict_from_dict (test.test_weakref.MappingTestCase) ... ok test_make_weak_keyed_dict_from_weak_keyed_dict (test.test_weakref.MappingTestCase) ... ok test_make_weak_keyed_dict_repr (test.test_weakref.MappingTestCase) ... ok test_make_weak_valued_dict_from_dict (test.test_weakref.MappingTestCase) ... ok test_make_weak_valued_dict_from_weak_valued_dict (test.test_weakref.MappingTestCase) ... ok test_make_weak_valued_dict_misc (test.test_weakref.MappingTestCase) ... ok test_make_weak_valued_dict_repr (test.test_weakref.MappingTestCase) ... ok test_threaded_weak_valued_pop (test.test_weakref.MappingTestCase) ... Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) Exception ignored in: <function WeakValueDictionary.__init__..remove at 0x7fb20612bb70> Traceback (most recent call last): File "/usr/lib64/python3.7/weakref.py", line 114, in remove del self.data[wr.key] KeyError: (10,) ok test_threaded_weak_valued_setdefault (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_bad_delitem (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_cascading_deletes (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_delitem (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_dict_popitem (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_dict_setdefault (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_dict_update (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_iters (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_len_cycles (test.test_weakref.MappingTestCase) ... ok test_weak_keyed_len_race (test.test_weakref.MappingTestCase) ... ok test_weak_keys (test.test_weakref.MappingTestCase) ... ok test_weak_keys_destroy_while_iterating (test.test_weakref.MappingTestCase) ... ok test_weak_valued_delitem (test.test_weakref.MappingTestCase) ... ok test_weak_valued_dict_popitem (test.test_weakref.MappingTestCase) ... ok test_weak_valued_dict_setdefault (test.test_weakref.MappingTestCase) ... ok test_weak_valued_dict_update (test.test_weakref.MappingTestCase) ... ok test_weak_valued_iters (test.test_weakref.MappingTestCase) ... ok test_weak_valued_len_cycles (test.test_weakref.MappingTestCase) ... ok test_weak_valued_len_race (test.test_weakref.MappingTestCase) ... ok test_weak_values (test.test_weakref.MappingTestCase) ... ok test_weak_values_destroy_while_iterating (test.test_weakref.MappingTestCase) ... ok ...
Ran 116 tests in 3.641s ...
Author: Antoine Pitrou (pitrou) *
Date: 2016-12-26 20:58
This is expected, because of https://bugs.python.org/issue28427
History
Date
User
Action
Args
2022-04-11 14:57:53
admin
set
github: 63741
2017-03-31 16:36:07
dstufft
set
pull_requests: + <pull%5Frequest828>
2016-12-26 20:58:59
pitrou
set
status: open -> closed
resolution: fixed
2016-12-26 20:58:47
pitrou
set
messages: +
2016-12-26 20:06:10
Arfrever
set
status: closed -> open
versions: + Python 3.7, - Python 3.4
nosy: + Arfrever
messages: +
resolution: fixed -> (no value)
2016-12-19 10:14:27
pitrou
set
status: open -> closed
resolution: fixed
messages: +
stage: patch review -> resolved
2016-12-19 10:14:00
python-dev
set
messages: +
2016-12-19 10:01:24
python-dev
set
nosy: + python-dev
messages: +
2016-10-25 21:11:58
serhiy.storchaka
set
nosy: + serhiy.storchaka
2016-10-25 20:39:44
arigo
set
messages: +
2015-06-18 15:24:27
pitrou
set
messages: +
versions: + Python 3.5, Python 3.6, - Python 3.3
2015-06-18 15:23:11
tinchester
set
nosy: + tinchester
messages: +
2015-02-02 23:42:34
arigo
set
keywords: + needs review, - patch
messages: +
stage: needs patch -> patch review
2015-02-02 23:40:50
arigo
set
files: + fix-weakvaluedict-2.7.diff
2015-02-02 23:40:35
arigo
set
files: + fix-weakvaluedict.diff
keywords: + patch
2013-12-09 10:41:07
pitrou
set
messages: +
2013-12-09 03:55:35
tim.peters
set
nosy: + tim.peters
messages: +
2013-12-08 22:19:31
pitrou
set
type: behavior
messages: +
stage: needs patch
2013-12-08 21:57:12
arigo
set
messages: +
2013-12-08 20:30:23
pitrou
set
messages: +
versions: + Python 3.4
2013-12-08 20:13:53
ned.deily
set
2013-11-10 08:57:24
arigo
set
files: + weakref.slice
2013-11-10 08:55:17
arigo
set
files: + x.py
2013-11-10 08:51:04
arigo
create