Repository navigation
Py_NewInterpreter() leaks a reference on warnoptions in _PySys_EndInit() #74783
Description
| BPO | 30598 |
|---|---|
| Nosy | @ncoghlan, @vstinner, @ericsnowcurrently |
| PRs |
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 2017-06-21.04:55:16.636>
created_at = <Date 2017-06-08.10:48:40.878>
labels = ['3.7', 'performance']
title = 'Py_NewInterpreter() leaks a reference on warnoptions in _PySys_EndInit()'
updated_at = <Date 2017-09-11.17:14:37.649>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2017-09-11.17:14:37.649>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-06-21.04:55:16.636>
closer = 'ncoghlan'
components = []
creation = <Date 2017-06-08.10:48:40.878>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 30598
keywords = []
message_count = 7.0
messages = ['295399', '295404', '295405', '295410', '296409', '296517', '301890']
nosy_count = 3.0
nosy_names = ['ncoghlan', 'vstinner', 'eric.snow']
pr_nums = ['1998']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue30598'
versions = ['Python 3.7']
Activity
vstinner commentedon Jun 8, 2017
Recently, the Python initialization was reworked to start to implement the PEP-432:
Now, Py_NewInterpreter() leaks a reference on warnoptions in _PySys_EndInit(). We tried with Stéphane Wirtel and Louie Lu to add Py_DECREF(warnoptions), but test_capi does crash with this change.
The problem is that warnoptions is stored in a C global variable *and* in sys.warnoptions of each interpreter. The ownership of this variable is unclear.
I don't think that it's a good idea to share a list between two interpreters and so I created this issue to propose to redesign this variable.
The tricky part is that the C global variable is also accessed by 2 public C functions: PySys_ResetWarnOptions() and PySys_AddWarnOptionUnicode().
ncoghlan commentedon Jun 8, 2017
I'd suggest that the right fix here would be to move warnoptions into the config struct as proposed in the PEP: https://www.python.org/dev/peps/pep-0432/#supported-configuration-settings
The main reason we merged this as a private API was so we could do that setting-by-setting, with the test suite ensuring we weren't breaking anything.
It looks like in this case, it's the status quo that's broken, and the change makes it possible to fix it :)
vstinner commentedon Jun 8, 2017
See also bpo-30547 for other refleaks somehow related to that one.
vstinner commentedon Jun 8, 2017
New changeset 865de27 by Victor Stinner in branch 'master':
bpo-30598: _PySys_EndInit() now duplicates warnoptions (bpo-1998)
865de27
vstinner commentedon Jun 20, 2017
Nick: "I'd suggest that the right fix here would be to move warnoptions into the config struct as proposed in the PEP: https://www.python.org/dev/peps/pep-0432/#supported-configuration-settings"
Nick, Eric: do you want to do that it? If you are busy, that's fine, I will just close the issue since my concern (the ref leak) is now fixed.
ncoghlan commentedon Jun 21, 2017
We'll likely still move it eventually, but I don't think that's a good reason to keep this issue open - it's more a part of making incremental progress towards being able to make PEP-432 a public API.
vstinner commentedon Sep 11, 2017
This bug came back in master: see bpo-31420.