Memory leaks in functions

[jdemeyer]

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]

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.

[methane]

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]

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

Sure.

[methane]

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

[pablogsal]

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]

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

[pablogsal]

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]

"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]

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]

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]

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]

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.