This fixes it... I think?

Author: eacousineau

python/pybind11/custom_tests/test_gil_issue1723.cc

@@ -2,9 +2,11 @@
 #include <iostream>
 #include <thread>
 #include <future>
+#include <functional>
 
 #include <pybind11/embed.h>
 #include <pybind11/eval.h>
+#include <pybind11/functional.h>
 #include <pybind11/pybind11.h>
 
 namespace py = pybind11;
@@ -13,38 +15,66 @@ void hello() {
   std::cout << "hello\n";
 }
 
+// Keep this as C++ code!
 struct Executor {
+  using Callback = std::function<void ()>;
 
   void execute() {
     auto func = channel.get_future().get();
     func();
   }
 
-  explicit Executor(py::object callback)
-    : callback(std::move(callback)),
-      channel(),
+  explicit Executor()
+    : channel(),
       worker([this]{execute();}) {
   }
 
-  void run() {
-    pybind11::gil_scoped_release release{};
+  void run(Callback callback) {
     channel.set_value(callback);
   }
 
-  py::object callback;
-  std::promise<py::object> channel;
+  void stop() {
+    if (!stopped) {
+      worker.join();
+      stopped = true;
+    } else {
+      throw std::runtime_error("Cannot stop twice");
+    }
+  }
+
+  std::promise<Callback> channel;
   std::thread worker;
+  bool stopped{false};
 
   ~Executor() {
-    worker.join();
+    if (!stopped) {
+      std::cerr
+          << "WARNING: Joining thread in destructor for Python stuff might "
+             "be bad, b/c the `std::function<>` dtor could cause a decref "
+             "without GIL being held. Call `stop()` explicitly instead!\n";
+      stop();
+    }
   }
 };
 
 void init_module(py::module m) {
   m.def("hello", &hello, "Say hello");
   py::class_<Executor>(m, "Executor")
-      .def(py::init<py::object>())
-      .def("run", &Executor::run);
+      .def(py::init<>())
+      .def("run",
+        [](Executor& self, Executor::Callback callback) {
+          // Acquire when executing potential Python code!
+          // In general, you will want wrapped stuff to do GIL acquisition.
+          // NOTE: I (eric) dunno if `cpp_function` automatically does this or
+          // not...
+          auto gil_callback = [callback]() {
+            pybind11::gil_scoped_acquire lock{};
+            callback();
+          };
+          self.run(gil_callback);
+        },
+        py::arg("callback"))
+      .def("stop", &Executor::stop);
 }
 
 int main(int, char**) {
@@ -56,25 +86,17 @@ int main(int, char**) {
 
   py::print("[ Eval ]");
   py::exec(R"""(
+import time
+
 def bye():
     print('bye')
 
-def main():
-    pass
-
-if True:
-    m.hello()
-    ex = m.Executor(bye)
-    ex.run()
-
-use_trace = False
-if use_trace:
-    import sys, trace
-    sys.stdout = sys.stderr
-    tracer = trace.Trace(trace=1, count=0, ignoredirs=["/usr", sys.prefix])
-    tracer.runfunc(main)
-else:
-    main()
+m.hello()
+ex = m.Executor()
+ex.run(callback=bye)
+# Give thread time to execute.
+time.sleep(0.1)
+ex.stop()
 )""");
 
   py::print("[ Done ]");