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.

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

Memory leaks in functions #77599

Closed
jdemeyer opened this issue May 3, 2018 · 12 comments
Closed

Memory leaks in functions #77599

jdemeyer opened this issue May 3, 2018 · 12 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@jdemeyer
Copy link
Contributor

jdemeyer commented May 3, 2018

BPO 33418
Nosy @Yhg1s, @vstinner, @methane, @serhiy-storchaka, @jdemeyer, @pablogsal
PRs
  • bpo-33418: Add tp_clear for function object #8058
  • [3.8] Revert "bpo-33418: Add tp_clear for function object (GH-8058)" #15826
  • [3.8] bpo-33418: Restore tp_clear for function object #16502
    		•

    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 2019-09-02.15:19:27.413>
created_at = <Date 2018-05-03.17:54:23.579>
labels = ['interpreter-core', '3.8', 'performance']
title = 'Memory leaks in functions'
updated_at = <Date 2019-10-01.07:37:55.300>
user = 'https://github.com/jdemeyer'

    bugs.python.org fields:

    activity = <Date 2019-10-01.07:37:55.300>
actor = 'nascheme'
assignee = 'none'
closed = True
closed_date = <Date 2019-09-02.15:19:27.413>
closer = 'pablogsal'
components = ['Interpreter Core']
creation = <Date 2018-05-03.17:54:23.579>
creator = 'jdemeyer'
dependencies = []
files = []
hgrepos = []
issue_num = 33418
keywords = ['patch']
message_count = 12.0
messages = ['316125', '320953', '320954', '321009', '350993', '350994', '351013', '351587', '351604', '351605', '351687', '351737']
nosy_count = 6.0
nosy_names = ['twouters', 'vstinner', 'methane', 'serhiy.storchaka', 'jdemeyer', 'pablogsal']
pr_nums = ['8058', '15826', '16502']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue33418'
versions = ['Python 3.8']
    @jdemeyer
    Copy link
    Contributor Author

    jdemeyer commented May 3, 2018

    This is a memory leak:

    >>> while True:
...    def f(): pass
...    f.__doc__ = f

    This also:

    >>> while True:
...     f = [].append                                                                                                                                                   
...     f.__module__ = f

    This is because the classes "function" and "builtin_function_or_method" do not implement tp_clear.

    @jdemeyer jdemeyer added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels May 3, 2018
    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes labels May 3, 2018
    @methane
    Copy link
    Member

    methane commented Jul 3, 2018

    I'm not sure this should be backported to 3.6 and 2.7.
    While this is a obvious bug, f.__module__ = f seems very unnatural.

    @jdemeyer
    Copy link
    Contributor Author

    jdemeyer commented Jul 3, 2018

    While this is a obvious bug, f.__module__ = f seems very unnatural.

    Sure.

    @methane
    Copy link
    Member

    methane commented Jul 4, 2018

    New changeset 3c45240 by INADA Naoki in branch 'master':
    bpo-33418: Add tp_clear for function object (GH-8058)
    3c45240

    @methane methane removed the 3.7 (EOL) end of life label Jul 4, 2018
    @methane methane closed this as completed Jul 4, 2018
    @pablogsal
    Copy link
    Member

    This change introduced the possibility to have function objects in an inconsistent state. For example, when calling tp_clear on the function some code must be invoked that tries to ca the function but some fields are NULL causing a crash.

    I think we should revert this and think better how to protect against this situation.

    @pablogsal pablogsal reopened this Sep 2, 2019
    @pablogsal
    Copy link
    Member

    See also https://bugs.python.org/issue38006

    @pablogsal
    Copy link
    Member

    I am going to re-close this until we understand exactly how this is interacting with https://bugs.python.org/issue38006 as this seems more complicated than our first hypothesis.

    @vstinner
    Copy link
    Member

    "f.__doc__ = f" or "f.__module__ = f" don't make any sense. It looks like a bug triggered on purpose. While it's bad, it's not a big deal.

    The fix (commit 3c45240) is causing way worse issues than the "artificial" memory leak (reference cycle). I wrote PR 15826 to revert the change in Python 3.8. bpo-38006 root issues are not well understood nor fixed yet.

    @methane
    Copy link
    Member

    methane commented Sep 10, 2019

    I'm OK to revert it in 3.8.

    But I am worrying about func.func_closure. Can it create cyclic reference in real life applications?

    @Yhg1s
    Copy link
    Member

    Yhg1s commented Sep 10, 2019

    New changeset ccaea52 by T. Wouters (Victor Stinner) in branch '3.8':
    Revert "bpo-33418: Add tp_clear for function object (GH-8058)" (GH-15826)
    ccaea52

    @vstinner
    Copy link
    Member

    But I am worrying about func.func_closure. Can it create cyclic reference in real life applications?

    remove.__closure__ is part of a reference cycle in bpo-38006.

    I like func_clear() (which is still implemented in the master branch), but we need to understand why/how it was possible *crash Python* in bpo-38006.

    @jdemeyer
    Copy link
    Contributor Author

    It looks like a bug triggered on purpose.

    Absolutely. It's one of the many small issues that I found while working on PEP-590 and related things.

    @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.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    6 participants
    @vstinner @methane @jdemeyer @serhiy-storchaka @Yhg1s @pablogsal