-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix crash in gil_scoped_acquire
#5828
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Rostan Tabet <rtabet@nvidia.com>
Signed-off-by: Rostan Tabet <rtabet@nvidia.com>
d2863f5
to
c11b22e
Compare
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 just added this a couple days ago (PR #5822), to git rid of very frequent failures under macOS / free-threaded that nobody had the time to debug:
pybind11/tests/test_gil_scoped.py
Lines 205 to 209 in 852a4b5
if env.MACOS: | |
if not env.sys_is_gil_enabled(): | |
pytest.xfail(f"[TEST-GIL-SCOPED] {soabi} with GIL disabled: " + msg) | |
if env.PY_GIL_DISABLED: | |
pytest.xfail(f"[TEST-GIL-SCOPED] {soabi}: " + msg) |
WDYT about removing that in this PR? — So that we see immediately if your fix helps with that, too. If not, we can put that code back in a follow-on PR.
#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include(<barrier>) | ||
# define PYBIND11_HAS_BARRIER 1 | ||
# include <barrier> | ||
#endif | ||
|
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.
Could you please move this somewhere around here?
pybind11/include/pybind11/detail/common.h
Line 97 in 852a4b5
I think PYBIND11_HAS_STD_BARRIER
would be ideal (similar to the PYBIND11_HAS_STD_LAUNDER
that we have already).
# endif | ||
// Make sure that PyThreadState_Clear is not recursively called by finalizers. | ||
// See issue #5827 | ||
++tstate->gilstate_counter; |
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.
Totally not sure: Is there any point in guarding this with #if defined(...)
for certain Python versions and Py_GIL_DISABLED
?
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 focused on free-threading here but I think that the issue can also occur in destructors of thread-local variables in the regular build.
}); | ||
#endif | ||
|
||
m.attr("has_barrier") = |
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.
In other places we use a pattern that would be defined_PYBIND11_HAS_STD_BARRIER
here.
(It makes it a little easier to pin-point all things connected to PYBIND11_HAS_STD_BARRIER
across Python and C++ code.)
FWIW, this LGTM |
Description
Increase
gilstate_counter
before callingPyThreadState_Clear
ingil_scoped_acquire
to prevent finalizers to try to create and destroy a new thread state while the current thread state is being cleared.Fixes #5827
Suggested changelog entry:
Fix crash that can occur when finalizers acquire and release the GIL.