Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use traverse & finalize in xxlimited and in PEP 489 tests #68561

Closed
encukou opened this issue Jun 3, 2015 · 5 comments
Closed

Use traverse & finalize in xxlimited and in PEP 489 tests #68561

encukou opened this issue Jun 3, 2015 · 5 comments
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@encukou
Copy link
Member

encukou commented Jun 3, 2015

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
    Copy link
    Member Author

    encukou commented Jun 3, 2015

    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

    @encukou encukou added the extension-modules C modules in the Modules dir label Jun 3, 2015
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 4, 2015

    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
    Copy link
    Member Author

    encukou commented Jun 4, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jun 4, 2015

    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

    @ncoghlan ncoghlan closed this as completed Jun 4, 2015
    @ncoghlan ncoghlan added the type-bug An unexpected behavior, bug, or error label Jun 4, 2015
    @serhiy-storchaka
    Copy link
    Member

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @encukou @ncoghlan @serhiy-storchaka