classification
Title: Py_NewInterpreter() leaks a reference on warnoptions in _PySys_EndInit()
Type: resource usage Stage: resolved
Components: Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, ncoghlan, vstinner
Priority: normal Keywords:

Created on 2017-06-08 10:48 by vstinner, last changed 2017-09-11 17:14 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1998 merged vstinner, 2017-06-08 11:10
Messages (7)
msg295399 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 10:48
Recently, the Python initialization was reworked to start to implement the PEP 432:

- bpo-22257: commits 1abcf6700b4da6207fe859de40c6c1bada6b4fec and 6b4be195cd8868b76eb6fbe166acc39beee8ce36

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().
msg295404 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-06-08 11:04
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 :)
msg295405 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 11:11
See also bpo-30547 for other refleaks somehow related to that one.
msg295410 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 11:27
New changeset 865de27dd79571a4a5c7a7d22a07fb909c4a9f8e by Victor Stinner in branch 'master':
bpo-30598: _PySys_EndInit() now duplicates warnoptions (#1998)
https://github.com/python/cpython/commit/865de27dd79571a4a5c7a7d22a07fb909c4a9f8e
msg296409 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-20 10:21
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.
msg296517 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-06-21 04:55
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.
msg301890 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-11 17:14
This bug came back in master: see bpo-31420.
History
Date User Action Args
2017-09-11 17:14:37vstinnersetmessages: + msg301890
2017-06-21 04:55:16ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg296517

stage: resolved
2017-06-20 10:21:39vstinnersetmessages: + msg296409
2017-06-08 11:27:49vstinnersetmessages: + msg295410
2017-06-08 11:11:37vstinnersetmessages: + msg295405
2017-06-08 11:10:11vstinnersetpull_requests: + pull_request2064
2017-06-08 11:04:18ncoghlansetmessages: + msg295404
2017-06-08 10:48:40vstinnercreate