Fix thread safety for pybind11 loader_life_support (#3237)

* 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.

* Also update the internals version as the internal struct is no longer compatible

* Add test demonstrating threading works correctly.

It may be appropriate to run this under msan/tsan/etc.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update test to use lifetime-extended references rather than
std::string_view, as that's a C++ 17 feature.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Make loader_life_support members private

* Update version to dev2

* Update test to use python threading rather than concurrent.futures

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Remove unnecessary env in test

* Remove unnecessary pytest in test

* Use native C++ thread_local in place of python per-thread data structures to retain compatability

* clang-format test_thread.cpp

* Add a note about debugging the py::cast() error

* thread_test.py now propagates exceptions on join() calls.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove unused sys / merge

* Update include order in test_thread.cpp

* Remove spurious whitespace

* Update comment / whitespace.

* Address review comments

* lint cleanup

* Fix test IntStruct constructor.

* Add explicit to constructor

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
This commit is contained in:
Laramie Leavitt 2021-09-10 09:29:21 -07:00 committed by GitHub
parent 121b91f99c
commit 0e599589fe
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 148 additions and 30 deletions

View File

@ -11,11 +11,11 @@
#define PYBIND11_VERSION_MAJOR 2 #define PYBIND11_VERSION_MAJOR 2
#define PYBIND11_VERSION_MINOR 8 #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 // Similar to Python's convention: https://docs.python.org/3/c-api/apiabiversion.html
// Additional convention: 0xD = dev // Additional convention: 0xD = dev
#define PYBIND11_VERSION_HEX 0x020800D1 #define PYBIND11_VERSION_HEX 0x020800D2
#define PYBIND11_NAMESPACE_BEGIN(name) namespace name { #define PYBIND11_NAMESPACE_BEGIN(name) namespace name {
#define PYBIND11_NAMESPACE_END(name) } #define PYBIND11_NAMESPACE_END(name) }

View File

@ -106,7 +106,7 @@ struct internals {
std::unordered_map<const PyObject *, std::vector<PyObject *>> patients; std::unordered_map<const PyObject *, std::vector<PyObject *>> patients;
std::forward_list<ExceptionTranslator> registered_exception_translators; std::forward_list<ExceptionTranslator> registered_exception_translators;
std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across extensions 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::vector<PyObject *> unused_loader_patient_stack_remove_at_v5;
std::forward_list<std::string> static_strings; // Stores the std::strings backing detail::c_str() std::forward_list<std::string> static_strings; // Stores the std::strings backing detail::c_str()
PyTypeObject *static_property_type; PyTypeObject *static_property_type;
PyTypeObject *default_metaclass; PyTypeObject *default_metaclass;
@ -298,12 +298,12 @@ PYBIND11_NOINLINE internals &get_internals() {
#if PY_VERSION_HEX >= 0x03070000 #if PY_VERSION_HEX >= 0x03070000
internals_ptr->tstate = PyThread_tss_alloc(); internals_ptr->tstate = PyThread_tss_alloc();
if (!internals_ptr->tstate || (PyThread_tss_create(internals_ptr->tstate) != 0)) 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); PyThread_tss_set(internals_ptr->tstate, tstate);
#else #else
internals_ptr->tstate = PyThread_create_key(); internals_ptr->tstate = PyThread_create_key();
if (internals_ptr->tstate == -1) 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); PyThread_set_key_value(internals_ptr->tstate, tstate);
#endif #endif
internals_ptr->istate = tstate->interp; internals_ptr->istate = tstate->interp;

View File

@ -31,47 +31,54 @@ PYBIND11_NAMESPACE_BEGIN(detail)
/// A life support system for temporary objects created by `type_caster::load()`. /// 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. /// Adding a patient will keep it alive up until the enclosing function returns.
class loader_life_support { class loader_life_support {
private:
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: public:
/// A new patient frame is created when a function is entered /// A new patient frame is created when a function is entered
loader_life_support() { loader_life_support() {
get_internals().loader_patient_stack.push_back(nullptr); loader_life_support** stack = get_stack_pp();
parent = *stack;
*stack = this;
} }
/// ... and destroyed after it returns /// ... and destroyed after it returns
~loader_life_support() { ~loader_life_support() {
auto &stack = get_internals().loader_patient_stack; loader_life_support** stack = get_stack_pp();
if (stack.empty()) if (*stack != this)
pybind11_fail("loader_life_support: internal error"); pybind11_fail("loader_life_support: internal error");
*stack = parent;
auto ptr = stack.back(); for (auto* item : keep_alive)
stack.pop_back(); Py_DECREF(item);
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();
} }
/// This can only be used inside a pybind11-bound function, either by `argument_loader` /// 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. /// at argument preparation time or by `py::cast()` at execution time.
PYBIND11_NOINLINE static void add_patient(handle h) { PYBIND11_NOINLINE static void add_patient(handle h) {
auto &stack = get_internals().loader_patient_stack; loader_life_support* frame = *get_stack_pp();
if (stack.empty()) 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 " throw cast_error("When called outside a bound function, py::cast() cannot "
"do Python -> C++ conversions which require the creation " "do Python -> C++ conversions which require the creation "
"of temporary values"); "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());
} }
}; };

View File

@ -8,5 +8,5 @@ def _to_int(s):
return s return s
__version__ = "2.8.0.dev1" __version__ = "2.8.0.dev2"
version_info = tuple(_to_int(s) for s in __version__.split(".")) version_info = tuple(_to_int(s) for s in __version__.split("."))

View File

@ -129,6 +129,7 @@ set(PYBIND11_TEST_FILES
test_stl.cpp test_stl.cpp
test_stl_binders.cpp test_stl_binders.cpp
test_tagbased_polymorphic.cpp test_tagbased_polymorphic.cpp
test_thread.cpp
test_union.cpp test_union.cpp
test_virtual_functions.cpp) test_virtual_functions.cpp)

66
tests/test_thread.cpp Normal file
View File

@ -0,0 +1,66 @@
/*
tests/test_thread.cpp -- call pybind11 bound methods in threads
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.
*/
#include <pybind11/cast.h>
#include <pybind11/pybind11.h>
#include <chrono>
#include <thread>
#include "pybind11_tests.h"
namespace py = pybind11;
namespace {
struct IntStruct {
explicit IntStruct(int v) : value(v) {};
~IntStruct() { value = -value; }
IntStruct(const IntStruct&) = default;
IntStruct& operator=(const IntStruct&) = default;
int value;
};
} // namespace
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 lifetime extend the reference.
//
// This test should be run with ASAN for better effectiveness.
py::implicitly_convertible<int, IntStruct>();
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 != expected) {
throw std::runtime_error("Value changed!!");
}
});
m.def(
"test_no_gil",
[](int expected, const IntStruct &in) {
std::this_thread::sleep_for(std::chrono::milliseconds(5));
if (in.value != expected) {
throw std::runtime_error("Value 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.
}

44
tests/test_thread.py Normal file
View File

@ -0,0 +1,44 @@
# -*- coding: utf-8 -*-
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, 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():
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]:
x.join()
def test_implicit_conversion_no_gil():
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]:
x.join()