From 53b391900d67763efccda2990bdc492aa05022cd Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Thu, 2 Sep 2021 15:03:41 -0700
Subject: [PATCH 01/25] Fix thread safety for pybind11 loader_life_support

Fixes issue: https://github.com/pybind/pybind11/issues/2765

This converts the vector of PyObjects to either a single void* or
a per-thread void* depending on the WITH_THREAD define.

The new field is used by each thread to construct a stack
of loader_life_support frames that can extend the life of python
objects.

The pointer is updated when the loader_life_support object is allocated
(which happens before a call) as well as on release.

Each loader_life_support maintains a set of PyObject references
that need to be lifetime extended; this is done by storing them
in a c++ std::unordered_set and clearing the references when the
method completes.
---
 include/pybind11/detail/common.h           |  4 +-
 include/pybind11/detail/internals.h        | 46 ++++++++++++++++++++--
 include/pybind11/detail/type_caster_base.h | 37 +++++++----------
 3 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h
index e4f00293ec..2d9accac11 100644
--- a/include/pybind11/detail/common.h
+++ b/include/pybind11/detail/common.h
@@ -11,11 +11,11 @@
 
 #define PYBIND11_VERSION_MAJOR 2
 #define PYBIND11_VERSION_MINOR 8
-#define PYBIND11_VERSION_PATCH 0.dev1
+#define PYBIND11_VERSION_PATCH 0.dev2
 
 // Similar to Python's convention: https://docs.python.org/3/c-api/apiabiversion.html
 // Additional convention: 0xD = dev
-#define PYBIND11_VERSION_HEX 0x020800D1
+#define PYBIND11_VERSION_HEX 0x020800D2
 
 #define PYBIND11_NAMESPACE_BEGIN(name) namespace name {
 #define PYBIND11_NAMESPACE_END(name) }
diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h
index b177801a11..bcc936c8e7 100644
--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -106,14 +106,16 @@ struct internals {
     std::unordered_map<const PyObject *, std::vector<PyObject *>> patients;
     std::forward_list<ExceptionTranslator> registered_exception_translators;
     std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across extensions
-    std::vector<PyObject *> loader_patient_stack; // Used by `loader_life_support`
     std::forward_list<std::string> static_strings; // Stores the std::strings backing detail::c_str()
     PyTypeObject *static_property_type;
     PyTypeObject *default_metaclass;
     PyObject *instance_base;
-#if defined(WITH_THREAD)
+#if !defined(WITH_THREAD)
+    void* loader_patient_ptr = nullptr; // Used by `loader_life_support`
+#else // defined(WITH_THREAD)
     PYBIND11_TLS_KEY_INIT(tstate);
     PyInterpreterState *istate = nullptr;
+    PYBIND11_TLS_KEY_INIT(loader_patient_key);
     ~internals() {
         // This destructor is called *after* Py_Finalize() in finalize_interpreter().
         // That *SHOULD BE* fine. The following details what happens when PyThread_tss_free is called.
@@ -123,6 +125,7 @@ struct internals {
         // of those have anything to do with CPython internals.
         // PyMem_RawFree *requires* that the `tstate` be allocated with the CPython allocator.
         PYBIND11_TLS_FREE(tstate);
+        PYBIND11_TLS_FREE(loader_patient_key);
     }
 #endif
 };
@@ -298,15 +301,26 @@ PYBIND11_NOINLINE internals &get_internals() {
         #if PY_VERSION_HEX >= 0x03070000
             internals_ptr->tstate = PyThread_tss_alloc();
             if (!internals_ptr->tstate || (PyThread_tss_create(internals_ptr->tstate) != 0))
-                pybind11_fail("get_internals: could not successfully initialize the TSS key!");
+                pybind11_fail("get_internals: could not successfully initialize the tstate TSS key!");
             PyThread_tss_set(internals_ptr->tstate, tstate);
         #else
             internals_ptr->tstate = PyThread_create_key();
             if (internals_ptr->tstate == -1)
-                pybind11_fail("get_internals: could not successfully initialize the TLS key!");
+                pybind11_fail("get_internals: could not successfully initialize the tstate TLS key!");
             PyThread_set_key_value(internals_ptr->tstate, tstate);
         #endif
         internals_ptr->istate = tstate->interp;
+
+        #if PY_VERSION_HEX >= 0x03070000
+            internals_ptr->loader_patient_key = PyThread_tss_alloc();
+            if (!internals_ptr->loader_patient_key || (PyThread_tss_create(internals_ptr->loader_patient_key) != 0))
+                pybind11_fail("get_internals: could not successfully initialize the loader_patient TSS key!");
+        #else
+            internals_ptr->loader_patient_key = PyThread_create_key();
+            if (internals_ptr->loader_patient_key == -1)
+                pybind11_fail("get_internals: could not successfully initialize the loader_patient TLS key!");
+        #endif
+
 #endif
         builtins[id] = capsule(internals_pp);
         internals_ptr->registered_exception_translators.push_front(&translate_exception);
@@ -335,6 +349,30 @@ inline local_internals &get_local_internals() {
   return locals;
 }
 
+/// The patient pointer is used to store patient data for a call frame.
+/// See loader_life_support for use.
+inline void* get_loader_patient_pointer() {
+#if !defined(WITH_THREAD)
+  return get_internals().loader_patient_ptr;
+#else
+  auto &internals = get_internals();
+  return PYBIND11_TLS_GET_VALUE(internals.loader_patient_key);
+#endif
+}
+
+inline void set_loader_patient_pointer(void* ptr) {
+#if !defined(WITH_THREAD)
+  get_internals().loader_patient_ptr = ptr;
+#else
+  auto &internals = get_internals();
+#if PY_VERSION_HEX >= 0x03070000
+  PyThread_tss_set(internals.loader_patient_key, ptr);
+#else
+  PyThread_set_key_value(internals.loader_patient_key, ptr);
+#endif
+#endif
+}
+
 
 /// Constructs a std::string with the given arguments, stores it in `internals`, and returns its
 /// `c_str()`.  Such strings objects have a long storage duration -- the internal strings are only
diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 5a5acc2e6a..97e5e55b41 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -32,46 +32,37 @@ PYBIND11_NAMESPACE_BEGIN(detail)
 /// Adding a patient will keep it alive up until the enclosing function returns.
 class loader_life_support {
 public:
+    void* parent = nullptr;
+    std::unordered_set<PyObject *> keep_alive;
+
     /// A new patient frame is created when a function is entered
     loader_life_support() {
-        get_internals().loader_patient_stack.push_back(nullptr);
+        parent = get_loader_patient_pointer();
+        set_loader_patient_pointer(this);
     }
 
     /// ... and destroyed after it returns
     ~loader_life_support() {
-        auto &stack = get_internals().loader_patient_stack;
-        if (stack.empty())
+        auto* frame = reinterpret_cast<loader_life_support*>(get_loader_patient_pointer());
+        if (frame != this)
             pybind11_fail("loader_life_support: internal error");
+        set_loader_patient_pointer(parent);
 
-        auto ptr = stack.back();
-        stack.pop_back();
-        Py_CLEAR(ptr);
-
-        // A heuristic to reduce the stack's capacity (e.g. after long recursive calls)
-        if (stack.capacity() > 16 && !stack.empty() && stack.capacity() / stack.size() > 2)
-            stack.shrink_to_fit();
+        for (auto* item : keep_alive)
+            Py_DECREF(item);
     }
 
     /// This can only be used inside a pybind11-bound function, either by `argument_loader`
     /// at argument preparation time or by `py::cast()` at execution time.
     PYBIND11_NOINLINE static void add_patient(handle h) {
-        auto &stack = get_internals().loader_patient_stack;
-        if (stack.empty())
+        auto* frame = reinterpret_cast<loader_life_support*>(get_loader_patient_pointer());
+        if (!frame)
             throw cast_error("When called outside a bound function, py::cast() cannot "
                              "do Python -> C++ conversions which require the creation "
                              "of temporary values");
 
-        auto &list_ptr = stack.back();
-        if (list_ptr == nullptr) {
-            list_ptr = PyList_New(1);
-            if (!list_ptr)
-                pybind11_fail("loader_life_support: error allocating list");
-            PyList_SET_ITEM(list_ptr, 0, h.inc_ref().ptr());
-        } else {
-            auto result = PyList_Append(list_ptr, h.ptr());
-            if (result == -1)
-                pybind11_fail("loader_life_support: error adding patient");
-        }
+        if (frame->keep_alive.insert(h.ptr()).second)
+          Py_INCREF(h.ptr());
     }
 };
 

From caa974ff8cd3c262346907b90d15321ca36d4243 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Thu, 2 Sep 2021 15:36:14 -0700
Subject: [PATCH 02/25] Also update the internals version as the internal
 struct is no longer compatible

---
 include/pybind11/detail/internals.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h
index bcc936c8e7..edf360f194 100644
--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -157,7 +157,7 @@ struct type_info {
 };
 
 /// Tracks the `internals` and `type_info` ABI version independent of the main library version
-#define PYBIND11_INTERNALS_VERSION 4
+#define PYBIND11_INTERNALS_VERSION 5
 
 /// On MSVC, debug and release builds are not ABI-compatible!
 #if defined(_MSC_VER) && defined(_DEBUG)

From 9179d60fe1a787d4b243765133f55f36d5b8fb6e Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Thu, 2 Sep 2021 18:24:47 -0700
Subject: [PATCH 03/25] Add test demonstrating threading works correctly.

It may be appropriate to run this under msan/tsan/etc.
---
 tests/CMakeLists.txt  |  1 +
 tests/test_thread.cpp | 35 +++++++++++++++++++++++++++++++++++
 tests/test_thread.py  | 31 +++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 tests/test_thread.cpp
 create mode 100644 tests/test_thread.py

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index d71a51e6a0..f014771d54 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -129,6 +129,7 @@ set(PYBIND11_TEST_FILES
     test_stl.cpp
     test_stl_binders.cpp
     test_tagbased_polymorphic.cpp
+    test_thread.cpp
     test_union.cpp
     test_virtual_functions.cpp)
 
diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
new file mode 100644
index 0000000000..b1ff6bee06
--- /dev/null
+++ b/tests/test_thread.cpp
@@ -0,0 +1,35 @@
+/*
+    tests/test_thread.cpp -- call pybind11 bound methods in threads
+
+    Copyright (c) 2017 Laramie Leavitt (Google LLC) <lar@google.com>
+
+    All rights reserved. Use of this source code is governed by a
+    BSD-style license that can be found in the LICENSE file.
+*/
+
+#include <string_view>
+#include <string>
+
+#define PYBIND11_HAS_STRING_VIEW 1
+
+#include <pybind11/cast.h>
+#include <pybind11/pybind11.h>
+#include <pybind11/stl.h>
+
+#include "pybind11_tests.h"
+
+
+TEST_SUBMODULE(thread, m) {
+
+    // std::string_view uses loader_life_support to ensure that the string contents
+    // remains alive for the life of the call. These methods are invoked concurrently
+    m.def("method", [](std::string_view str) -> std::string { 
+        return std::string(str); 
+    });
+
+    m.def("method_no_gil", [](std::string_view str) -> std::string { 
+        return std::string(str); 
+    },
+    py::call_guard<py::gil_scoped_release>());
+
+}
diff --git a/tests/test_thread.py b/tests/test_thread.py
new file mode 100644
index 0000000000..1bedd6dadb
--- /dev/null
+++ b/tests/test_thread.py
@@ -0,0 +1,31 @@
+# -*- coding: utf-8 -*-
+import pytest
+
+import concurrent.futures
+import env  # noqa: F401
+from pybind11_tests import thread as m
+
+
+def method(s):
+   return m.method(s)
+
+def method_no_gil(s):
+   return m.method_no_gil(s)
+
+
+def test_message():
+    inputs = [ '%d' % i for i in range(20,30) ]
+    with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
+        results = list(executor.map(method, inputs))
+    results.sort()
+    for i in range(len(results)):
+        assert results[i] == ('%s' % (i + 20))
+
+
+def test_message_no_gil():
+    inputs = [ '%d' % i for i in range(20,30) ]
+    with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
+        results = list(executor.map(method_no_gil, inputs))
+    results.sort()
+    for i in range(len(results)):
+        assert results[i] == ('%s' % (i + 20))

From 0c2bf551d9ee881f933367fc7e9ae90a072ddbe6 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Fri, 3 Sep 2021 01:26:24 +0000
Subject: [PATCH 04/25] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 tests/test_thread.cpp |  8 ++++----
 tests/test_thread.py  | 16 +++++++++-------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index b1ff6bee06..6067911b9e 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -23,12 +23,12 @@ TEST_SUBMODULE(thread, m) {
 
     // std::string_view uses loader_life_support to ensure that the string contents
     // remains alive for the life of the call. These methods are invoked concurrently
-    m.def("method", [](std::string_view str) -> std::string { 
-        return std::string(str); 
+    m.def("method", [](std::string_view str) -> std::string {
+        return std::string(str);
     });
 
-    m.def("method_no_gil", [](std::string_view str) -> std::string { 
-        return std::string(str); 
+    m.def("method_no_gil", [](std::string_view str) -> std::string {
+        return std::string(str);
     },
     py::call_guard<py::gil_scoped_release>());
 
diff --git a/tests/test_thread.py b/tests/test_thread.py
index 1bedd6dadb..594be33fe6 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -1,31 +1,33 @@
 # -*- coding: utf-8 -*-
+import concurrent.futures
+
 import pytest
 
-import concurrent.futures
 import env  # noqa: F401
 from pybind11_tests import thread as m
 
 
 def method(s):
-   return m.method(s)
+    return m.method(s)
+
 
 def method_no_gil(s):
-   return m.method_no_gil(s)
+    return m.method_no_gil(s)
 
 
 def test_message():
-    inputs = [ '%d' % i for i in range(20,30) ]
+    inputs = ["%d" % i for i in range(20, 30)]
     with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
         results = list(executor.map(method, inputs))
     results.sort()
     for i in range(len(results)):
-        assert results[i] == ('%s' % (i + 20))
+        assert results[i] == ("%s" % (i + 20))
 
 
 def test_message_no_gil():
-    inputs = [ '%d' % i for i in range(20,30) ]
+    inputs = ["%d" % i for i in range(20, 30)]
     with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
         results = list(executor.map(method_no_gil, inputs))
     results.sort()
     for i in range(len(results)):
-        assert results[i] == ('%s' % (i + 20))
+        assert results[i] == ("%s" % (i + 20))

From 4b7dc7ae35d82d0af95cd1d6326512c3eb81e0ed Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 09:21:08 -0700
Subject: [PATCH 05/25] Update test to use lifetime-extended references rather
 than std::string_view, as that's a C++ 17 feature.

---
 tests/test_thread.cpp | 53 ++++++++++++++++++++++++++++++++++---------
 tests/test_thread.py  | 25 +++++---------------
 2 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index b1ff6bee06..cbee9a868a 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -7,29 +7,60 @@
     BSD-style license that can be found in the LICENSE file.
 */
 
-#include <string_view>
-#include <string>
-
-#define PYBIND11_HAS_STRING_VIEW 1
+#include <chrono>
+#include <thread>
 
 #include <pybind11/cast.h>
 #include <pybind11/pybind11.h>
-#include <pybind11/stl.h>
 
 #include "pybind11_tests.h"
 
 
+namespace py = pybind11;
+
+
+namespace {
+
+struct IntStruct {
+    int value;
+};
+
+}
+
 TEST_SUBMODULE(thread, m) {
 
-    // std::string_view uses loader_life_support to ensure that the string contents
-    // remains alive for the life of the call. These methods are invoked concurrently
-    m.def("method", [](std::string_view str) -> std::string { 
-        return std::string(str); 
+
+    py::class_<IntStruct>(m, "IntStruct")
+        .def(py::init([](const int i) { return IntStruct{i}; }));
+
+    // implicitly_convertible uses loader_life_support when an implicit
+    // conversion is required in order to llifetime extend the reference.
+    py::implicitly_convertible<int, IntStruct>();
+
+
+    m.def("test", [](const IntStruct& in) {
+        IntStruct copy = in;
+
+        {
+          py::gil_scoped_release release;
+          std::this_thread::sleep_for(std::chrono::milliseconds(5));
+        }
+
+        if (in.value != copy.value) {
+            throw std::runtime_error("Reference changed!!");
+        }
     });
 
-    m.def("method_no_gil", [](std::string_view str) -> std::string { 
-        return std::string(str); 
+   m.def("test_no_gil", [](const IntStruct& in) {
+        IntStruct copy = in;
+        std::this_thread::sleep_for(std::chrono::milliseconds(5));
+        if (in.value != copy.value) {
+            throw std::runtime_error("Reference changed!!");
+        }
     },
     py::call_guard<py::gil_scoped_release>());
 
+    // NOTE: std::string_view also uses loader_life_support to ensure that
+    // the string contents remain alive, but that's a C++ 17 feature.
 }
+
diff --git a/tests/test_thread.py b/tests/test_thread.py
index 1bedd6dadb..8209b40d39 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -6,26 +6,13 @@
 from pybind11_tests import thread as m
 
 
-def method(s):
-   return m.method(s)
-
-def method_no_gil(s):
-   return m.method_no_gil(s)
-
-
-def test_message():
-    inputs = [ '%d' % i for i in range(20,30) ]
+def test_implicit_conversion():
+    inputs = [i for i in range(20,30) ]
     with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
-        results = list(executor.map(method, inputs))
-    results.sort()
-    for i in range(len(results)):
-        assert results[i] == ('%s' % (i + 20))
+        executor.map(m.test, inputs)
 
 
-def test_message_no_gil():
-    inputs = [ '%d' % i for i in range(20,30) ]
+def test_implicit_conversion_no_gil():
+    inputs = [i for i in range(20,30) ]
     with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
-        results = list(executor.map(method_no_gil, inputs))
-    results.sort()
-    for i in range(len(results)):
-        assert results[i] == ('%s' % (i + 20))
+        executor.map(m.test_no_gil, inputs)

From d91a4a7552b40a8477a7213eeb91710d548586a7 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Fri, 3 Sep 2021 16:29:19 +0000
Subject: [PATCH 06/25] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 tests/test_thread.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index 656f1dca78..93247633f6 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -8,12 +8,12 @@
 
 
 def test_implicit_conversion():
-    inputs = [i for i in range(20,30) ]
+    inputs = [i for i in range(20, 30)]
     with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
         executor.map(m.test, inputs)
 
 
 def test_implicit_conversion_no_gil():
-    inputs = [i for i in range(20,30) ]
+    inputs = [i for i in range(20, 30)]
     with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
         executor.map(m.test_no_gil, inputs)

From c6720ca463369c8cbf3a5860786a5d1cf9a32a52 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 09:37:55 -0700
Subject: [PATCH 07/25] Make loader_life_support members private

---
 include/pybind11/detail/type_caster_base.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 97e5e55b41..da070836c1 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -31,10 +31,11 @@ PYBIND11_NAMESPACE_BEGIN(detail)
 /// A life support system for temporary objects created by `type_caster::load()`.
 /// Adding a patient will keep it alive up until the enclosing function returns.
 class loader_life_support {
-public:
+private:
     void* parent = nullptr;
     std::unordered_set<PyObject *> keep_alive;
 
+public:
     /// A new patient frame is created when a function is entered
     loader_life_support() {
         parent = get_loader_patient_pointer();
@@ -47,7 +48,6 @@ class loader_life_support {
         if (frame != this)
             pybind11_fail("loader_life_support: internal error");
         set_loader_patient_pointer(parent);
-
         for (auto* item : keep_alive)
             Py_DECREF(item);
     }
@@ -62,7 +62,7 @@ class loader_life_support {
                              "of temporary values");
 
         if (frame->keep_alive.insert(h.ptr()).second)
-          Py_INCREF(h.ptr());
+            Py_INCREF(h.ptr());
     }
 };
 

From 8a1a59f47f05e46628044d4ff1fada156e55dbf7 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 10:51:59 -0700
Subject: [PATCH 08/25] Update version to dev2

---
 pybind11/_version.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pybind11/_version.py b/pybind11/_version.py
index 610d39bf1c..d212f1dfb7 100644
--- a/pybind11/_version.py
+++ b/pybind11/_version.py
@@ -8,5 +8,5 @@ def _to_int(s):
         return s
 
 
-__version__ = "2.8.0.dev1"
+__version__ = "2.8.0.dev2"
 version_info = tuple(_to_int(s) for s in __version__.split("."))

From dfc94f3c120beeed346e178d900564b270bf2a13 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 11:09:49 -0700
Subject: [PATCH 09/25] Update test to use python threading rather than
 concurrent.futures

---
 tests/test_thread.py | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index 93247633f6..ce5b0361e5 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -1,19 +1,35 @@
 # -*- coding: utf-8 -*-
-import concurrent.futures
 
 import pytest
+import threading
 
 import env  # noqa: F401
 from pybind11_tests import thread as m
 
 
 def test_implicit_conversion():
-    inputs = [i for i in range(20, 30)]
-    with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
-        executor.map(m.test, inputs)
+    def loop(count):
+        for i in range(count):
+            m.test(i)
+
+    a = threading.Thread(target=loop, args=(10,))
+    b = threading.Thread(target=loop, args=(10,))
+    c = threading.Thread(target=loop, args=(10,)) 
+    for x in [a,b,c]:
+        x.start()
+    for x in [c,b,a]:
+        x.join()
 
 
 def test_implicit_conversion_no_gil():
-    inputs = [i for i in range(20, 30)]
-    with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
-        executor.map(m.test_no_gil, inputs)
+    def loop(count):
+        for i in range(count):
+            m.test_no_gil(i)
+
+    a = threading.Thread(target=loop, args=(10,))
+    b = threading.Thread(target=loop, args=(10,))
+    c = threading.Thread(target=loop, args=(10,)) 
+    for x in [a,b,c]:
+        x.start()
+    for x in [c,b,a]:
+        x.join()

From 4f25e31ae01d96c9a2131471825c413dac5934a2 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Fri, 3 Sep 2021 18:10:29 +0000
Subject: [PATCH 10/25] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 tests/test_thread.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index ce5b0361e5..efc735f9d0 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -1,8 +1,9 @@
 # -*- coding: utf-8 -*-
 
-import pytest
 import threading
 
+import pytest
+
 import env  # noqa: F401
 from pybind11_tests import thread as m
 
@@ -14,10 +15,10 @@ def loop(count):
 
     a = threading.Thread(target=loop, args=(10,))
     b = threading.Thread(target=loop, args=(10,))
-    c = threading.Thread(target=loop, args=(10,)) 
-    for x in [a,b,c]:
+    c = threading.Thread(target=loop, args=(10,))
+    for x in [a, b, c]:
         x.start()
-    for x in [c,b,a]:
+    for x in [c, b, a]:
         x.join()
 
 
@@ -28,8 +29,8 @@ def loop(count):
 
     a = threading.Thread(target=loop, args=(10,))
     b = threading.Thread(target=loop, args=(10,))
-    c = threading.Thread(target=loop, args=(10,)) 
-    for x in [a,b,c]:
+    c = threading.Thread(target=loop, args=(10,))
+    for x in [a, b, c]:
         x.start()
-    for x in [c,b,a]:
+    for x in [c, b, a]:
         x.join()

From 366f40d35a80e5fbfe5c9eb20f05bb367136cef7 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 11:22:22 -0700
Subject: [PATCH 11/25] Remove unnecessary env in test

---
 tests/test_thread.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index efc735f9d0..f9854ddd55 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -4,7 +4,6 @@
 
 import pytest
 
-import env  # noqa: F401
 from pybind11_tests import thread as m
 
 

From b5a0538570465d6e1cd555ad2f82985797f7f349 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 11:23:54 -0700
Subject: [PATCH 12/25] Remove unnecessary pytest in test

---
 tests/test_thread.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index f9854ddd55..d59c78086a 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -2,8 +2,6 @@
 
 import threading
 
-import pytest
-
 from pybind11_tests import thread as m
 
 

From ffc52a34b43437b82ecb24f7334633a704ee74fc Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 12:30:04 -0700
Subject: [PATCH 13/25] Use native C++ thread_local in place of python
 per-thread data structures to retain compatability

---
 include/pybind11/detail/internals.h        | 45 ++--------------------
 include/pybind11/detail/type_caster_base.h | 25 ++++++++----
 2 files changed, 21 insertions(+), 49 deletions(-)

diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h
index edf360f194..f11d2beffe 100644
--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -106,16 +106,14 @@ struct internals {
     std::unordered_map<const PyObject *, std::vector<PyObject *>> patients;
     std::forward_list<ExceptionTranslator> registered_exception_translators;
     std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across extensions
+    std::vector<PyObject *> unused_loader_patient_stack_remove_at_v5;
     std::forward_list<std::string> static_strings; // Stores the std::strings backing detail::c_str()
     PyTypeObject *static_property_type;
     PyTypeObject *default_metaclass;
     PyObject *instance_base;
-#if !defined(WITH_THREAD)
-    void* loader_patient_ptr = nullptr; // Used by `loader_life_support`
-#else // defined(WITH_THREAD)
+#if defined(WITH_THREAD)
     PYBIND11_TLS_KEY_INIT(tstate);
     PyInterpreterState *istate = nullptr;
-    PYBIND11_TLS_KEY_INIT(loader_patient_key);
     ~internals() {
         // This destructor is called *after* Py_Finalize() in finalize_interpreter().
         // That *SHOULD BE* fine. The following details what happens when PyThread_tss_free is called.
@@ -125,7 +123,6 @@ struct internals {
         // of those have anything to do with CPython internals.
         // PyMem_RawFree *requires* that the `tstate` be allocated with the CPython allocator.
         PYBIND11_TLS_FREE(tstate);
-        PYBIND11_TLS_FREE(loader_patient_key);
     }
 #endif
 };
@@ -157,7 +154,7 @@ struct type_info {
 };
 
 /// Tracks the `internals` and `type_info` ABI version independent of the main library version
-#define PYBIND11_INTERNALS_VERSION 5
+#define PYBIND11_INTERNALS_VERSION 4
 
 /// On MSVC, debug and release builds are not ABI-compatible!
 #if defined(_MSC_VER) && defined(_DEBUG)
@@ -310,17 +307,6 @@ PYBIND11_NOINLINE internals &get_internals() {
             PyThread_set_key_value(internals_ptr->tstate, tstate);
         #endif
         internals_ptr->istate = tstate->interp;
-
-        #if PY_VERSION_HEX >= 0x03070000
-            internals_ptr->loader_patient_key = PyThread_tss_alloc();
-            if (!internals_ptr->loader_patient_key || (PyThread_tss_create(internals_ptr->loader_patient_key) != 0))
-                pybind11_fail("get_internals: could not successfully initialize the loader_patient TSS key!");
-        #else
-            internals_ptr->loader_patient_key = PyThread_create_key();
-            if (internals_ptr->loader_patient_key == -1)
-                pybind11_fail("get_internals: could not successfully initialize the loader_patient TLS key!");
-        #endif
-
 #endif
         builtins[id] = capsule(internals_pp);
         internals_ptr->registered_exception_translators.push_front(&translate_exception);
@@ -349,31 +335,6 @@ inline local_internals &get_local_internals() {
   return locals;
 }
 
-/// The patient pointer is used to store patient data for a call frame.
-/// See loader_life_support for use.
-inline void* get_loader_patient_pointer() {
-#if !defined(WITH_THREAD)
-  return get_internals().loader_patient_ptr;
-#else
-  auto &internals = get_internals();
-  return PYBIND11_TLS_GET_VALUE(internals.loader_patient_key);
-#endif
-}
-
-inline void set_loader_patient_pointer(void* ptr) {
-#if !defined(WITH_THREAD)
-  get_internals().loader_patient_ptr = ptr;
-#else
-  auto &internals = get_internals();
-#if PY_VERSION_HEX >= 0x03070000
-  PyThread_tss_set(internals.loader_patient_key, ptr);
-#else
-  PyThread_set_key_value(internals.loader_patient_key, ptr);
-#endif
-#endif
-}
-
-
 /// Constructs a std::string with the given arguments, stores it in `internals`, and returns its
 /// `c_str()`.  Such strings objects have a long storage duration -- the internal strings are only
 /// cleared when the program exits or after interpreter shutdown (when embedding), and so are
diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index da070836c1..5410c7dc1a 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -28,26 +28,36 @@
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 PYBIND11_NAMESPACE_BEGIN(detail)
 
+
 /// A life support system for temporary objects created by `type_caster::load()`.
 /// Adding a patient will keep it alive up until the enclosing function returns.
 class loader_life_support {
 private:
-    void* parent = nullptr;
+    loader_life_support* parent = nullptr;
     std::unordered_set<PyObject *> keep_alive;
 
+    static loader_life_support** get_stack_pp() {
+#if defined(WITH_THREAD)
+        thread_local static loader_life_support* per_thread_stack = nullptr;
+        return &per_thread_stack;
+#else
+        static loader_life_support* global_stack = nullptr;
+        return &global_stack;
+#endif
+    }
+
 public:
     /// A new patient frame is created when a function is entered
     loader_life_support() {
-        parent = get_loader_patient_pointer();
-        set_loader_patient_pointer(this);
+        parent = *get_stack_pp();
+        *get_stack_pp() = this;
     }
 
     /// ... and destroyed after it returns
     ~loader_life_support() {
-        auto* frame = reinterpret_cast<loader_life_support*>(get_loader_patient_pointer());
-        if (frame != this)
+        if (*get_stack_pp() != this)
             pybind11_fail("loader_life_support: internal error");
-        set_loader_patient_pointer(parent);
+        *get_stack_pp() = parent;
         for (auto* item : keep_alive)
             Py_DECREF(item);
     }
@@ -55,7 +65,7 @@ class loader_life_support {
     /// This can only be used inside a pybind11-bound function, either by `argument_loader`
     /// at argument preparation time or by `py::cast()` at execution time.
     PYBIND11_NOINLINE static void add_patient(handle h) {
-        auto* frame = reinterpret_cast<loader_life_support*>(get_loader_patient_pointer());
+        loader_life_support* frame =  *get_stack_pp();
         if (!frame)
             throw cast_error("When called outside a bound function, py::cast() cannot "
                              "do Python -> C++ conversions which require the creation "
@@ -66,6 +76,7 @@ class loader_life_support {
     }
 };
 
+
 // Gets the cache entry for the given type, creating it if necessary.  The return value is the pair
 // returned by emplace, i.e. an iterator for the entry and a bool set to `true` if the entry was
 // just created.

From dc7df664fa179317fe110085626df81829cc9484 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 12:41:08 -0700
Subject: [PATCH 14/25] clang-format test_thread.cpp

---
 tests/test_thread.cpp | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index f2df2d86e8..7c8d54757d 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -15,34 +15,30 @@
 
 #include "pybind11_tests.h"
 
-
 namespace py = pybind11;
 
-
 namespace {
 
 struct IntStruct {
     int value;
 };
 
-}
+} // namespace
 
 TEST_SUBMODULE(thread, m) {
 
-    py::class_<IntStruct>(m, "IntStruct")
-        .def(py::init([](const int i) { return IntStruct{i}; }));
+    py::class_<IntStruct>(m, "IntStruct").def(py::init([](const int i) { return IntStruct{i}; }));
 
     // implicitly_convertible uses loader_life_support when an implicit
     // conversion is required in order to llifetime extend the reference.
     py::implicitly_convertible<int, IntStruct>();
 
-
-    m.def("test", [](const IntStruct& in) {
+    m.def("test", [](const IntStruct &in) {
         IntStruct copy = in;
 
         {
-          py::gil_scoped_release release;
-          std::this_thread::sleep_for(std::chrono::milliseconds(5));
+            py::gil_scoped_release release;
+            std::this_thread::sleep_for(std::chrono::milliseconds(5));
         }
 
         if (in.value != copy.value) {
@@ -50,14 +46,16 @@ TEST_SUBMODULE(thread, m) {
         }
     });
 
-   m.def("test_no_gil", [](const IntStruct& in) {
-        IntStruct copy = in;
-        std::this_thread::sleep_for(std::chrono::milliseconds(5));
-        if (in.value != copy.value) {
-            throw std::runtime_error("Reference changed!!");
-        }
-    },
-    py::call_guard<py::gil_scoped_release>());
+    m.def(
+        "test_no_gil",
+        [](const IntStruct &in) {
+            IntStruct copy = in;
+            std::this_thread::sleep_for(std::chrono::milliseconds(5));
+            if (in.value != copy.value) {
+                throw std::runtime_error("Reference changed!!");
+            }
+        },
+        py::call_guard<py::gil_scoped_release>());
 
     // NOTE: std::string_view also uses loader_life_support to ensure that
     // the string contents remain alive, but that's a C++ 17 feature.

From dd8f2641440c3cc0ce78c4c5e39717baf8f2337d Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Fri, 3 Sep 2021 13:48:27 -0700
Subject: [PATCH 15/25] Add a note about debugging the py::cast() error

---
 include/pybind11/detail/type_caster_base.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 5410c7dc1a..f01a260dfa 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -66,10 +66,15 @@ class loader_life_support {
     /// at argument preparation time or by `py::cast()` at execution time.
     PYBIND11_NOINLINE static void add_patient(handle h) {
         loader_life_support* frame =  *get_stack_pp();
-        if (!frame)
+        if (!frame) {
+            // NOTE: It would be nice to include the stack frames here, as this indicates
+            // use of pybind11::cast<> outside the normal call framework, finding such
+            // a location is challenging. Developers could consider printing out
+            // stack frame addresses here using something like __builtin_frame_address(0)
             throw cast_error("When called outside a bound function, py::cast() cannot "
                              "do Python -> C++ conversions which require the creation "
                              "of temporary values");
+        }
 
         if (frame->keep_alive.insert(h.ptr()).second)
             Py_INCREF(h.ptr());

From c4c6acbcd87e57be3d1d15ddf9f96cd55d4d43b6 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Tue, 7 Sep 2021 11:26:11 -0700
Subject: [PATCH 16/25] thread_test.py now propagates exceptions on join()
 calls.

---
 tests/test_thread.py | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index d59c78086a..8c65d79842 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -1,18 +1,34 @@
 # -*- coding: utf-8 -*-
 
 import threading
+import sys
 
 from pybind11_tests import thread as m
 
 
-def test_implicit_conversion():
-    def loop(count):
-        for i in range(count):
-            m.test(i)
+class Thread(threading.Thread):
+  def __init__(self, fn):
+    super(Thread, self).__init__()
+    self.fn = fn
+    self.e = None
+
+  def run(self):
+    try:
+        for i in range(10):
+            self.fn(i)
+    except Exception as e:
+        self.e = e
+
+  def join(self):
+    super(Thread, self).join()
+    if self.e:
+        raise self.e
 
-    a = threading.Thread(target=loop, args=(10,))
-    b = threading.Thread(target=loop, args=(10,))
-    c = threading.Thread(target=loop, args=(10,))
+
+def test_implicit_conversion():
+    a = Thread(m.test)
+    b = Thread(m.test)
+    c = Thread(m.test)
     for x in [a, b, c]:
         x.start()
     for x in [c, b, a]:
@@ -20,13 +36,9 @@ def loop(count):
 
 
 def test_implicit_conversion_no_gil():
-    def loop(count):
-        for i in range(count):
-            m.test_no_gil(i)
-
-    a = threading.Thread(target=loop, args=(10,))
-    b = threading.Thread(target=loop, args=(10,))
-    c = threading.Thread(target=loop, args=(10,))
+    a = Thread(m.test_no_gil)
+    b = Thread(m.test_no_gil)
+    c = Thread(m.test_no_gil)
     for x in [a, b, c]:
         x.start()
     for x in [c, b, a]:

From 6ad3de6a0306249eae817044360e03a94607c449 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Tue, 7 Sep 2021 18:28:09 +0000
Subject: [PATCH 17/25] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 tests/test_thread.py | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index 8c65d79842..712ca423c0 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -1,28 +1,28 @@
 # -*- coding: utf-8 -*-
 
-import threading
 import sys
+import threading
 
 from pybind11_tests import thread as m
 
 
 class Thread(threading.Thread):
-  def __init__(self, fn):
-    super(Thread, self).__init__()
-    self.fn = fn
-    self.e = None
-
-  def run(self):
-    try:
-        for i in range(10):
-            self.fn(i)
-    except Exception as e:
-        self.e = e
-
-  def join(self):
-    super(Thread, self).join()
-    if self.e:
-        raise self.e
+    def __init__(self, fn):
+        super(Thread, self).__init__()
+        self.fn = fn
+        self.e = None
+
+    def run(self):
+        try:
+            for i in range(10):
+                self.fn(i)
+        except Exception as e:
+            self.e = e
+
+    def join(self):
+        super(Thread, self).join()
+        if self.e:
+            raise self.e
 
 
 def test_implicit_conversion():

From d7e306721096158da3d28a22c305fae5dd1ecbe1 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Tue, 7 Sep 2021 11:33:09 -0700
Subject: [PATCH 18/25] remove unused sys / merge

---
 tests/test_thread.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test_thread.py b/tests/test_thread.py
index 712ca423c0..16e107705c 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -1,6 +1,5 @@
 # -*- coding: utf-8 -*-
 
-import sys
 import threading
 
 from pybind11_tests import thread as m

From 638d091081a986b34ceef9070f81c00af7832b16 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Wed, 8 Sep 2021 22:06:05 -0700
Subject: [PATCH 19/25] Update include order in test_thread.cpp

---
 tests/test_thread.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index 7c8d54757d..5b4056e23a 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -7,12 +7,12 @@
     BSD-style license that can be found in the LICENSE file.
 */
 
-#include <chrono>
-#include <thread>
-
 #include <pybind11/cast.h>
 #include <pybind11/pybind11.h>
 
+#include <chrono>
+#include <thread>
+
 #include "pybind11_tests.h"
 
 namespace py = pybind11;

From a06f851739f72881a2d6fea49d06dd56bf9c06b6 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Wed, 8 Sep 2021 22:07:54 -0700
Subject: [PATCH 20/25] Remove spurious whitespace

---
 include/pybind11/detail/type_caster_base.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index f01a260dfa..5d25723d25 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -28,7 +28,6 @@
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 PYBIND11_NAMESPACE_BEGIN(detail)
 
-
 /// A life support system for temporary objects created by `type_caster::load()`.
 /// Adding a patient will keep it alive up until the enclosing function returns.
 class loader_life_support {
@@ -81,7 +80,6 @@ class loader_life_support {
     }
 };
 
-
 // Gets the cache entry for the given type, creating it if necessary.  The return value is the pair
 // returned by emplace, i.e. an iterator for the entry and a bool set to `true` if the entry was
 // just created.

From 5c58953c4e72f77763c71c12d320f1f000a9701b Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Wed, 8 Sep 2021 22:10:33 -0700
Subject: [PATCH 21/25] Update comment / whitespace.

---
 include/pybind11/detail/internals.h | 1 +
 tests/test_thread.cpp               | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h
index f11d2beffe..39c28e7732 100644
--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -335,6 +335,7 @@ inline local_internals &get_local_internals() {
   return locals;
 }
 
+
 /// Constructs a std::string with the given arguments, stores it in `internals`, and returns its
 /// `c_str()`.  Such strings objects have a long storage duration -- the internal strings are only
 /// cleared when the program exits or after interpreter shutdown (when embedding), and so are
diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index 5b4056e23a..b05351808a 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -1,7 +1,7 @@
 /*
     tests/test_thread.cpp -- call pybind11 bound methods in threads
 
-    Copyright (c) 2017 Laramie Leavitt (Google LLC) <lar@google.com>
+    Copyright (c) 2021 Laramie Leavitt (Google LLC) <lar@google.com>
 
     All rights reserved. Use of this source code is governed by a
     BSD-style license that can be found in the LICENSE file.

From 5f668552a369270b1ecc0b955cadb091e551048b Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Thu, 9 Sep 2021 10:06:11 -0700
Subject: [PATCH 22/25] Address review comments

---
 include/pybind11/detail/type_caster_base.h | 10 +++++----
 tests/test_thread.cpp                      | 24 +++++++++++++---------
 tests/test_thread.py                       |  2 +-
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 5d25723d25..542947a6e4 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -48,15 +48,17 @@ class loader_life_support {
 public:
     /// A new patient frame is created when a function is entered
     loader_life_support() {
-        parent = *get_stack_pp();
-        *get_stack_pp() = this;
+        loader_life_support** stack = get_stack_pp();
+        parent = *stack;
+        *stack = this;
     }
 
     /// ... and destroyed after it returns
     ~loader_life_support() {
-        if (*get_stack_pp() != this)
+        loader_life_support** stack = get_stack_pp();
+        if (*stack != this)
             pybind11_fail("loader_life_support: internal error");
-        *get_stack_pp() = parent;
+        *stack = parent;
         for (auto* item : keep_alive)
             Py_DECREF(item);
     }
diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index b05351808a..4fede1d3ca 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -20,6 +20,11 @@ namespace py = pybind11;
 namespace {
 
 struct IntStruct {
+    IntStruct(int v): value(v) {}
+    ~IntStruct() { value = -value; }
+    IntStruct(const IntStruct&) = default;
+    IntStruct& operator=(const IntStruct&) = default;
+
     int value;
 };
 
@@ -30,29 +35,28 @@ TEST_SUBMODULE(thread, m) {
     py::class_<IntStruct>(m, "IntStruct").def(py::init([](const int i) { return IntStruct{i}; }));
 
     // implicitly_convertible uses loader_life_support when an implicit
-    // conversion is required in order to llifetime extend the reference.
+    // conversion is required in order to lifetime extend the reference.
+    //
+    // This test should be run with ASAN for better effectiveness.
     py::implicitly_convertible<int, IntStruct>();
 
-    m.def("test", [](const IntStruct &in) {
-        IntStruct copy = in;
-
+    m.def("test", [](int expected, const IntStruct &in) {
         {
             py::gil_scoped_release release;
             std::this_thread::sleep_for(std::chrono::milliseconds(5));
         }
 
-        if (in.value != copy.value) {
-            throw std::runtime_error("Reference changed!!");
+        if (in.value != expected) {
+            throw std::runtime_error("Value changed!!");
         }
     });
 
     m.def(
         "test_no_gil",
-        [](const IntStruct &in) {
-            IntStruct copy = in;
+        [](int expected, const IntStruct &in) {
             std::this_thread::sleep_for(std::chrono::milliseconds(5));
-            if (in.value != copy.value) {
-                throw std::runtime_error("Reference changed!!");
+            if (in.value != expected) {
+                throw std::runtime_error("Value changed!!");
             }
         },
         py::call_guard<py::gil_scoped_release>());
diff --git a/tests/test_thread.py b/tests/test_thread.py
index 16e107705c..f9db1babaf 100644
--- a/tests/test_thread.py
+++ b/tests/test_thread.py
@@ -14,7 +14,7 @@ def __init__(self, fn):
     def run(self):
         try:
             for i in range(10):
-                self.fn(i)
+                self.fn(i, i)
         except Exception as e:
             self.e = e
 

From 1237bbec50ac5e225078e8c313252db05961ed2a Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Thu, 9 Sep 2021 10:24:04 -0700
Subject: [PATCH 23/25] lint cleanup

---
 tests/test_thread.cpp | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index 4fede1d3ca..e742bdcd2f 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -20,7 +20,6 @@ namespace py = pybind11;
 namespace {
 
 struct IntStruct {
-    IntStruct(int v): value(v) {}
     ~IntStruct() { value = -value; }
     IntStruct(const IntStruct&) = default;
     IntStruct& operator=(const IntStruct&) = default;

From afbc06648917e50e382ce1ef43624dfec05ccd35 Mon Sep 17 00:00:00 2001
From: Laramie Leavitt <lar@google.com>
Date: Thu, 9 Sep 2021 12:57:34 -0700
Subject: [PATCH 24/25] Fix test IntStruct constructor.

---
 tests/test_thread.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index e742bdcd2f..3643488e15 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -20,6 +20,7 @@ namespace py = pybind11;
 namespace {
 
 struct IntStruct {
+    IntStruct(int v) : value(v) {};
     ~IntStruct() { value = -value; }
     IntStruct(const IntStruct&) = default;
     IntStruct& operator=(const IntStruct&) = default;
@@ -31,7 +32,7 @@ struct IntStruct {
 
 TEST_SUBMODULE(thread, m) {
 
-    py::class_<IntStruct>(m, "IntStruct").def(py::init([](const int i) { return IntStruct{i}; }));
+    py::class_<IntStruct>(m, "IntStruct").def(py::init([](const int i) { return IntStruct(i); }));
 
     // implicitly_convertible uses loader_life_support when an implicit
     // conversion is required in order to lifetime extend the reference.

From 57871047c5b5156bab10f60edd96b5214516758b Mon Sep 17 00:00:00 2001
From: Aaron Gokaslan <skylion.aaron@gmail.com>
Date: Fri, 10 Sep 2021 10:26:28 -0400
Subject: [PATCH 25/25] Add explicit to constructor

---
 tests/test_thread.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp
index 3643488e15..19d91768b3 100644
--- a/tests/test_thread.cpp
+++ b/tests/test_thread.cpp
@@ -20,7 +20,7 @@ namespace py = pybind11;
 namespace {
 
 struct IntStruct {
-    IntStruct(int v) : value(v) {};
+    explicit IntStruct(int v) : value(v) {};
     ~IntStruct() { value = -value; }
     IntStruct(const IntStruct&) = default;
     IntStruct& operator=(const IntStruct&) = default;