fix(sim): close_gui segfault (#290)

close_gui in sim doesnt segfault anymore by checking if the gui still has a value in the optional variable which will be set to None if the gui gets closed/deregistered

Author: juelg

python/rcs/sim/sim.py

@@ -1,4 +1,5 @@
 import atexit
+import contextlib
 import multiprocessing as mp
 import uuid
 from logging import getLogger
@@ -63,6 +64,7 @@ def __init__(self, mjmdl: str | PathLike | ModelComposer, cfg: SimConfig | None
         self._gui_client: Optional[_GuiClient] = None
         self._gui_process: Optional[mp.context.SpawnProcess] = None
         self._stop_event: Optional[EventClass] = None
+        self._gui_atexit_registered = False
         if cfg is not None:
             self.set_config(cfg)
 
@@ -72,17 +74,26 @@ def close_gui(self):
         if self._gui_process is not None:
             self._gui_process.join()
         self._stop_gui_server()
+        self._gui_uuid = None
+        self._gui_client = None
+        self._gui_process = None
+        self._stop_event = None
+        if self._gui_atexit_registered:
+            with contextlib.suppress(ValueError):
+                atexit.unregister(self.close_gui)
+            self._gui_atexit_registered = False
 
     def open_gui(self):
         if self._gui_uuid is None:
             self._gui_uuid = "rcs_" + str(uuid.uuid4())
             self._start_gui_server(self._gui_uuid)
-        if self._gui_client is None:
-            ctx = mp.get_context("spawn")
-            self._stop_event = ctx.Event()
-            self._gui_process = ctx.Process(
+        if self._gui_process is None or not self._gui_process.is_alive():
+            self._stop_event = self._mp_context.Event()
+            self._gui_process = self._mp_context.Process(
                 target=gui_loop,
                 args=(self._gui_uuid, self._stop_event),
             )
             self._gui_process.start()
-        atexit.register(self.close_gui)
+        if not self._gui_atexit_registered:
+            atexit.register(self.close_gui)
+            self._gui_atexit_registered = True

python/tests/test_sim_gui.py

@@ -0,0 +1,27 @@
+import multiprocessing
+from pathlib import Path
+from uuid import uuid4
+
+REPO_ROOT = Path(__file__).resolve().parents[2]
+
+
+def _gui_server_stop_then_step():
+    import os
+
+    os.chdir(REPO_ROOT)
+    from rcs.sim import Sim
+
+    sim = Sim(Path("assets/scenes/empty_world/scene.xml"))
+    sim._gui_uuid = "rcs_test_" + str(uuid4())
+    sim._start_gui_server(sim._gui_uuid)
+    sim.step(2)
+    sim.close_gui()
+    sim.step(1)
+
+
+def test_gui_server_can_be_stopped_before_later_steps():
+    ctx = multiprocessing.get_context("spawn")
+    proc = ctx.Process(target=_gui_server_stop_then_step)
+    proc.start()
+    proc.join(timeout=10)
+    assert proc.exitcode == 0, f"process exited with {proc.exitcode}"

src/sim/gui.h

@@ -75,7 +75,7 @@ class GuiClient {
  private:
   mjModel* m;
   mjData* d;
-  const std::string& id;
+  const std::string id;
   struct shm shm;
 };
 

src/sim/gui_client.cpp

@@ -9,12 +9,12 @@ namespace sim {
 using namespace boost::interprocess;
 
 GuiClient::GuiClient(const std::string& id)
-    : shm{.manager{open_only, id.c_str()},
-          .state_lock{open_only, (id + STATE_LOCK_POSTFIX).c_str()},
-          .info_lock{open_only, (id + INFO_LOCK_POSTFIX).c_str()}},
+    : m{nullptr},
+      d{nullptr},
       id{id},
-      m{nullptr},
-      d{nullptr} {
+      shm{.manager{open_only, id.c_str()},
+          .state_lock{open_only, (id + STATE_LOCK_POSTFIX).c_str()},
+          .info_lock{open_only, (id + INFO_LOCK_POSTFIX).c_str()}} {
   // setup shared memory
   std::tie(this->shm.info_byte, std::ignore) =
       this->shm.manager.find<bool>(INFO_BYTE);

src/sim/sim.cpp

@@ -176,11 +176,17 @@ void Sim::start_gui_server(const std::string& id) {
     throw std::runtime_error("Start gui server should be called only once.");
   }
   this->gui.emplace(this->m, this->d, id);
-  this->register_cb(
-      std::bind(&GuiServer::update_mjdata_callback, &this->gui.value()), 0);
+  if (!this->gui_callback_registered) {
+    this->register_cb(
+        [this]() {
+          if (this->gui.has_value()) {
+            this->gui->update_mjdata_callback();
+          }
+        },
+        0);
+    this->gui_callback_registered = true;
+  }
 }
-// TODO: when stop_gui_server is called, the callback still exists but now
-// points to an non existing gui
 void Sim::stop_gui_server() { this->gui.reset(); }
 }  // namespace sim
 }  // namespace rcs

src/sim/sim.h

@@ -68,6 +68,7 @@ class Sim {
   size_t convergence_steps = 0;
   bool converged = true;
   std::optional<GuiServer> gui;
+  bool gui_callback_registered = false;
 
  public:
   // TODO: hide m & d, pass as parameter to callback (easier refactoring)