-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-132775: Fix Interpreter.call() __main__ Visibility #135595
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
gh-132775: Fix Interpreter.call() __main__ Visibility #135595
Conversation
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 8be5bc2 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135595%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Python/crossinterp.c
Outdated
finally: | ||
Py_DECREF(msgobj); | ||
return res; | ||
assert(PyImport_GetModule(&_Py_ID(__main__)) == main->cached.module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refleaks in test_crossinterp.py
come from PyImport_GetModule()
in the assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Python/crossinterp.c
Outdated
assert(main->cached.module != NULL); | ||
assert(main->cached.loaded != NULL); | ||
PyObject *exc = _PyErr_GetRaisedException(tstate); | ||
assert(PyImport_GetModule(&_Py_ID(__main__)) == main->cached.loaded); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Python/crossinterp.c
Outdated
} | ||
finally: | ||
if (exc != NULL) { | ||
ctx->main.cached.failed = _PyErr_GetRaisedException(tstate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx->main.cached.failed
can be set by *_isolated_main()
above via sync_module_capture_exc()
, in which case test_func_in___main___invalid
fails with refleaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Python/crossinterp.c
Outdated
if (_Py_CheckMainModule(mod) < 0) { | ||
// This is probably unrecoverable, so don't bother caching the error. | ||
assert(_PyErr_Occurred(tstate)); | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that Py_XDECREF(mod)
does not need to follow all _Py_CheckMainModule()
in this file on error, unlike in ceval.c
.
This is a little off topic and maybe unlikely, but I'd prefer _Py_CheckMainModule()
to decref the error message, as the function can be used without _Py_GetMainModule()
:
Lines 1142 to 1145 in 8dd8b5c
PyObject *msg = PyUnicode_FromString("invalid __main__ module"); | |
if (msg != NULL) { | |
(void)PyErr_SetImportError(msg, &_Py_ID(__main__), NULL); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Once again, @neonene, thanks for taking the time to track down these subtle details! |
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 364d81c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135595%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
!buildbot AMD64 CentOS9 NoGIL Refleaks |
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 364d81c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135595%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…-135595) As noted in the new tests, there are a few situations we must carefully accommodate for functions that get pickled during interp.call(). We do so by running the script from the main interpreter's __main__ module in a hidden module in the other interpreter. That hidden module is used as the function __globals__. (cherry picked from commit 269e19e) Co-authored-by: Eric Snow <[email protected]>
GH-135638 is a backport of this pull request to the 3.14 branch. |
As noted in the new tests, there are a few situations we must carefully accommodate for functions that get pickled during interp.call(). We do so by running the script from the main interpreter's __main__ module in a hidden module in the other interpreter. That hidden module is used as the function __globals__. (cherry picked from commit 269e19e, AKA gh-135595) Co-authored-by: Eric Snow <[email protected]>
…-135595) As noted in the new tests, there are a few situations we must carefully accommodate for functions that get pickled during interp.call(). We do so by running the script from the main interpreter's __main__ module in a hidden module in the other interpreter. That hidden module is used as the function __globals__.
As noted in the new tests, there are a few situations we must carefully accommodate
for functions that get pickled during
interp.call()
. We do so by running the scriptfrom the main interpreter's
__main__
module in a hidden module in the otherinterpreter. That hidden module is used as the function
__globals__
.