Title: Use traverse & finalize in xxlimited and in PEP 489 tests
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: encukou, ncoghlan, python-dev
Priority: normal Keywords: patch

Created on 2015-06-03 13:21 by encukou, last changed 2015-06-04 12:58 by ncoghlan. This issue is now closed.

File name Uploaded Description Edit
xxlimited-finalize.patch encukou, 2015-06-03 13:21
Messages (4)
msg244743 - (view) Author: Petr Viktorin (encukou) * Date: 2015-06-03 13:21
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 (#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 #24268.

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

Here is a patch to fix these.

msg244807 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-06-04 08:00
Would it also be worth making at docs update to tp_dealloc, suggesting the use of tp_traverse/finalize?:

And perhaps from PyType_FromSpec?
msg244809 - (view) Author: Petr Viktorin (encukou) * Date: 2015-06-04 08:53
tp_traverse is completely orthogonal to tp_dealloc, it's needed to detect (and then break) reference cycles like:
    obj = xxlimited.Xxo() = 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 issue 16690 is a good place to track tp_dealloc docs changes.

msg244813 - (view) Author: Roundup Robot (python-dev) Date: 2015-06-04 11:58
New changeset 265eeb60443a by Nick Coghlan in branch '3.5':
Issue #24373: Eliminate PEP 489 test refleaks

New changeset f24cd8bc5250 by Nick Coghlan in branch 'default':
Merge fix for issue #24373 from 3.5
Date User Action Args
2015-06-04 12:58:45ncoghlansetstatus: open -> closed
type: behavior
resolution: fixed
stage: resolved
2015-06-04 11:58:34python-devsetnosy: + python-dev
messages: + msg244813
2015-06-04 08:53:10encukousetmessages: + msg244809
2015-06-04 08:00:38ncoghlansetmessages: + msg244807
2015-06-03 13:21:01encukoucreate