classification
Title: Unintentional hard reference assignment in Python version of OrderedDict.move_to_end
Type: resource usage Stage:
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: andrabogildea, python-dev, rhettinger, serhiy.storchaka
Priority: low Keywords: patch

Created on 2016-12-31 06:11 by rhettinger, last changed 2016-12-31 19:09 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
od_proxy.diff rhettinger, 2016-12-31 06:11 Technique 1: Reuse existing weakref review
od_proxy2.diff rhettinger, 2016-12-31 06:15 Technique 2: Create a new proxy review
od_proxy2a.diff rhettinger, 2016-12-31 07:25 Revised version of Technique 2 patch review
Messages (8)
msg284367 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-12-31 06:11
The root.prev and first.prev assignments should use weak references rather than hard references.  Spotted by Andra Bogildea.
msg284368 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-31 06:33
Makes sense. Any chance to create tests?
msg284375 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-12-31 07:33
I'm leaving this for Andra Bogildea to work through and comment on.  

Testing is likely to be awkward at best.  It would involve white box implementation specific steps like accessing the name managed variables and rotating through the doubly linked list to verify that all the prev-links have the type, <class 'weakproxy'>.
msg284377 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-31 08:00
od_proxy2a.diff LGTM.
msg284389 - (view) Author: Andra Bogildea (andrabogildea) Date: 2016-12-31 12:20
od_proxy.diff and od_proxy2a.diff are the same. Did you meant soft_link = proxy(link) in od_proxy2a.diff?

I believe Technique 1 (od_proxy.diff) is preferable, because it reuses the proxy object.

Regarding automated tests, I agree with what’s been said. The usage of weak references is an internal detail (and name mangling makes it even more inconvenient).
msg284410 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-31 19:03
New changeset 19376765d7c3 by Raymond Hettinger in branch '3.5':
Issue #29119: Fix weakref in OrderedDict.move_to_end(). Work by Andra Bogildea.
https://hg.python.org/cpython/rev/19376765d7c3
msg284411 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-31 19:08
New changeset cd811b867393 by Raymond Hettinger in branch '3.6':
Issue #29119:  Merge in ACK from 3.5 branch
https://hg.python.org/cpython/rev/cd811b867393
msg284412 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-12-31 19:09
Nice work Andra.  Thank you.
History
Date User Action Args
2016-12-31 19:09:38rhettingersetstatus: open -> closed
resolution: fixed
messages: + msg284412
2016-12-31 19:08:30python-devsetmessages: + msg284411
2016-12-31 19:03:28python-devsetnosy: + python-dev
messages: + msg284410
2016-12-31 12:20:26andrabogildeasetnosy: + andrabogildea
messages: + msg284389
2016-12-31 08:00:04serhiy.storchakasetmessages: + msg284377
2016-12-31 07:33:17rhettingersetmessages: + msg284375
2016-12-31 07:25:08rhettingersetfiles: + od_proxy2a.diff
2016-12-31 06:33:37serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg284368
2016-12-31 06:15:15rhettingersetpriority: normal -> low
files: + od_proxy2.diff
2016-12-31 06:11:43rhettingercreate