Unintentional hard reference assignment in Python version of OrderedDict.move_to_end

[rhettinger]

BPO	29119
Nosy	@rhettinger, @serhiy-storchaka, @andrabogildea
Files	od_proxy.diff: Technique 1: Reuse existing weakref od_proxy2.diff: Technique 2: Create a new proxy od_proxy2a.diff: Revised version of Technique 2 patch

^{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-31.19:09:38.404>
created_at = <Date 2016-12-31.06:11:43.002>
labels = ['3.7', 'library', 'performance']
title = 'Unintentional hard reference assignment in Python version of OrderedDict.move_to_end'
updated_at = <Date 2016-12-31.19:09:38.403>
user = 'https://github.com/rhettinger'

bugs.python.org fields:

activity = <Date 2016-12-31.19:09:38.403>
actor = 'rhettinger'
assignee = 'none'
closed = True
closed_date = <Date 2016-12-31.19:09:38.404>
closer = 'rhettinger'
components = ['Library (Lib)']
creation = <Date 2016-12-31.06:11:43.002>
creator = 'rhettinger'
dependencies = []
files = ['46097', '46098', '46099']
hgrepos = []
issue_num = 29119
keywords = ['patch']
message_count = 8.0
messages = ['284367', '284368', '284375', '284377', '284389', '284410', '284411', '284412']
nosy_count = 4.0
nosy_names = ['rhettinger', 'python-dev', 'serhiy.storchaka', 'andrabogildea']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue29119'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[rhettinger]

The root.prev and first.prev assignments should use weak references rather than hard references. Spotted by Andra Bogildea.

[serhiy-storchaka]

Makes sense. Any chance to create tests?

[rhettinger]

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

[serhiy-storchaka]

od_proxy2a.diff LGTM.

[andrabogildea]

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

[python-dev]

New changeset 19376765d7c3 by Raymond Hettinger in branch '3.5':
Issue bpo-29119: Fix weakref in OrderedDict.move_to_end(). Work by Andra Bogildea.
https://hg.python.org/cpython/rev/19376765d7c3

[python-dev]

New changeset cd811b867393 by Raymond Hettinger in branch '3.6':
Issue bpo-29119: Merge in ACK from 3.5 branch
https://hg.python.org/cpython/rev/cd811b867393

[rhettinger]

Nice work Andra. Thank you.