[BUG]: retranslating local exception with custom data class hangs in free-threading cpython

vfdev-5 commented 2 months ago

Required prerequisites

[X] Make sure you've read the documentation. Your issue may be addressed there.
[X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
[X] Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

2.13.5 and from source

Problem description

I have a program stuck when retranslating local exception with custom data class using CPython with free-threading mode enabled.

This is due to internals.mutex being in the locked state when the callback is running. When using custom data, all_type_info_get_cache method is called internally which also tries to lock internals.mutex.

See the repro code below

Reproducible example code

C++ test1.cpp

// https://pybind11.readthedocs.io/en/stable/basics.html
// clang++ -O3 -Wall -shared -std=c++11 -fPIC $(python -m pybind11 --includes) test1.cpp -o test1.so
// python -c "import test1; test1.check()"

#include <string>
#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

namespace py = pybind11;

struct CustomData {
  CustomData(const std::string & a): a(a) {}
  std::string a;
};

struct CustomError {
  CustomError(const CustomData & message) : message(message) {}
  CustomData message;
};

PYBIND11_MODULE(test1, m, py::mod_gil_not_used()) {
  m.doc() = "test custom exception with free-threading";

  m.def("check", [](){
    auto d1 = CustomData("abc");
    throw CustomError(d1);
  });

  py::class_<CustomData>(m, "CustomData", py::module_local())
        .def(py::init<const std::string &>())
        .def_readwrite("a", &CustomData::a);
  py::register_local_exception_translator([](std::exception_ptr p) {
    try {
      if (p)
        std::rethrow_exception(p);
    } catch (const CustomError &e) {

      printf("Handle CustomError exception\n");
      auto mod = py::module_::import("exceptions1");
      py::object obj = mod.attr("CustomError");
      printf("Before the exception creation\n");

      // Here we can check that internals.mutex is locked: internals.mutex.mutex._bits == 1
      // obj(e.message) calls `all_type_info_get_cache` which would try to lock again internals

      // // If we unlock internals.mutex then obj(e.message) will work otherwise
      // // execution hangs.
      // auto &internals = py::detail::get_internals();
      // internals.mutex.unlock();

      py::object obj2 = obj(e.message);

      // We should lock again if we unlocked it
      // internals.mutex.lock();

      printf("After the exception creation\n");
      PyErr_SetObject(PyExc_Exception, obj2.ptr());

    }
  });
}

Python code: exceptions1.py

class CustomError(Exception):
    def __init__(self, message):
        self.message = message
        super().__init__(message)

    def __str__(self):
        s = "[python]: " + self.message.a
        return s

Run:

python -c "import test1; test1.check()"

Output:

Handle CustomError exception
Before the exception creation

Is this a regression? Put the last known working version here if it is.

Not a regression

colesbury commented 1 month ago

I think the easiest approach might be to use a different mutex for the exception translators than for the rest of the internals. Other than initialization and finalization, I think the accesses for exception translators and the rest of internals are separate

The alternative is to unlock before calling exception translators, but to do that safely I think you may need to copy the list of translators.

vfdev-5 commented 1 month ago

Thanks @colesbury let me check these approaches!

I tried another alternative doing the following, checking whether the mutex is already locked and lock it only if unlocked:

// Wrapper around PyMutex to provide BasicLockable semantics
class pymutex {
    PyMutex mutex;

public:
    pymutex() : mutex({}) {}
    void lock() { PyMutex_Lock(&mutex); }
    void unlock() { PyMutex_Unlock(&mutex); }
    bool has_lock() { return mutex._bits == 1; }
};

#ifdef Py_GIL_DISABLED
#    define PYBIND11_LOCK_INTERNALS(internals) std::unique_lock<pymutex> lock((internals).mutex, std::defer_lock); if ((internals).mutex.has_lock()) { lock.lock(); }
#else
#    define PYBIND11_LOCK_INTERNALS(internals)
#endif

What do you think?

colesbury commented 1 month ago

That looks like it would skip locking the mutex if it's held by another thread, which would not be safe. But also, your sample code only locks the mutex if it's already locked, so I don't think it will ever get locked.

pybind / pybind11