Fix GLFW crash when Python exits while the passive viewer is running.

Fixes #790

PiperOrigin-RevId: 549440435
Change-Id: I808f6aaab6662225e5fe1131e0da72eca6011ba9

doc/changelog.rst

@@ -30,6 +30,8 @@ Python bindings
   ``update_texture`` methods to allow users to update renderable assets.
   (`#812 <https://github.com/deepmind/mujoco/issues/812>`_, `#958 <https://github.com/deepmind/mujoco/issues/958>`_,
   `#965 <https://github.com/deepmind/mujoco/issues/965>`_)
+- Fix GLFW crash when Python exits while the passive viewer is running.
+  (`#790 <https://github.com/deepmind/mujoco/issues/790>`_)
 
 Models
 ^^^^^^

python/mujoco/CMakeLists.txt

@@ -405,6 +405,7 @@ target_link_libraries(
   _simulate
   PRIVATE mujoco
           mujoco::libsimulate
+          errors_header
           raw
           structs_header
 )

python/mujoco/simulate.cc

@@ -13,14 +13,17 @@
 // limitations under the License.
 
 #include <atomic>
+#include <chrono>
 #include <cstring>
 #include <memory>
 #include <string>
+#include <thread>
 #include <utility>
 
 #include <glfw_adapter.h>
 #include <glfw_dispatch.h>
 #include <simulate.h>
+#include "errors.h"
 #include "structs.h"
 #include <pybind11/gil.h>
 #include <pybind11/pybind11.h>
@@ -31,21 +34,20 @@ namespace {
 namespace py = ::pybind11;
 
 template <typename T, int N>
-constexpr inline std::size_t sizeof_arr(const T(&arr)[N]) {
+constexpr inline std::size_t sizeof_arr(const T (&arr)[N]) {
   return sizeof(arr);
 }
 
-class SimulateWrapper : public mujoco::Simulate {
+class SimulateWrapper {
  public:
   SimulateWrapper(std::unique_ptr<PlatformUIAdapter> platform_ui_adapter,
-                  py::object scn, py::object cam,
+                  py::object scn, py::object cam, py::object opt,
-                  py::object opt, py::object pert,  bool fully_managed)
+                  py::object pert, bool fully_managed)
-      : Simulate(std::move(platform_ui_adapter),
+      : simulate_(new mujoco::Simulate(
-                 scn.cast<MjvSceneWrapper&>().get(),
+            std::move(platform_ui_adapter), scn.cast<MjvSceneWrapper&>().get(),
-                 cam.cast<MjvCameraWrapper&>().get(),
+            cam.cast<MjvCameraWrapper&>().get(),
-                 opt.cast<MjvOptionWrapper&>().get(),
+            opt.cast<MjvOptionWrapper&>().get(),
-                 pert.cast<MjvPerturbWrapper&>().get(),
+            pert.cast<MjvPerturbWrapper&>().get(), fully_managed)),
-                 fully_managed),
         m_(py::none()),
         d_(py::none()),
         scn_(scn),
@@ -53,12 +55,33 @@ class SimulateWrapper : public mujoco::Simulate {
         opt_(opt),
         pert_(pert) {}
 
+  ~SimulateWrapper() { Destroy(); }
+
+  void Destroy() {
+    if (simulate_) {
+      delete simulate_;
+      simulate_ = nullptr;
+      destroyed_.store(1);
+    }
+  }
+
+  void WaitUntilDestroyed() {
+    // TODO: replace with atomic wait when we migrate to C++20
+    while (!destroyed_.load()) {
+      std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    }
+  }
+
   void Load(py::object m, py::object d, const std::string& path) {
+    if (!simulate_) {
+      return;
+    }
+
     mjModel* m_raw = m.cast<MjModelWrapper&>().get();
     mjData* d_raw = d.cast<MjDataWrapper&>().get();
     {
       py::gil_scoped_release no_gil;
-      Simulate::Load(m_raw, d_raw, path.c_str());
+      simulate_->Load(m_raw, d_raw, path.c_str());
     }
     m_ = m;
     d_ = d;
@@ -66,7 +89,12 @@ class SimulateWrapper : public mujoco::Simulate {
     d_raw_ = d_raw;
   }
 
+  mujoco::Simulate* simulate() { return simulate_; }
+
  private:
+  mujoco::Simulate* simulate_;
+  std::atomic_int destroyed_ = 0;
+
   // Hold references to keep these Python objects alive for as long as the
   // simulate object.
   py::object m_;
@@ -80,6 +108,49 @@ class SimulateWrapper : public mujoco::Simulate {
   mjData* d_raw_ = nullptr;
 };
 
+inline mujoco::Simulate& SimulateRefOrThrow(SimulateWrapper& wrapper) {
+  auto* sim = wrapper.simulate();
+  if (!sim) {
+    throw UnexpectedError("simulate object is already deleted");
+  }
+  return *sim;
+}
+
+template <typename T, typename... Args>
+inline auto CallIfNotNull(T (*func)(mujoco::Simulate&, Args...)) {
+  return [func](SimulateWrapper& wrapper, Args&&... args) {
+    return func(SimulateRefOrThrow(wrapper), std::forward<Args>(args)...);
+  };
+}
+
+template <typename... Args>
+inline auto CallIfNotNull(void (*func)(mujoco::Simulate&, Args...)) {
+  return [func](SimulateWrapper& wrapper, Args&&... args) -> void {
+    func(SimulateRefOrThrow(wrapper), std::forward<Args>(args)...);
+  };
+}
+
+template <typename... Args>
+inline auto CallIfNotNull(void (mujoco::Simulate::*func)(Args...)) {
+  return [func](SimulateWrapper& wrapper, Args&&... args) -> void {
+    (SimulateRefOrThrow(wrapper).*func)(std::forward<Args>(args)...);
+  };
+}
+
+template <typename T>
+inline auto GetIfNotNull(T mujoco::Simulate::*member) {
+  return [member](SimulateWrapper& wrapper) -> T& {
+    return SimulateRefOrThrow(wrapper).*member;
+  };
+}
+
+template <typename T, typename... Args>
+inline auto SetIfNotNull(T mujoco::Simulate::*member) {
+  return [member](SimulateWrapper& wrapper, const T& value) -> void {
+    SimulateRefOrThrow(wrapper).*member = value;
+  };
+}
+
 PYBIND11_MODULE(_simulate, pymodule) {
   py::class_<SimulateMutex>(pymodule, "Mutex")
       .def(
@@ -100,110 +171,109 @@ PYBIND11_MODULE(_simulate, pymodule) {
             std::make_unique<mujoco::GlfwAdapter>(), scn, cam, opt, pert,
             fully_managed);
       }))
+      .def("destroy", &SimulateWrapper::Destroy,
+           py::call_guard<py::gil_scoped_release>())
       .def("load", &SimulateWrapper::Load)
-      .def("sync", &mujoco::Simulate::Sync,
+      .def("sync", CallIfNotNull(&mujoco::Simulate::Sync),
            py::call_guard<py::gil_scoped_release>())
 
+      .def("render_loop", CallIfNotNull(&mujoco::Simulate::RenderLoop),
+           py::call_guard<py::gil_scoped_release>())
+      .def("lock", GetIfNotNull(&mujoco::Simulate::mtx),
+           py::call_guard<py::gil_scoped_release>(),
+           py::return_value_policy::reference_internal)
+      .def_property_readonly("ctrl_noise_std",
+                             GetIfNotNull(&mujoco::Simulate::ctrl_noise_std),
+                             py::call_guard<py::gil_scoped_release>())
+      .def_property_readonly("ctrl_noise_rate",
+                             GetIfNotNull(&mujoco::Simulate::ctrl_noise_rate),
+                             py::call_guard<py::gil_scoped_release>())
+
+      .def_property_readonly("real_time_index",
+                             GetIfNotNull(&mujoco::Simulate::real_time_index),
+                             py::call_guard<py::gil_scoped_release>())
+      .def_property("speed_changed",
+                    GetIfNotNull(&mujoco::Simulate::speed_changed),
+                    SetIfNotNull(&mujoco::Simulate::speed_changed),
+                    py::call_guard<py::gil_scoped_release>())
+      .def_property("measured_slowdown",
+                    GetIfNotNull(&mujoco::Simulate::measured_slowdown),
+                    SetIfNotNull(&mujoco::Simulate::measured_slowdown),
+                    py::call_guard<py::gil_scoped_release>())
+      .def_property_readonly("refresh_rate",
+                             GetIfNotNull(&mujoco::Simulate::refresh_rate),
+                             py::call_guard<py::gil_scoped_release>())
+
+      .def_property_readonly("busywait",
+                             GetIfNotNull(&mujoco::Simulate::busywait),
+                             py::call_guard<py::gil_scoped_release>())
+      .def_property_readonly("run", GetIfNotNull(&mujoco::Simulate::run),
+                             py::call_guard<py::gil_scoped_release>())
+
+      .def_property_readonly("exitrequest",
+                             CallIfNotNull(+[](mujoco::Simulate& sim) {
+                               return sim.exitrequest.load();
+                             }),
+                             py::call_guard<py::gil_scoped_release>())
       .def(
-          "render_loop",
+          "exit",
-          [](SimulateWrapper& simulate) { simulate.RenderLoop(); },
+          [](SimulateWrapper& wrapper) {
-          py::call_guard<py::gil_scoped_release>())
+            mujoco::Simulate* sim = wrapper.simulate();
-      .def(
+            if (!sim) {
-          "lock",
+              return;
-          [](SimulateWrapper& simulate) -> SimulateMutex& {
+            }
-            return simulate.mtx;
+
+            int value = 0;
+            sim->exitrequest.compare_exchange_strong(value, 1);
+            wrapper.WaitUntilDestroyed();
           },
-          py::call_guard<py::gil_scoped_release>(),
+          py::call_guard<py::gil_scoped_release>())
-          py::return_value_policy::reference_internal)
-      .def_readonly("ctrl_noise_std", &mujoco::Simulate::ctrl_noise_std,
-                    py::call_guard<py::gil_scoped_release>())
-      .def_readonly("ctrl_noise_rate", &mujoco::Simulate::ctrl_noise_rate,
-                    py::call_guard<py::gil_scoped_release>())
 
-      .def_readonly("real_time_index", &mujoco::Simulate::real_time_index,
+      .def_property_readonly("uiloadrequest",
-                    py::call_guard<py::gil_scoped_release>())
+                             CallIfNotNull(+[](mujoco::Simulate& sim) {
-      .def_readwrite("speed_changed", &mujoco::Simulate::speed_changed,
+                               return sim.uiloadrequest.load();
-                     py::call_guard<py::gil_scoped_release>())
+                             }),
-      .def_readwrite("measured_slowdown", &mujoco::Simulate::measured_slowdown,
+                             py::call_guard<py::gil_scoped_release>())
-                     py::call_guard<py::gil_scoped_release>())
+      .def("uiloadrequest_decrement", CallIfNotNull(+[](mujoco::Simulate& sim) {
-      .def_readonly("refresh_rate", &mujoco::Simulate::refresh_rate,
+             sim.uiloadrequest.fetch_sub(1);
-                    py::call_guard<py::gil_scoped_release>())
+           }),
-
+           py::call_guard<py::gil_scoped_release>())
-      .def_readonly("busywait", &mujoco::Simulate::busywait,
+      .def("update_hfield",
-                    py::call_guard<py::gil_scoped_release>())
+           CallIfNotNull(+[](mujoco::Simulate& sim, int hfieldid) {
-      .def_readonly("run", &mujoco::Simulate::run,
+             sim.UpdateHField(hfieldid);
-                    py::call_guard<py::gil_scoped_release>())
+           }),
+           py::call_guard<py::gil_scoped_release>())
+      .def("update_mesh", CallIfNotNull(+[](mujoco::Simulate& sim, int meshid) {
+             sim.UpdateMesh(meshid);
+           }),
+           py::call_guard<py::gil_scoped_release>())
+      .def("update_texture",
+           CallIfNotNull(+[](mujoco::Simulate& sim, int texid) {
+             sim.UpdateTexture(texid);
+           }),
+           py::call_guard<py::gil_scoped_release>())
 
       .def_property(
-          "exitrequest",
+          "droploadrequest", CallIfNotNull(+[](mujoco::Simulate& sim) {
-          [](SimulateWrapper& simulate) { return simulate.exitrequest.load(); },
+            return sim.droploadrequest.load();
-          [](SimulateWrapper& simulate, int exitrequest) {
+          }),
-            simulate.exitrequest.store(exitrequest);
+          CallIfNotNull(+[](mujoco::Simulate& sim, bool droploadrequest) {
-          },
+            sim.droploadrequest.store(droploadrequest);
+          }),
           py::call_guard<py::gil_scoped_release>())
-
+      .def_property_readonly("dropfilename",
-      .def_property_readonly(
+                             GetIfNotNull(&mujoco::Simulate::dropfilename),
-          "uiloadrequest",
+                             py::call_guard<py::gil_scoped_release>())
-          [](SimulateWrapper& simulate) {
+      .def_property_readonly("filename",
-            return simulate.uiloadrequest.load();
+                             GetIfNotNull(&mujoco::Simulate::filename),
-          },
+                             py::call_guard<py::gil_scoped_release>())
-          py::call_guard<py::gil_scoped_release>())
-      .def(
-          "uiloadrequest_decrement",
-          [](SimulateWrapper& simulate) {
-            simulate.uiloadrequest.fetch_sub(1);
-          },
-          py::call_guard<py::gil_scoped_release>())
-      .def(
-          "update_hfield",
-          [](SimulateWrapper& simulate, int hfieldid) {
-            simulate.UpdateHField(hfieldid);
-          },
-          py::call_guard<py::gil_scoped_release>())
-      .def(
-          "update_mesh",
-          [](SimulateWrapper& simulate, int meshid) {
-            simulate.UpdateMesh(meshid);
-          },
-          py::call_guard<py::gil_scoped_release>())
-      .def(
-          "update_texture",
-          [](SimulateWrapper& simulate, int texid) {
-            simulate.UpdateTexture(texid);
-          },
-          py::call_guard<py::gil_scoped_release>())
-
       .def_property(
-          "droploadrequest",
+          "load_error", GetIfNotNull(&mujoco::Simulate::load_error),
-          [](SimulateWrapper& simulate) {
+          CallIfNotNull(+[](mujoco::Simulate& sim, const std::string& error) {
-            return simulate.droploadrequest.load();
+            const auto max_length = sizeof_arr(sim.load_error);
-          },
+            std::strncpy(sim.load_error, error.c_str(), max_length - 1);
-          [](SimulateWrapper& simulate, bool droploadrequest) {
+            sim.load_error[max_length - 1] = '\0';
-            simulate.droploadrequest.store(droploadrequest);
+          }));
-          },
-          py::call_guard<py::gil_scoped_release>())
-      .def_property_readonly(
-          "dropfilename",
-          [](SimulateWrapper& simulate) -> std::string {
-            return simulate.dropfilename;
-          },
-          py::call_guard<py::gil_scoped_release>())
-      .def_property_readonly(
-          "filename",
-          [](SimulateWrapper& simulate) -> std::string {
-            return simulate.filename;
-          },
-          py::call_guard<py::gil_scoped_release>())
-      .def_property(
-          "load_error",
-          [](SimulateWrapper& simulate) -> std::string {
-            return simulate.load_error;
-          },
-          [](SimulateWrapper& simulate, const std::string& error) {
-            const auto max_length = sizeof_arr(simulate.load_error);
-            std::strncpy(simulate.load_error, error.c_str(), max_length - 1);
-            simulate.load_error[max_length - 1] = '\0';
-          });
 
   pymodule.def("set_glfw_dlhandle", [](std::uintptr_t dlhandle) {
     mujoco::Glfw(reinterpret_cast<void*>(dlhandle));

python/mujoco/viewer.py

@@ -96,7 +96,7 @@ class Handle:
   def close(self):
     sim = self._sim()
     if sim is not None:
-      sim.exitrequest = 1
+      sim.exit()
 
   def is_running(self) -> bool:
     sim = self._sim()
@@ -348,18 +348,19 @@ def _launch_internal(
     side_thread = threading.Thread(
         target=_reload, args=(simulate, loader, notify_loaded))
 
-  def make_exit_requester(simulate):
+  def make_exit(simulate):
-    def exit_requester():
+    def exit_simulate():
-      simulate.exitrequest = True
+      simulate.exit()
-    return exit_requester
+    return exit_simulate
 
-  exit_requester = make_exit_requester(simulate)
+  exit_simulate = make_exit(simulate)
-  atexit.register(exit_requester)
+  atexit.register(exit_simulate)
 
   side_thread.start()
   simulate.render_loop()
-  atexit.unregister(exit_requester)
+  atexit.unregister(exit_simulate)
   side_thread.join()
+  simulate.destroy()
 
 
 def launch(model: Optional[mujoco.MjModel] = None,

simulate/glfw_adapter.cc

@@ -110,6 +110,7 @@ GlfwAdapter::GlfwAdapter() {
 }
 
 GlfwAdapter::~GlfwAdapter() {
+  FreeMjrContext();
   Glfw().glfwMakeContextCurrent(nullptr);
   Glfw().glfwDestroyWindow(window_);
 }

simulate/platform_ui_adapter.cc

@@ -21,7 +21,7 @@ PlatformUIAdapter::PlatformUIAdapter() {
   mjr_defaultContext(&con_);
 }
 
-PlatformUIAdapter::~PlatformUIAdapter() {
+void PlatformUIAdapter::FreeMjrContext() {
   mjr_freeContext(&con_);
 }
 

simulate/platform_ui_adapter.h

@@ -22,7 +22,7 @@
 namespace mujoco {
 class PlatformUIAdapter {
  public:
-  virtual ~PlatformUIAdapter();
+  virtual ~PlatformUIAdapter() = default;
 
   inline mjuiState& state() { return state_; }
   inline const mjuiState& state() const { return state_; }
@@ -38,7 +38,7 @@ class PlatformUIAdapter {
     layout_callback_ = layout_callback;
   }
 
-  // Optionally overrideable function to (re)create an mjrContext for an mjModel
+  // Optionally overridable function to (re)create an mjrContext for an mjModel
   virtual bool RefreshMjrContext(const mjModel* m, int fontscale);
 
   virtual bool EnsureContextSize();
@@ -73,6 +73,7 @@ class PlatformUIAdapter {
 
  protected:
   PlatformUIAdapter();
+  void FreeMjrContext();
 
   // Event handlers
   void OnFilesDrop(int count, const char** paths);