qipython.guard: Adds GIL manipulation types.

In `pybind11`, GIL manipulation RAII guard types are not reentrant (at
least `gil_scoped_release` is not) which means we cannot reliably use it
in subfunctions without having previous knowledge of the GIL state.

This patch adds new re-entrant types which wrap `pybind11` own guards.
These new types also check at destruction if the interpreter is
finalizing, potentially (if `pybind11` supports it) disabling the guard,
thus avoiding a thread termination.

It also replaces any occurrences of previous guards with the new ones.

Change-Id: Ie9b134ce05df1787ae5c7b93eae7e750fb317123
Reviewed-on: http://gerrit2.aldebaran.lan/1792
Reviewed-by: jmonnon <jmonnon@aldebaran.com>
Reviewed-by: philippe.martin <philippe.martin@softbankrobotics.com>
Tested-by: vincent.palancher <vincent.palancher@softbankrobotics.com>

Author: Vincent Palancher

qipython/common.hpp

@@ -19,6 +19,23 @@
 #include <future>
 #include <thread>
 
+
+// MAJOR, MINOR and PATCH must be in [0, 255].
+#define QI_PYBIND11_VERSION(MAJOR, MINOR, PATCH) \
+  (((MAJOR) << 16) | \
+   ((MINOR) <<  8) | \
+   ((PATCH) <<  0))
+
+#ifdef PYBIND11_VERSION_HEX
+// Remove the lowest 8 bits which represent the serial and level version components.
+# define QI_CURRENT_PYBIND11_VERSION (PYBIND11_VERSION_HEX >> 8)
+#else
+# define QI_CURRENT_PYBIND11_VERSION \
+  QI_PYBIND11_VERSION(PYBIND11_VERSION_MAJOR, \
+                      PYBIND11_VERSION_MINOR, \
+                      PYBIND11_VERSION_PATCH)
+#endif
+
 namespace qi
 {
 namespace py
@@ -93,32 +110,20 @@ boost::optional<T> extractKeywordArg(pybind11::dict kwargs,
   return pyArg.cast<T>();
 }
 
-/// A deleter that deletes the pointer outside of the GIL.
-///
-/// Useful for types that might deadlock on destruction if they keep the GIL
-/// locked.
-struct DeleteOutsideGIL
-{
-  template<typename T>
-  void operator()(T* ptr) const
-  {
-    pybind11::gil_scoped_release unlock;
-    delete ptr;
-  }
-};
-
-/// Delay the destruction of an object to a thread.
-struct DeleteInOtherThread
+/// Returns whether or not the interpreter is finalizing. If the information
+/// could not be fetched, the function returns an empty optional. Otherwise, it
+/// returns an optional set with a boolean stating if the interpreter is indeed
+/// finalizing or not.
+inline boost::optional<bool> interpreterIsFinalizing()
 {
-  template<typename T>
-  void operator()(T* ptr) const
-  {
-    pybind11::gil_scoped_release unlock;
-    auto fut = std::async(std::launch::async, [](std::unique_ptr<T>) {},
-                          std::unique_ptr<T>(ptr));
-    QI_IGNORE_UNUSED(fut);
-  }
-};
+// `_Py_IsFinalizing` is only available on CPython 3.7+
+#if PY_VERSION_HEX >= 0x03070000
+  return boost::make_optional(_Py_IsFinalizing() != 0);
+#else
+  // There is no way of knowing on older versions.
+  return {};
+#endif
+}
 
 } // namespace py
 } // namespace qi

qipython/pyfuture.hpp

@@ -25,12 +25,12 @@ using Promise = qi::Promise<AnyValue>;
 // future or its value.
 inline pybind11::object resultObject(const Future& fut, bool async)
 {
-  pybind11::gil_scoped_acquire lock;
+  GILAcquire lock;
   if (async)
     return castToPyObject(fut);
 
   // Wait for the future outside of the GIL.
-  auto res = invokeGuarded<pybind11::gil_scoped_release>(qi::SrcFuture{}, fut);
+  auto res = invokeGuarded<GILRelease>(qi::SrcFuture{}, fut);
   return castToPyObject(res);
 }
 

qipython/pyguard.hpp

@@ -17,6 +17,12 @@ namespace qi
 namespace py
 {
 
+/// Returns whether or not the GIL is currently held by the current thread.
+inline bool currentThreadHoldsGil()
+{
+  return PyGILState_Check() == 1;
+}
+
 /// DefaultConstructible Guard
 /// Procedure<_ (Args)> F
 template<typename Guard, typename F, typename... Args>
@@ -121,13 +127,115 @@ class Guarded
   explicit operator const Object&() const { return object(); }
 };
 
+/// G == pybind11::gil_scoped_acquire || G == pybind11::gil_scoped_release
+template<typename G>
+void pybind11GuardDisarm(G& guard)
+{
+  QI_IGNORE_UNUSED(guard);
+// The disarm API was introduced in v2.6.2.
+#if QI_CURRENT_PYBIND11_VERSION >= QI_PYBIND11_VERSION(2,6,2)
+  guard.disarm();
+#endif
+}
+
 } // namespace detail
 
+/// RAII utility type that guarantees that the GIL is locked for the scope of
+/// the lifetime of the object.
+///
+/// This type is re-entrant.
+///
+/// postcondition: `GILAcquire acq;` establishes `currentThreadHoldsGil()`
+struct GILAcquire
+{
+  inline GILAcquire()
+  {
+    // `gil_scoped_acquire` is re-entrant by itself, so we don't need to check
+    // whether or not the GIL is already held by the current thread.
+    QI_ASSERT(currentThreadHoldsGil());
+  }
+
+  GILAcquire(const GILAcquire&) = delete;
+  GILAcquire& operator=(const GILAcquire&) = delete;
+
+  inline ~GILAcquire()
+  {
+    const auto optIsFinalizing = interpreterIsFinalizing();
+    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
+    if (definitelyFinalizing)
+      detail::pybind11GuardDisarm(_acq);
+  }
+
+private:
+  pybind11::gil_scoped_acquire _acq;
+};
+
+/// RAII utility type that guarantees that the GIL is unlocked for the scope of
+/// the lifetime of the object.
+///
+/// This type is re-entrant.
+///
+/// postcondition: `GILRelease rel;` establishes `!currentThreadHoldsGil()`
+struct GILRelease
+{
+  inline GILRelease()
+  {
+    if (currentThreadHoldsGil())
+      _release.emplace();
+    QI_ASSERT(!currentThreadHoldsGil());
+  }
+
+  GILRelease(const GILRelease&) = delete;
+  GILRelease& operator=(const GILRelease&) = delete;
+
+  inline ~GILRelease()
+  {
+    const auto optIsFinalizing = interpreterIsFinalizing();
+    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
+    if (_release && definitelyFinalizing)
+      detail::pybind11GuardDisarm(*_release);
+  }
+
+private:
+  boost::optional<pybind11::gil_scoped_release> _release;
+};
+
 /// Wraps a pybind11::object value and locks the GIL on copy and destruction.
 ///
 /// This is useful for instance to put pybind11 objects in lambda functions so
 /// that they can be copied around safely.
-using GILGuardedObject = detail::Guarded<pybind11::gil_scoped_acquire, pybind11::object>;
+using GILGuardedObject = detail::Guarded<GILAcquire, pybind11::object>;
+
+/// Deleter that deletes the pointer outside the GIL.
+///
+/// Useful for types that might deadlock on destruction if they keep the GIL
+/// locked.
+struct DeleteOutsideGIL
+{
+  template<typename T>
+  void operator()(T* ptr) const
+  {
+    GILRelease unlock;
+    delete ptr;
+  }
+};
+
+/// Deleter that delays the destruction of an object to another thread.
+struct DeleteInOtherThread
+{
+  template<typename T>
+  void operator()(T* ptr) const
+  {
+    GILRelease unlock;
+    // `std::async` returns an object, that unless moved from will block when
+    // destroyed until the task is complete, which is unwanted behavior here.
+    // Therefore we just take the result of the `std::async` call into a local
+    // variable and then ignore it.
+    auto fut = std::async(std::launch::async, [](std::unique_ptr<T>) {},
+                          std::unique_ptr<T>(ptr));
+    QI_IGNORE_UNUSED(fut);
+  }
+};
 
 } // namespace py
 } // namespace qi

src/pyapplication.cpp

@@ -15,6 +15,7 @@
 #include <qi/log.hpp>
 
 #include <qipython/common.hpp>
+#include <qipython/pyguard.hpp>
 #include <qipython/pyapplication.hpp>
 #include <qipython/pysession.hpp>
 
@@ -80,25 +81,25 @@ void exportApplication(::py::module& m)
   using namespace ::py;
   using namespace ::py::literals;
 
-  gil_scoped_acquire lock;
+  GILAcquire lock;
 
   class_<Application, std::unique_ptr<Application, DeleteInOtherThread>>(
     m, "Application")
     .def(init(withArgcArgv<>([](int& argc, char**& argv) {
-           gil_scoped_release unlock;
+           GILRelease unlock;
            return new Application(argc, argv);
          })),
          "args"_a)
-    .def_static("run", &Application::run, call_guard<gil_scoped_release>())
-    .def_static("stop", &Application::stop, call_guard<gil_scoped_release>());
+    .def_static("run", &Application::run, call_guard<GILRelease>())
+    .def_static("stop", &Application::stop, call_guard<GILRelease>());
 
   class_<ApplicationSession,
          std::unique_ptr<ApplicationSession, DeleteInOtherThread>>(
     m, "ApplicationSession")
 
     .def(init(withArgcArgv<bool, const std::string&>(
            [](int& argc, char**& argv, bool autoExit, const std::string& url) {
-             gil_scoped_release unlock;
+             GILRelease unlock;
              ApplicationSession::Config config;
              if (!autoExit)
                config.setOption(qi::ApplicationSession::Option_NoAutoExit);
@@ -108,30 +109,30 @@ void exportApplication(::py::module& m)
            })),
          "args"_a, "autoExit"_a, "url"_a)
 
-    .def("run", &ApplicationSession::run, call_guard<gil_scoped_release>(),
+    .def("run", &ApplicationSession::run, call_guard<GILRelease>(),
          doc("Block until the end of the program (call "
              ":py:func:`qi.ApplicationSession.stop` to end the program)."))
 
     .def_static("stop", &ApplicationSession::stop,
-                call_guard<gil_scoped_release>(),
+                call_guard<GILRelease>(),
                 doc(
                   "Ask the application to stop, the run function will return."))
 
     .def("start", &ApplicationSession::startSession,
-         call_guard<gil_scoped_release>(),
+         call_guard<GILRelease>(),
          doc("Start the connection of the session, once this function is "
              "called everything is fully initialized and working."))
 
     .def_static("atRun", &ApplicationSession::atRun,
-                call_guard<gil_scoped_release>(), "func"_a,
+                call_guard<GILRelease>(), "func"_a,
                 doc(
                   "Add a callback that will be executed when run() is called."))
 
     .def_property_readonly("url",
                            [](const ApplicationSession& app) {
                              return app.url().str();
                            },
-                           call_guard<gil_scoped_release>(),
+                           call_guard<GILRelease>(),
                            doc("The url given to the Application. It's the url "
                                "used to connect the session."))
 

src/pyasync.cpp

@@ -5,6 +5,7 @@
 
 #include <qipython/pysignal.hpp>
 #include <qipython/common.hpp>
+#include <qipython/pyguard.hpp>
 #include <qipython/pyfuture.hpp>
 #include <qipython/pyobject.hpp>
 #include <qipython/pystrand.hpp>
@@ -29,7 +30,7 @@ ::py::object async(::py::function pyCallback,
                    ::py::args args,
                    ::py::kwargs kwargs)
 {
-  ::py::gil_scoped_acquire lock;
+  GILAcquire lock;
 
   qi::uint64_t usDelay = 0;
   if (const auto optUsDelay = extractKeywordArg<qi::uint64_t>(kwargs, delayArgName))
@@ -42,7 +43,7 @@ ::py::object async(::py::function pyCallback,
   GILGuardedObject guardKwargs(std::move(kwargs));
 
   auto invokeCallback = [=]() mutable {
-    ::py::gil_scoped_acquire lock;
+    GILAcquire lock;
     auto res = ::py::object((*guardCb)(**guardArgs, ***guardKwargs)).cast<AnyValue>();
     // Release references immediately while we hold the GIL, instead of waiting
     // for the lambda destructor to relock the GIL.
@@ -79,7 +80,7 @@ void exportAsync(::py::module& m)
   using namespace ::py;
   using namespace ::py::literals;
 
-  gil_scoped_acquire lock;
+  GILAcquire lock;
 
   m.def("runAsync", &async,
         "callback"_a,
@@ -106,7 +107,7 @@ void exportAsync(::py::module& m)
          [](PeriodicTask& task, qi::int64_t usPeriod) {
            task.setPeriod(qi::MicroSeconds(usPeriod));
          },
-         call_guard<gil_scoped_release>(), "usPeriod"_a,
+         call_guard<GILRelease>(), "usPeriod"_a,
          doc("Set the call interval in microseconds.\n"
              "This call will wait until next callback invocation to apply the "
              "change.\n"
@@ -118,7 +119,7 @@ void exportAsync(::py::module& m)
              "   task.setUsPeriod(100)\n"
              "   task.start()\n"
              ":param usPeriod: the period in microseconds"))
-    .def("start", &PeriodicTask::start, call_guard<gil_scoped_release>(),
+    .def("start", &PeriodicTask::start, call_guard<GILRelease>(),
          "immediate"_a,
          doc("Start the periodic task at specified period. No effect if "
              "already running.\n"
@@ -127,20 +128,20 @@ void exportAsync(::py::module& m)
              ".. warning::\n"
              "   concurrent calls to start() and stop() will result in "
              "undefined behavior."))
-    .def("stop", &PeriodicTask::stop, call_guard<gil_scoped_release>(),
+    .def("stop", &PeriodicTask::stop, call_guard<GILRelease>(),
          doc("Stop the periodic task. When this function returns, the callback "
              "will not be called "
              "anymore. Can be called from within the callback function\n"
              ".. warning::\n"
              "   concurrent calls to start() and stop() will result in "
              "undefined behavior."))
     .def("asyncStop", &PeriodicTask::asyncStop,
-         call_guard<gil_scoped_release>(),
+         call_guard<GILRelease>(),
          doc("Request for periodic task to stop asynchronously.\n"
              "Can be called from within the callback function."))
     .def(
       "compensateCallbackTime", &PeriodicTask::compensateCallbackTime,
-      call_guard<gil_scoped_release>(), "compensate"_a,
+      call_guard<GILRelease>(), "compensate"_a,
       doc(
         ":param compensate: boolean. True to activate the compensation.\n"
         "When compensation is activated, call interval will take into account "
@@ -149,7 +150,7 @@ void exportAsync(::py::module& m)
         "   when the callback is longer than the specified period, "
         "compensation will result in the callback being called successively "
         "without pause."))
-    .def("setName", &PeriodicTask::setName, call_guard<gil_scoped_release>(),
+    .def("setName", &PeriodicTask::setName, call_guard<GILRelease>(),
          "name"_a, doc("Set name for debugging and tracking purpose"))
     .def("isRunning", &PeriodicTask::isRunning,
          doc(":returns: true if task is running"))

src/pyclock.cpp

@@ -5,6 +5,7 @@
 
 #include <qipython/pyclock.hpp>
 #include <qipython/common.hpp>
+#include <qipython/pyguard.hpp>
 #include <qi/clock.hpp>
 #include <pybind11/pybind11.h>
 
@@ -31,7 +32,7 @@ void exportClock(::py::module& m)
   using namespace ::py;
   using namespace ::py::literals;
 
-  gil_scoped_acquire lock;
+  GILAcquire lock;
 
   m.def("clockNow", &now<Clock>,
         doc(":returns: current timestamp on qi::Clock, as a number of nanoseconds"));