[BUG] pybind11::cast is not thread safe

[zachariahreed]

in particular use of loader_life_support seems problematic. This function keeps state in the global loader_patient_stack:

pybind11/include/pybind11/detail/internals.h

Line 105 in b7dfe5c

std::vector<PyObject *> loader_patient_stack; // Used by `loader_life_support`

In the case where a cast can release the GIL, two thread's push/pop of loader_patient_stack can become interleaved.

While releasing the GIL in a conversion seems like a bad idea, it can be surprisingly hard to avoid. Any cast that results in the bytecode interpreter being run (either directly or through something like a __del__ method) can cause python itself to release GIL (sys.setswitchinterval).

[YannickJadoul]

@zachariahreed, that's interesting. Do you have concrete code that manages to show this, or how did you run into this?

I was about to shout "But of course you should hold the GIL", but then you already thought of that.
However, by the time the interpreter stops running and hands back control to pybind11, I'd expect the GIL to be held again? Do you mind explaining slightly more in depth how the GIL can be released without pybind11 knowing about it?

[zachariahreed]

The GIL is held when you get control back. The problem comes when another thread with pybind11-wrapped code gets scheduled during the time when you don't have control. Then you end up in a situation like:

Thread1          Thread2
--------------   --------------
Push             ...
  Cast           ...
    ByteCode     ...
       Yield     ...
         ...     Push
         ...       ByteCode
         ...         Yield
Pop -> Boom      ...

The specific code that triggered this was use of pybind11::module::import inside of a custom caster. Much of the import machinery is written in pure python these days and its complex enough that it can certainly run longer than the multiplexing period.

[YannickJadoul]

Argh, right, yes. So rather than poping, we should remember which pointer was added?

[zachariahreed]

I don't think that's sufficient. During the time between when thread 1 gets control back and before the pop, if additional temporaries are created they'll end up in thread2's "arena". The flow control is not linear, so I don't think there's any way a vector-based strategy can work. I looked very briefly at moving this into thread-local storage, but I was a bit concerned about performance as this seems to be a hot path.

[YannickJadoul]

You're probably right... To the best of your knowledge right now (not asking you to dive into pybind11), is it just loader_patient_stack (which could get a reasonably quick fix, if we have a way of trying this out) or should we do a full investigation of what else can go wrong?

[zachariahreed]

I don't fully understand the details of the casting implementation / when temporaries are created. But, as far as I can tell, this seems to be the only piece of global / singleton data mutated. I think it's probably just loader_patient_stack.

[jokervTv]

So, does this mean I cannot use openmp or intel tbb ?

[daniel-smith]

I believe I am experiencing the same issue, but via a slightly different mechanism. In my case, I am observing this issue via temporaries stored in the loader_patient_stack via the argument_loader. Specifically for types where py::implicitly_convertible is used, and where arguments of those types are passed by reference to the bound function. Argument objects where an implicit conversion is performed are stored in this stack to keep them alive.

pybind11/include/pybind11/detail/type_caster_base.h

Lines 709 to 714 in 617cb65

	// Perform an implicit conversion
	if (convert) {
	for (auto &converter : typeinfo->implicit_conversions) {
	auto temp = reinterpret_steal<object>(converter(src.ptr(), typeinfo->type));
	if (load_impl<ThisT>(temp, false)) {
	loader_life_support::add_patient(temp);

If calling such a function concurrently from multiple threads, and where that function releases the GIL, I am intermittently observing the argument reference becoming "invalidated", i.e. showing garble at some point after the GIL is released. The way I was able to observe this was by copying the argument into a local variable prior to releasing the GIL, then comparing the copy to the argument once the GIL is released. I'm able to reproduce this frequently with two threads calling the same function concurrently within a loop. Capturing and inspecting items from the loader_patient_stack before and after the GIL is released show the current thread is basically observing the pushing/popping of the stack from other threads. The upshot is that if we have an argument passed by reference that is backed by an item in the loader_patient_stack, that memory can be wiped out by another thread if the GIL is released - rendering the passed-by-reference argument invalid.

So in summary, it appears to not be not thread-safe to pass arguments by reference, where those arguments have implicit conversions specified - due to the aforementioned thread-safety issues of loader_patient_stack.

[daniel-smith]

OK, so here is a small example that reproduces the above issue. I used the cmake_example project as a starting point, hence the naming.

pybind11 module:

#include <pybind11/pybind11.h>

#include <chrono>
#include <thread>

namespace py = pybind11;
using namespace std::chrono_literals;

struct Bar
{
    int value;
};

void foo(const Bar& bar)
{
    const auto copy = bar;

    {
        py::gil_scoped_release release;

        // Simulate long running task.
        std::this_thread::sleep_for(100ms);
    }

    if (bar.value != copy.value)
    {
        throw std::exception("Reference changed!!");
    }
}

PYBIND11_MODULE(cmake_example, m)
{
    py::class_<Bar>(m, "Bar")
        .def(py::init([](const int i) { return Bar{i}; }));

    py::implicitly_convertible<int, Bar>();

    m.def("foo", &foo);
}

Python test:

import cmake_example as c

import pytest
from concurrent.futures.thread import ThreadPoolExecutor


@pytest.mark.parametrize('repeat', range(10))
def test(repeat):
    thread_count = 2
    call_count = 10
    values = [i for i in range(call_count)]

    with ThreadPoolExecutor(max_workers=thread_count) as executor:
        results = [executor.submit(c.foo, i) for i in values]

    # Materialise the results - exception will be thrown from 'foo' when reference is invalid.
    results = list(r.result() for r in results)

Example results:

========================================================================= short test summary info =========================================================================
FAILED main.py::test[1] - RuntimeError: Reference changed!!
FAILED main.py::test[3] - RuntimeError: Reference changed!!
FAILED main.py::test[4] - RuntimeError: Reference changed!!
FAILED main.py::test[5] - RuntimeError: Reference changed!!
FAILED main.py::test[7] - RuntimeError: Reference changed!!
FAILED main.py::test[8] - RuntimeError: Reference changed!!
======================================================================= 6 failed, 4 passed in 5.27s =======================================================================

[daniel-smith]

I looked very briefly at moving this into thread-local storage, but I was a bit concerned about performance as this seems to be a hot path.

I can confirm that using thread-local storage (TLS) is indeed a solution that will solve this problem. @zachariahreed, what specifically around performance were you concerned about? As I understand, there is some concern around TLS overhead when used in a dynamic module, but I'm wondering whether this is anything to be particularly concerned about here. Some benchmarks (see https://testbit.eu/2015/thread-local-storage-benchmark) suggest very minimal overhead on modern platforms. Obviously it would be prudent to run benchmarks for this particular case, but I'm curious what others' thoughts may be around this.

From my brief look into this, using a thread_local variable to replace the patient_loader_stack from the "internals" would probably involve the least amount of change. E.g. something like the following in the loader_life_support class:

static std::vector<PyObject*>& loader_patient_stack()
{
    thread_local std::vector<PyObject*> stack;
    return stack;
}

v2.7.1...daniel-smith:bugfix/loader_life_support

Another approach might be to have an instance of a PyObject* list per call - wrapped in some kind of "context" that gets passed through with each function call here:

pybind11/include/pybind11/pybind11.h

Lines 802 to 805 in cb60ed4

	// 6. Call the function.
	try {
	loader_life_support guard{};
	result = func.impl(call);

That would avoid the use of TLS - but would also involve an arguably unfeasible amount of refactoring - as the instance would also need to make its way through to the type casters. Not sure if anyone else has any opinions on either of these approaches, or any alternative approaches. I personally feel that TLS seems appropriate, but would be good to see what others think.

[Skylion007]

@daniel-smith Would welcome a PR on this front (with relevant tests). We do use TLS for the get_internals() struct, so it wouldn't be out of the question. It also wouldn't impact single_threaded performance as long as it's guarded WITH_THREADS like other TLS for get_internals() is.

There also has been some refactoring wanted on the patients STLs to use unordered_maps or such for faster access in case of duplicates such as #1253 and this might be a good time to do both.