classification
Title: WeakValueDictionary bug in setdefault()&pop()
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, arigo, fdrake, pitrou, python-dev, serhiy.storchaka, tim.peters, tinchester
Priority: normal Keywords: needs review

Created on 2013-11-10 08:51 by arigo, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
x.py arigo, 2013-11-10 08:55 Raw test for Python 3. On Python 2, replace _thread with thread.
weakref.slice arigo, 2013-11-10 08:57 Proposed fix for these two methods (not as a diff because it's a bit unreadable)
fix-weakvaluedict.diff arigo, 2015-02-02 23:40 Test and fix for trunk review
fix-weakvaluedict-2.7.diff arigo, 2015-02-02 23:40 Test and fix for 2.7 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (15)
msg202510 - (view) Author: Armin Rigo (arigo) * (Python committer) 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.
msg205603 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-08 20:30
I think the underlying question is: are weak dicts otherwise MT-safe?
msg205609 - (view) Author: Armin Rigo (arigo) * (Python committer) 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.
msg205614 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-08 22:19
Ah, you're right. We just need to cook up a patch then.
msg205645 - (view) Author: Tim Peters (tim.peters) * (Python committer) 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):

?
msg205674 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-09 10:41
Changeset ea70032a24b1 is where the pop(*args) thing comes from.
msg235301 - (view) Author: Armin Rigo (arigo) * (Python committer) 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.
msg245470 - (view) 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.
msg245471 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-06-18 15:24
Yes, we need to fix this. Sorry for being a bit slow.
msg279455 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2016-10-25 20:39
ping
msg283599 - (view) 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
msg283601 - (view) 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
msg283602 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-12-19 10:14
This is finally fixed!
msg284040 - (view) 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__.<locals>.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__.<locals>.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__.<locals>.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__.<locals>.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__.<locals>.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__.<locals>.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__.<locals>.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
...
msg284046 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-12-26 20:58
This is expected, because of https://bugs.python.org/issue28427
History
Date User Action Args
2017-03-31 16:36:07dstufftsetpull_requests: + pull_request828
2016-12-26 20:58:59pitrousetstatus: open -> closed
resolution: fixed
2016-12-26 20:58:47pitrousetmessages: + msg284046
2016-12-26 20:06:10Arfreversetstatus: closed -> open
versions: + Python 3.7, - Python 3.4
nosy: + Arfrever

messages: + msg284040

resolution: fixed -> (no value)
2016-12-19 10:14:27pitrousetstatus: open -> closed
resolution: fixed
messages: + msg283602

stage: patch review -> resolved
2016-12-19 10:14:00python-devsetmessages: + msg283601
2016-12-19 10:01:24python-devsetnosy: + python-dev
messages: + msg283599
2016-10-25 21:11:58serhiy.storchakasetnosy: + serhiy.storchaka
2016-10-25 20:39:44arigosetmessages: + msg279455
2015-06-18 15:24:27pitrousetmessages: + msg245471
versions: + Python 3.5, Python 3.6, - Python 3.3
2015-06-18 15:23:11tinchestersetnosy: + tinchester
messages: + msg245470
2015-02-02 23:42:34arigosetkeywords: + needs review, - patch

messages: + msg235301
stage: needs patch -> patch review
2015-02-02 23:40:50arigosetfiles: + fix-weakvaluedict-2.7.diff
2015-02-02 23:40:35arigosetfiles: + fix-weakvaluedict.diff
keywords: + patch
2013-12-09 10:41:07pitrousetmessages: + msg205674
2013-12-09 03:55:35tim.peterssetnosy: + tim.peters
messages: + msg205645
2013-12-08 22:19:31pitrousettype: behavior
messages: + msg205614
stage: needs patch
2013-12-08 21:57:12arigosetmessages: + msg205609
2013-12-08 20:30:23pitrousetmessages: + msg205603
versions: + Python 3.4
2013-12-08 20:13:53ned.deilysetnosy: + fdrake, pitrou
2013-11-10 08:57:24arigosetfiles: + weakref.slice
2013-11-10 08:55:17arigosetfiles: + x.py
2013-11-10 08:51:04arigocreate