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

types.Union should support GC #88719

Closed
Fidget-Spinner opened this issue Jul 2, 2021 · 10 comments
Closed

types.Union should support GC #88719

Fidget-Spinner opened this issue Jul 2, 2021 · 10 comments
Labels
3.10 only security fixes 3.11 only security fixes release-blocker

Comments

@Fidget-Spinner
Copy link
Member

BPO 44553
Nosy @gvanrossum, @serhiy-storchaka, @gvanrossum, @pablogsal, @miss-islington, @Fidget-Spinner
PRs
  • bpo-44553 : Implement GC methods for types.Union #26993
  • [3.10] bpo-44553 : Implement GC methods for types.Union (GH-26993) #27002
  • bpo-44553: Correct failure in tp_new for the union object #27008
  • [3.10] bpo-44553: Correct failure in tp_new for the union object (GH-27008) #27009
    		•

    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 2021-07-03.20:07:19.248>
created_at = <Date 2021-07-02.14:53:38.461>
labels = ['release-blocker', '3.10', '3.11']
title = 'types.Union should support GC'
updated_at = <Date 2021-07-04.05:44:17.041>
user = 'https://github.com/Fidget-Spinner'

    bugs.python.org fields:

    activity = <Date 2021-07-04.05:44:17.041>
actor = 'kj'
assignee = 'none'
closed = True
closed_date = <Date 2021-07-03.20:07:19.248>
closer = 'pablogsal'
components = []
creation = <Date 2021-07-02.14:53:38.461>
creator = 'kj'
dependencies = []
files = []
hgrepos = []
issue_num = 44553
keywords = ['patch']
message_count = 10.0
messages = ['396867', '396897', '396900', '396901', '396920', '396921', '396924', '396925', '396931', '396938']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'serhiy.storchaka', 'Guido.van.Rossum', 'pablogsal', 'miss-islington', 'kj']
pr_nums = ['26993', '27002', '27008', '27009']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44553'
versions = ['Python 3.10', 'Python 3.11']
    @Fidget-Spinner
    Copy link
    Member Author

    types.Union objects can contain reference cycles, therefore causing memory leaks.

    E.g.::

    import sys, gc
from typing import TypeVar

gc.collect()
for _ in range(10):
 sys.gettotalrefcount()
 T = TypeVar('T')
 U = int | list[T]
 T.blah = U
 del T
 del U
 gc.collect()

    Result:
    84470
    0
    84488
    0
    84504
    0
    84520
    0
    84536
    0

    I'm sending a small PR soon to implement GC methods to fix this.

    @Fidget-Spinner Fidget-Spinner added 3.10 only security fixes 3.11 only security fixes labels Jul 2, 2021
    @serhiy-storchaka
    Copy link
    Member

    New changeset 1097384 by Ken Jin in branch 'main':
    bpo-44553 : Implement GC methods for types.Union (GH-26993)
    1097384

    @miss-islington
    Copy link
    Contributor

    New changeset 0856134 by Miss Islington (bot) in branch '3.10':
    bpo-44553 : Implement GC methods for types.Union (GH-26993)
    0856134

    @Fidget-Spinner
    Copy link
    Member Author

    Thanks for the helpful review and merge Serhiy.

    @pablogsal
    Copy link
    Member

    This commits seems to have broken the tracerefs buildbots:

    https://buildbot.python.org/all/#/builders/484/builds/322
    https://buildbot.python.org/all/#/builders/678/builds/127

    Please, take a look at soon as possible, otherwise per the buildbot policy we will need to revert the commits to avoid masking other issues.

    @pablogsal pablogsal reopened this Jul 3, 2021
    @pablogsal pablogsal reopened this Jul 3, 2021
    @pablogsal
    Copy link
    Member

    New changeset bc39614 by Pablo Galindo in branch 'main':
    bpo-44553: Correct failure in tp_new for the union object (GH-27008)
    bc39614

    @pablogsal
    Copy link
    Member

    New changeset 000b9e8 by Miss Islington (bot) in branch '3.10':
    bpo-44553: Correct failure in tp_new for the union object (GH-27008) (GH-27009)
    000b9e8

    @gvanrossum
    Copy link
    Member

    Thanks for the fix, Pablo!

    Ken Jin, we learned something today!

    @Fidget-Spinner
    Copy link
    Member Author

    Woops, thanks for the quick fix Pablo. I'm guessing the takeaway here is
    that we should start tracking a GC-ed object as early as possible?

    @Fidget-Spinner
    Copy link
    Member Author

    Oh I didn't see Pablo's message on the PR:

    @Fidget-Spinner For next occasions, the problem with this is that PyObject_GC_Del cannot be called like that because it overrides a bunch of cleanups like the call to _Py_ForgetReference and other possible handling.

    Thanks for the explanation!

    @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
    3.10 only security fixes 3.11 only security fixes release-blocker
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @gvanrossum @serhiy-storchaka @pablogsal @Fidget-Spinner @miss-islington