[issue28427] WeakValueDictionary next bug (with multithreading)

dubiousjim added the comment: In response to Issue #7105, self._pending_removals was added to WeakValueDictionaries (and also WeakKeyDictionaries, but they're not relevant to what I'm about to discuss). This was in changesets 58194 to tip and 58195 to 3.1, back in Jan 2010. In those changesets

[issue28427] WeakValueDictionary next bug (with multithreading)

Changes by Serhiy Storchaka : -- pull_requests: -849 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://m

[issue28427] WeakValueDictionary next bug (with multithreading)

Larry Hastings added the comment: I tested this in a freshly-built 3.4.6. Although it reproduced the behavior you're complaining about--it threw the assert in Armin's test.py, and Serhiy's issue28427.py prints an admonishing FAIL--neither test *crashes* CPython. So I'm not convinced either o

[issue28427] WeakValueDictionary next bug (with multithreading)

Changes by Donald Stufft : -- pull_requests: +849 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: I've pushed the fixes now. It does introduce a small amount of additional code duplication in dictobject.c, but nothing unmanageable. Sidenote: all branches now have a different version of dict object each, which makes maintenance really painful... -

[issue28427] WeakValueDictionary next bug (with multithreading)

Roundup Robot added the comment: New changeset 9acdcafd1418 by Antoine Pitrou in branch '2.7': Issue #28427: old keys should not remove new values from https://hg.python.org/cpython/rev/9acdcafd1418 -- ___ Python tracker

[issue28427] WeakValueDictionary next bug (with multithreading)

Roundup Robot added the comment: New changeset b8b0718d424f by Antoine Pitrou in branch '3.5': Issue #28427: old keys should not remove new values from https://hg.python.org/cpython/rev/b8b0718d424f New changeset 97d6616b2d22 by Antoine Pitrou in branch '3.6': Issue #28427: old keys should not r

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: The dict implementation in 3.6 has become very complicated, so I'd like someone to review the attached 3.6 patch. Serhiy, Inada? -- nosy: +inada.naoki Added file: http://bugs.python.org/file45965/issue28427-3.6.patch ___

[issue28427] WeakValueDictionary next bug (with multithreading)

Armin Rigo added the comment: Agreed about fixing the other issues. I'm still unclear that we need anything more than just the _remove_dead_weakref function to do that, but also, I don't see a particular problem with adding self._commit_removals() a bit everywhere. About the O(1) expectation

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: > Now about __len__() returning a wrong result: it is a more complicated issue, > I fear. I already patched weakref.py inside PyPy in order to pass a unit > test inside CPython's test suite. This patch is to change __len__ to look > like this: [...] Thanks

[issue28427] WeakValueDictionary next bug (with multithreading)

Armin Rigo added the comment: I think the issue of __len__() is a different matter. More below. >> The C function would simply call PyObject_GetItem() and >> PyObject_DelItem()---without releasing the GIL in the middle. > > If you implement it like that, and the dictionary has non-trivial > key

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: Hi Armin, > is it still necessary to modify weakref.py so much, then? Not sure. I'll take a look again. Modifying __len__() at least is necessary, as the previous version took into account the length of _pending_removals (and could therefore return wrong resu

[issue28427] WeakValueDictionary next bug (with multithreading)

Armin Rigo added the comment: issue28427-atomic.patch: is it still necessary to modify weakref.py so much, then? What I had in mind was a C function with Python signature "del_if_equal(dict, key, value)"; the C function doesn't need to know about weakrefs and checking if they are dead. The C

[issue28427] WeakValueDictionary next bug (with multithreading)

Changes by Antoine Pitrou : -- stage: -> patch review ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: Here is a patch showing the "atomic C function" approach. It will avoid the aforementioned memory growth in the common case, in exchange for a small bit of additional C code. -- Added file: http://bugs.python.org/file45710/issue28427-atomic.patch ___

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: Note the issue with this patch is that it may keep keys (with dead values) alive longer than necessary: - this may prevent memory consumption from decreasing - this may keep alive some system resources This is ok when keys are small simple objects (strings or t

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: Here is a pure Python patch. -- Added file: http://bugs.python.org/file45709/issue28427-3.patch ___ Python tracker ___ _

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: (or we bite the bullet and add a C helper function for the atomic test-and-delete thing) -- ___ Python tracker ___

[issue28427] WeakValueDictionary next bug (with multithreading)

Antoine Pitrou added the comment: One possibility would be to always delay removals (always put them in _pending_removals). We would then have to enforce removals from time to time, but synchronously. -- nosy: +pitrou, tim.peters ___ Python tracker

[issue28427] WeakValueDictionary next bug (with multithreading)

Serhiy Storchaka added the comment: Following patch fixes more cases. But I don't think it fixes race conditions, it just makes them less likely. Increased priority because this bug makes weakref.WeakValueDictionary unusable in multithread program. -- components: +Library (Lib) priori

[issue28427] WeakValueDictionary next bug (with multithreading)

Armin Rigo added the comment: I'll admit I don't know how to properly fix this issue. What I came up with so far would need an atomic compare_and_delete operation on the dictionary self.data, so that we can do atomically: +elif self.data[wr.key] is wr: del

[issue28427] WeakValueDictionary next bug (with multithreading)

Serhiy Storchaka added the comment: Here is simpler reproducer for Python 3. One thread updates WeakValueDictionary in a loop, other threads runs garbage collecting in a loop. Values are collected asynchronously and this can cause removing new value by old key. Following patch fixes this examp

[issue28427] WeakValueDictionary next bug (with multithreading)

Changes by Serhiy Storchaka : -- keywords: +patch Added file: http://bugs.python.org/file45083/issue28427.patch ___ Python tracker ___ ___

[issue28427] WeakValueDictionary next bug (with multithreading)

Changes by Serhiy Storchaka : -- nosy: +fdrake, serhiy.storchaka versions: +Python 3.7 -Python 3.3 ___ Python tracker ___ ___ Python-b

[issue28427] WeakValueDictionary next bug (with multithreading)

New submission from Armin Rigo: Follow-up on http://bugs.python.org/issue19542. Another crash of using WeakValueDictionary() in a thread-local fashion inside a multi-threaded program. I must admit I'm not exactly sure why this occurs, but it is definitely showing an issue: two threads indepen