py::scoped_critical_section tagging on py::class_ definitions.

[laramiel]

This touches on #5684

I'm working on tensorstore (http://github.com/google/tensorstore) free threading (google/tensorstore#218) and looking at possible py::scoped_critical_section use.

I'd like a way to easily tag a cls.def(...) or other method definition as requiring a critical section. The only current option is to add a wrapper type which adds a mutex to the existing C++ type.

This is somewhat important as any mutable access in free-threaded mode likely requires a critical section. That is, basically any type which allows internal/mutable updates via cls.def_property(getter, setter, ...) should use some sort of synchronization. This synchronization is difficult to achieve using the py::call_guard<T> mechanism, since the guard_type T must be default constructible. This works fine for using a GIL lock, but unfortunately for free threading, there isn't a way to provide the py::handle to py::call_guard.

Tensorstore has quite a few such types where adding such a wrapper type would be a burden; in lieu of this we'd rather rely on the built-in python mutex (https://peps.python.org/pep-0703/#reference-counting) for most simple types.

It appears that the other path to handle this is to use py::handle self as the self parameter to cls.def... calls, do the locking,
then add the py::cast<T> call in each location. Of course, the difficulty there is that avoids much of the checking on the invocation side.

Something which used to be this:

  cls.def(
      "copy",
      [](Self& self) {
        return self;
      })

Becomes much more ungainly:

  cls.def(
      "copy",
      [](py::handle self) {
        py::detail::make_caster<Self&> caster;
        if (!caster.load(self, false)) {
          // this skips all the checks for function overloads,
          // so we'd need to do that by hand.
          py::raise_error(...);
        }
        py::scoped_critical_section cs(self);
        auto& cpp_self = py::detail::cast_op<Self&>(caster);
        return cpp_self;
      })

---

Wouldn't this be nicer:

  cls.def(
      "copy",
      [](Self& self) {
        return self;
      },
      py::with_critical_section)

  cls.def(
      "__eq__",
      [](Self& a, Self& b) {
        return a == b;
      },
      py::with_critical_section2)

The current init, precall, postcall methods of process_attribute are insufficient, unless
In order for the with_critical_section to work, we'd probably need to add roughly the following:

file attr.h:

template<size_t I = 0>
struct with_critical_section {};
template<size_t I = 0, size_t J = 1>
struct with_critical_section2 {
  static_assert(I != J);
};

template <size_t I>
struct process_attribute<with_critical_section<I>> {
    static void postcall(function_call &, handle) {
      call.cs = std::nullopt;
    }

    static void precall(function_call &call) {
      if (call.args.size() < (I+1)) py::raise_error(...)
      if (call.cs.has_value()) py::raise_error(...)
      call.cs.emplace(call.args[I]);
    }
};

template<size_t I, size_t J>
struct process_attribute<with_critical_section2<I, J>> {
    static void postcall(function_call &, handle) {
      call.cs = std::nullopt;
    }

    static void precall(function_call &call) {
      if (call.args.size() < (I+1)) py::raise_error(...)
      if (call.args.size() < (J+1)) py::raise_error(...)
      if (call.cs.has_value()) py::raise_error(...)
      call.cs.emplace(call.args[I], call.args[J]);
    }
};

file cast.h:

struct function_call {
   ...
   std::optional<py::scoped_critical_section> cs;
};

---

There are other features which could, potentially, be useful. For example, we could have a wrapper type which allows
passing both the handle and the result of cast_op to the caller.

  cls.def(
      "copy",
      [](py::with_handle<Self>& self) {
        py::scoped_critical_section(self.handle);
        return self.value;
      })

Which we could probably achieve via something like this:

// pair-like type holding a value and reference.
template <typename T>
struct with_handle {
  pybind11::handle handle;
  T value;
};

// caster for with_handle.
template <typename T>
struct with_handle_caster {
 private:
  using base_caster = pybind11::detail::make_caster<T>;

 public:
  static constexpr auto name = base_caster::name {}

  // cast converts from C++ -> Python
  static pybind11::handle cast(with_handle<T> src,
                               pybind11::return_value_policy policy,
                               pybind11::handle p) {
    static_assert(false);
  }

  // load converts from Python -> C++
  bool load(pybind11::handle src, bool convert) {
    base_caster base;
    if (base.load(src, convert)) {
      value.emplace(src, cast_op<T>(base));
      return true;
    }
    return false;
  }

  explicit operator with_handle<T>&() { return *value; }
  explicit operator with_handle<T>*() { return &*value; }

  template <typename _T>
  using cast_op_type = pybind11::detail::cast_op_type<_T>;

 private:
  std::optional<with_handle<T>> value;
};

// specialization.
struct ::pybind11::detail::type_caster<with_handle<T>> : public with_handle_caster<T> {};

Ping @rwgk to get your thoughts here. Also @jbms

[jbms]

I like the with_handle approach. I think that one also doesn't need to be upstreamed to work, right?

I think caster struct should embed the base caster, because sometimes the caster stores necessary additional objects I believe.