Skip to content

bpo-24596: Decref module in PyRun_SimpleFileExFlags() on SystemExit#7918

Merged
pitrou merged 2 commits into
python:masterfrom
ZackerySpytz:bpo-24596-decref-m-systemexit
Jul 3, 2018
Merged

bpo-24596: Decref module in PyRun_SimpleFileExFlags() on SystemExit#7918
pitrou merged 2 commits into
python:masterfrom
ZackerySpytz:bpo-24596-decref-m-systemexit

Conversation

@ZackerySpytz

@ZackerySpytz ZackerySpytz commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

PyErr_Print() will not return when the exception is a SystemExit, so
decref the main module object in that case.

https://bugs.python.org/issue24596

PyErr_Print() will not return when the exception is a SystemExit, so
decref the __main__ module object in that case.
Comment thread Python/pythonrun.c Outdated
flush_io();
if (v == NULL) {
if (PyErr_ExceptionMatches(PyExc_SystemExit)) {
Py_DECREF(m);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this a special case? We could just always Py_DECREF(m) early.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Py_DECREF(m) would then be called one too many times.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor refactoring would be needed, but I don't think that's insurmountable (perhaps you can use Py_CLEAR instead).

Comment thread Lib/test/test_cmd_line.py Outdated
print("del sys.modules['__main__']", file=script)
assert_python_ok(filename)

def test_global_del_SystemExit(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think test_cmd_line is the right place for this. test_gc already has related tests (see e.g. test_gc_main_module_at_shutdown() there).

@ZackerySpytz

Copy link
Copy Markdown
Contributor Author

@pitrou Thanks for the comments. I've updated the PR.

@pitrou pitrou merged commit d8cba5d into python:master Jul 3, 2018
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @ZackerySpytz for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @ZackerySpytz for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@pitrou

pitrou commented Jul 3, 2018

Copy link
Copy Markdown
Member

Thanks @ZackerySpytz !

@bedevere-bot

Copy link
Copy Markdown

GH-8069 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 3, 2018
…ythonGH-7918)

PyErr_Print() will not return when the exception is a SystemExit, so
decref the __main__ module object in that case.
(cherry picked from commit d8cba5d)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-bot

Copy link
Copy Markdown

GH-8070 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 3, 2018
…ythonGH-7918)

PyErr_Print() will not return when the exception is a SystemExit, so
decref the __main__ module object in that case.
(cherry picked from commit d8cba5d)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
pitrou pushed a commit that referenced this pull request Jul 3, 2018
…H-7918) (GH-8070)

PyErr_Print() will not return when the exception is a SystemExit, so
decref the __main__ module object in that case.
(cherry picked from commit d8cba5d)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
pitrou pushed a commit that referenced this pull request Jul 3, 2018
…H-7918) (GH-8069)

PyErr_Print() will not return when the exception is a SystemExit, so
decref the __main__ module object in that case.
(cherry picked from commit d8cba5d)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants