fix: make implicitly_convertable sub-interpreter and free-threading safe #5777
+23
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…b-interpreter and free-threading safe.
The previous code had multiple (one for every type pair, as this is a template function), which may have posed a problem for some platforms.
|
This change causes GraalPy to fail at shutdown (apparently trying to call a Python function after python has ended). I don't really have idea idea why this change would cause a problem like this. All the tests pass (before the shutdown failure). (Also note, several of the other failures this past run are from github giving 502 errors, but the GraalPy failures are real.) |
set_flag is an RAII guard for a thread-specific reentrancy flag. Copying or moving it would risk double-resetting or rearming the flag, breaking the protection. Disable copy/move constructors and assignment operators to make this explicit.
|
Hi @b-pass, I added two minor commits, by-products of me trying to understand this change. Please feel free to undo what doesn't make sense to you. Regarding the GraalPy failures, could you please look here? https://chatgpt.com/share/688854b1-8b00-8008-8d34-9d3f0d77926a Warning: There is quite a bit of weirdness in that conversation. You might want to ignore most of it. However, could you please look for
There are also Option B and Option C. Do any of those have merit? Another idea, for poking around until we hopefully hit on something: --- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -98,7 +98,9 @@ public:
// Neither of those have anything to do with CPython internals. PyMem_RawFree *requires*
// that the `key` be allocated with the CPython allocator (as it is by
// PyThread_tss_create).
+#if !defined(GRAALVM_PYTHON)
PYBIND11_TLS_FREE(key_);
+#endif
}
thread_specific_storage(thread_specific_storage const &) = delete;Oh, the CI finished just now, with 3 graalpy failures, and one pesky unrelated failure that we can ignore (🐍 (macos-13, 3.13t, -DCMAKE_CXX_STANDARD=11) / 🧪). I'll try that diff now to see what happens. |
|
In GraalPy, the TSS is implemented in Java, so it needs the JVM to be somewhat alive to do these calls. I think the proper way would be to check |
rwgk
reviewed
Jul 30, 2025
include/pybind11/detail/internals.h
Outdated
| // However, in GraalPy (as of v24.2 or older), TSS is implemented by Java and this call | ||
| // requires a living Python interpreter. | ||
| #ifdef GRAALVM_PYTHON | ||
| if (!Py_IsInitialized()) { |
There was a problem hiding this comment.
From old battles like here I remembered:
if (!Py_IsInitialized() || Py_Finalizing()) {
When asked about this, ChatGPT suggested:
Why
Py_IsInitialized()may not be sufficient
Py_IsInitialized()only checks whether the global interpreter state pointer is non-null.During finalization,
Py_IsInitialized()will remain true until very late, even after large parts of the runtime are already torn down.Accessing some Python APIs during this late stage (especially in alternative runtimes like GraalPy) can trigger undefined behavior or crashes.
WDYT?
|
@colesbury Could you please help with a review of this (small) PR? It touches code you added with PR #5148 (now here). |
rwgk
approved these changes
Jul 31, 2025
colesbury
reviewed
Aug 1, 2025
rwgk
approved these changes
Aug 2, 2025
henryiii
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This code in
implicitly_convertible()was using astatic boolwhich is not sub-interpreter safe.The free-threading code attempted to make this thread safe by making it a
thread_local, butthread_localdoes not work on some older macOS targets, so the correct way to fix this is to use thread-specific-storage.Suggested changelog entry:
implicitly_convertible()