Skip to content

Commit 780ec11

Browse files
b-passrwgk
andauthored
Make implicitly_convertable sub-interpreter and free-threading safe (#5777)
* Replace static bool with thread-specific-storage to make this code sub-interpreter and free-threading safe. * Make sure there is only one tss value in existence for this. 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. * Make set_flag in implicitly_convertible() non-copyable/movable 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. * Minor cleanup to avoid venturing into UB territory. * Experiment: Disable `~thread_specific_storage()` body when using GraalPy. * Try the suggestion to only call TSS_free if the python interpreter is still active. * Add IsFinalizing check * Put this back to having a per-template-instance static --------- Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>
1 parent 9af8adb commit 780ec11

File tree

2 files changed

+23
-11
lines changed

2 files changed

+23
-11
lines changed

include/pybind11/detail/internals.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,14 +90,21 @@ class thread_specific_storage {
9090
}
9191

9292
~thread_specific_storage() {
93-
// This destructor could be called *after* Py_Finalize(). That *SHOULD BE* fine. The
94-
// following details what happens when PyThread_tss_free is called.
95-
// PYBIND11_TLS_FREE is PyThread_tss_free on python 3.7+. On older python, it does
93+
// This destructor is often called *after* Py_Finalize(). That *SHOULD BE* fine on most
94+
// platforms. The following details what happens when PyThread_tss_free is called in
95+
// CPython. PYBIND11_TLS_FREE is PyThread_tss_free on python 3.7+. On older python, it does
9696
// nothing. PyThread_tss_free calls PyThread_tss_delete and PyMem_RawFree.
9797
// PyThread_tss_delete just calls TlsFree (on Windows) or pthread_key_delete (on *NIX).
9898
// Neither of those have anything to do with CPython internals. PyMem_RawFree *requires*
9999
// that the `key` be allocated with the CPython allocator (as it is by
100100
// PyThread_tss_create).
101+
// However, in GraalPy (as of v24.2 or older), TSS is implemented by Java and this call
102+
// requires a living Python interpreter.
103+
#ifdef GRAALVM_PYTHON
104+
if (!Py_IsInitialized() || _Py_IsFinalizing()) {
105+
return;
106+
}
107+
#endif
101108
PYBIND11_TLS_FREE(key_);
102109
}
103110

include/pybind11/pybind11.h

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3148,17 +3148,22 @@ typing::Iterator<ValueType> make_value_iterator(Type &value, Extra &&...extra) {
31483148

31493149
template <typename InputType, typename OutputType>
31503150
void implicitly_convertible() {
3151+
static int tss_sentinel_pointee = 1; // arbitrary value
31513152
struct set_flag {
3152-
bool &flag;
3153-
explicit set_flag(bool &flag_) : flag(flag_) { flag_ = true; }
3154-
~set_flag() { flag = false; }
3153+
thread_specific_storage<int> &flag;
3154+
explicit set_flag(thread_specific_storage<int> &flag_) : flag(flag_) {
3155+
flag = &tss_sentinel_pointee; // trick: the pointer itself is the sentinel
3156+
}
3157+
~set_flag() { flag.reset(nullptr); }
3158+
3159+
// Prevent copying/moving to ensure RAII guard is used safely
3160+
set_flag(const set_flag &) = delete;
3161+
set_flag(set_flag &&) = delete;
3162+
set_flag &operator=(const set_flag &) = delete;
3163+
set_flag &operator=(set_flag &&) = delete;
31553164
};
31563165
auto implicit_caster = [](PyObject *obj, PyTypeObject *type) -> PyObject * {
3157-
#ifdef Py_GIL_DISABLED
3158-
thread_local bool currently_used = false;
3159-
#else
3160-
static bool currently_used = false;
3161-
#endif
3166+
static thread_specific_storage<int> currently_used;
31623167
if (currently_used) { // implicit conversions are non-reentrant
31633168
return nullptr;
31643169
}

0 commit comments

Comments
 (0)