Skip to content
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

Jump to bottom

WeakValueDictionary bug in setdefault()&pop() #63741

Closed
arigo mannequin opened this issue Nov 10, 2013 · 15 comments
Closed

WeakValueDictionary bug in setdefault()&pop() #63741

arigo mannequin opened this issue Nov 10, 2013 · 15 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@arigo
Copy link
Mannequin

arigo mannequin commented Nov 10, 2013

BPO 19542
Nosy @tim-one, @freddrake, @arigo, @pitrou, @serhiy-storchaka, @Tinche
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
    		•
    Files
  • x.py: Raw test for Python 3. On Python 2, replace _thread with thread.
  • weakref.slice: Proposed fix for these two methods (not as a diff because it's a bit unreadable)
  • fix-weakvaluedict.diff: Test and fix for trunk
  • fix-weakvaluedict-2.7.diff: Test and fix for 2.7
    		•

    Note: 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:

    assignee = None
closed_at = <Date 2016-12-26.20:58:59.392>
created_at = <Date 2013-11-10.08:51:04.410>
labels = ['3.7', 'type-bug', 'library']
title = 'WeakValueDictionary bug in setdefault()&pop()'
updated_at = <Date 2017-03-31.16:36:07.310>
user = 'https://github.com/arigo'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:07.310>
actor = 'dstufft'
assignee = 'none'
closed = True
closed_date = <Date 2016-12-26.20:58:59.392>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2013-11-10.08:51:04.410>
creator = 'arigo'
dependencies = []
files = ['32557', '32558', '37992', '37993']
hgrepos = []
issue_num = 19542
keywords = ['needs review']
message_count = 15.0
messages = ['202510', '205603', '205609', '205614', '205645', '205674', '235301', '245470', '245471', '279455', '283599', '283601', '283602', '284040', '284046']
nosy_count = 8.0
nosy_names = ['tim.peters', 'fdrake', 'arigo', 'pitrou', 'Arfrever', 'python-dev', 'serhiy.storchaka', 'tinchester']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19542'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']
    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Nov 10, 2013

    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.

    @arigo arigo mannequin added the stdlib Python modules in the Lib dir label Nov 10, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Dec 8, 2013

    I think the underlying question is: are weak dicts otherwise MT-safe?

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Dec 8, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 8, 2013

    Ah, you're right. We just need to cook up a patch then.

    @pitrou pitrou added the type-bug An unexpected behavior, bug, or error label Dec 8, 2013
    @tim-one
    Copy link
    Member

    tim-one commented Dec 9, 2013

    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):

    ?

    @pitrou
    Copy link
    Member

    pitrou commented Dec 9, 2013

    Changeset ea70032a24b1 is where the pop(*args) thing comes from.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Feb 2, 2015

    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.

    @Tinche
    Copy link
    Mannequin

    Tinche mannequin commented Jun 18, 2015

    We're actually getting bitten by this in production through the Riak Python client, so this isn't a strictly theoretical problem.

    @pitrou
    Copy link
    Member

    pitrou commented Jun 18, 2015

    Yes, we need to fix this. Sorry for being a bit slow.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 25, 2016

    ping

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 19, 2016

    New changeset 5a542a2bca08 by Antoine Pitrou in branch '3.5':
    Issue bpo-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 bpo-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 bpo-19542: Fix bugs in WeakValueDictionary.setdefault() and WeakValueDictionary.pop()
    https://hg.python.org/cpython/rev/ac2715d04119

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 19, 2016

    New changeset bcb1f0698401 by Antoine Pitrou in branch '2.7':
    Issue bpo-19542: Fix bugs in WeakValueDictionary.setdefault() and WeakValueDictionary.pop()
    https://hg.python.org/cpython/rev/bcb1f0698401

    @pitrou
    Copy link
    Member

    pitrou commented Dec 19, 2016

    This is finally fixed!

    @pitrou pitrou closed this as completed Dec 19, 2016
    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Dec 26, 2016

    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
    ...

    @Arfrever Arfrever mannequin added the 3.7 (EOL) end of life label Dec 26, 2016
    @Arfrever Arfrever mannequin reopened this Dec 26, 2016
    @pitrou
    Copy link
    Member

    pitrou commented Dec 26, 2016

    This is expected, because of https://bugs.python.org/issue28427

    @pitrou pitrou closed this as completed Dec 26, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @pitrou @tim-one