Use traverse & finalize in xxlimited and in PEP 489 tests

[encukou]

BPO	24373
Nosy	@ncoghlan, @encukou, @serhiy-storchaka
Files	xxlimited-finalize.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 2015-06-04.12:58:45.548>
created_at = <Date 2015-06-03.13:21:01.792>
labels = ['extension-modules', 'type-bug']
title = 'Use traverse & finalize in xxlimited and in PEP 489 tests'
updated_at = <Date 2018-05-25.08:53:50.243>
user = 'https://github.com/encukou'

bugs.python.org fields:

activity = <Date 2018-05-25.08:53:50.243>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2015-06-04.12:58:45.548>
closer = 'ncoghlan'
components = ['Extension Modules']
creation = <Date 2015-06-03.13:21:01.792>
creator = 'petr.viktorin'
dependencies = []
files = ['39605']
hgrepos = []
issue_num = 24373
keywords = ['patch']
message_count = 5.0
messages = ['244743', '244807', '244809', '244813', '317663']
nosy_count = 4.0
nosy_names = ['ncoghlan', 'petr.viktorin', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24373'
versions = ['Python 3.5', 'Python 3.6']

[encukou]

The example object in the xxlimited module can be part of a reference loop (it can contain itself), so it needs GC and tp_traverse.

The tp_dealloc hook was incorrect, and a correct version would be difficult to generalize for something more complicated than a example class (bpo-16690; [0]). It's better to avoid dealloc and show off tp_finalize.

Same for the class in _testmultiphase (PEP-489 tests), which is based on xxlimited. The incorrect dealloc is causing the reference leak mentioned in bpo-24268.

The Xxo object also wasn't actually added to the module.

Here is a patch to fix these.

[0] https://mail.python.org/pipermail/python-dev/2015-June/140422.html

[ncoghlan]

Would it also be worth making at docs update to tp_dealloc, suggesting the use of tp_traverse/finalize?: https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_dealloc

And perhaps from PyType_FromSpec? https://docs.python.org/3/c-api/type.html?highlight=pytype_fromspec#c.PyType_FromSpec

[encukou]

tp_traverse is completely orthogonal to tp_dealloc, it's needed to detect (and then break) reference cycles like:
obj = xxlimited.Xxo()
obj.foo = obj

As for tp_finalize: yes, mentioning it in tp_dealloc docs would be good, but I'll need a bit more studying to understand the problem correctly. The cases fixed here are relatively simple; Antoine gives more complex ones in [0]. When I feel qualified to give advice, I'll change the docs. (And most likely, write a PEP to make things easier; some changes to classes will be needed anyway to make PEP-489 multi-phase init work nicely in all cases).
But, I plan to focus my CPython time on documenting PEP-489 before diving in here. I think bpo-16690 is a good place to track tp_dealloc docs changes.

[0] https://mail.python.org/pipermail/python-dev/2015-June/140423.html

[python-dev]

New changeset 265eeb60443a by Nick Coghlan in branch '3.5':
Issue bpo-24373: Eliminate PEP-489 test refleaks
https://hg.python.org/cpython/rev/265eeb60443a

New changeset f24cd8bc5250 by Nick Coghlan in branch 'default':
Merge fix for issue bpo-24373 from 3.5
https://hg.python.org/cpython/rev/f24cd8bc5250

[serhiy-storchaka]

tp_finalize handlers should return "void". See bpo-33644.