-
-
Notifications
You must be signed in to change notification settings - Fork 30k
-
-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WeakValueDictionary bug in setdefault()&pop() #63741
Comments
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. |
I think the underlying question is: are weak dicts otherwise MT-safe? |
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. |
Ah, you're right. We just need to cook up a patch then. |
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 _noarg = object() # unique sentinel
def pop(self, key, default=_noarg): ? |
Changeset ea70032a24b1 is where the pop(*args) thing comes from. |
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. |
We're actually getting bitten by this in production through the Riak Python client, so this isn't a strictly theoretical problem. |
Yes, we need to fix this. Sorry for being a bit slow. |
ping |
New changeset 5a542a2bca08 by Antoine Pitrou in branch '3.5': New changeset f3706a9430da by Antoine Pitrou in branch '3.6': New changeset ac2715d04119 by Antoine Pitrou in branch 'default': |
New changeset bcb1f0698401 by Antoine Pitrou in branch '2.7': |
This is finally fixed! |
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 |
This is expected, because of https://bugs.python.org/issue28427 |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: