From 5bd010779e8758f58234a102ff6c14bb8a9f6273 Mon Sep 17 00:00:00 2001
From: "Addisu Z. Taddese" <addisu@openrobotics.org>
Date: Wed, 13 Oct 2021 14:52:16 -0500
Subject: [PATCH] More thread safety fixes

Reorganize code so that the `ecm` and `updateInfo` variables are only
accessed from the Qt thread.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
---
 include/ignition/gazebo/gui/GuiRunner.hh      |  7 +++++-
 src/gui/GuiRunner.cc                          | 24 ++++++++++++++-----
 .../component_inspector/ComponentInspector.cc | 11 +--------
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/include/ignition/gazebo/gui/GuiRunner.hh b/include/ignition/gazebo/gui/GuiRunner.hh
index 92277fbf2e..cc8386945a 100644
--- a/include/ignition/gazebo/gui/GuiRunner.hh
+++ b/include/ignition/gazebo/gui/GuiRunner.hh
@@ -60,10 +60,15 @@ class IGNITION_GAZEBO_GUI_VISIBLE GuiRunner : public QObject
   /// \param[in] _res Response containing new state.
   private: void OnStateAsyncService(const msgs::SerializedStepMap &_res);
 
-  /// \brief Callback when a new state is received from the server.
+  /// \brief Callback when a new state is received from the server. Actual
+  /// updating of the ECM is delegated to OnStateQt
   /// \param[in] _msg New state message.
   private: void OnState(const msgs::SerializedStepMap &_msg);
 
+  /// \brief Called by the Qt thread to update the ECM with new state
+  /// \param[in] _msg New state message.
+  private: Q_INVOKABLE void OnStateQt(const msgs::SerializedStepMap &_msg);
+
   /// \brief Update the plugins.
   /// \todo(anyone) Move to GuiRunner::Implementation when porting to v5
   private: Q_INVOKABLE void UpdatePlugins();
diff --git a/src/gui/GuiRunner.cc b/src/gui/GuiRunner.cc
index 875fd3ba80..a023c9205d 100644
--- a/src/gui/GuiRunner.cc
+++ b/src/gui/GuiRunner.cc
@@ -30,13 +30,15 @@
 using namespace ignition;
 using namespace gazebo;
 
-/// \todo(anyone) Move to GuiRunner::Implementation when porting to v5
-/// \brief Mutex to protect the plugin update.
-static std::mutex gUpdateMutex;
+// Register SerializedStepMap to the Qt meta type system so we can pass objects
+// of this type in QMetaObject::invokeMethod
+Q_DECLARE_METATYPE(msgs::SerializedStepMap)
 
 /////////////////////////////////////////////////
 GuiRunner::GuiRunner(const std::string &_worldName)
 {
+  qRegisterMetaType<msgs::SerializedStepMap>();
+
   this->setProperty("worldName", QString::fromStdString(_worldName));
 
   auto win = gui::App()->findChild<ignition::gui::MainWindow *>();
@@ -109,14 +111,24 @@ void GuiRunner::OnState(const msgs::SerializedStepMap &_msg)
 {
   IGN_PROFILE_THREAD_NAME("GuiRunner::OnState");
   IGN_PROFILE("GuiRunner::Update");
+  // Since this function may be called from a transport thread, we push the
+  // OnStateQt function to the queue so that its called from the Qt thread. This
+  // ensures that only one thread has access to the ecm and updateInfo
+  // variables.
+  QMetaObject::invokeMethod(this, "OnStateQt", Qt::QueuedConnection,
+                            Q_ARG(msgs::SerializedStepMap, _msg));
+}
 
-  std::lock_guard<std::mutex> lock(gUpdateMutex);
+/////////////////////////////////////////////////
+void GuiRunner::OnStateQt(const msgs::SerializedStepMap &_msg)
+{
+  IGN_PROFILE_THREAD_NAME("Qt thread");
+  IGN_PROFILE("GuiRunner::Update");
   this->ecm.SetState(_msg.state());
 
   // Update all plugins
   this->updateInfo = convert<UpdateInfo>(_msg.stats());
-  QMetaObject::invokeMethod(this, "UpdatePlugins",
-      Qt::BlockingQueuedConnection);
+  this->UpdatePlugins();
   this->ecm.ClearNewlyCreatedEntities();
   this->ecm.ProcessRemoveEntityRequests();
 }
diff --git a/src/gui/plugins/component_inspector/ComponentInspector.cc b/src/gui/plugins/component_inspector/ComponentInspector.cc
index 809af3f19f..a628836956 100644
--- a/src/gui/plugins/component_inspector/ComponentInspector.cc
+++ b/src/gui/plugins/component_inspector/ComponentInspector.cc
@@ -91,9 +91,6 @@ namespace ignition::gazebo
 
     /// \brief Transport node for making command requests
     public: transport::Node node;
-
-    /// \brief Mutex to protect componentsModel
-    public: QMutex mutex;
   };
 }
 
@@ -340,8 +337,6 @@ void ComponentInspector::Update(const UpdateInfo &,
   if (this->dataPtr->paused)
     return;
 
-  QMutexLocker locker(&this->dataPtr->mutex);
-
   auto componentTypes = _ecm.ComponentTypes(this->dataPtr->entity);
 
   // List all components
@@ -436,11 +431,7 @@ void ComponentInspector::Update(const UpdateInfo &,
     // Add component to list
     else
     {
-      QMetaObject::invokeMethod(&this->dataPtr->componentsModel,
-          "AddComponentType",
-          Qt::DirectConnection,
-          Q_RETURN_ARG(QStandardItem *, item),
-          Q_ARG(ignition::gazebo::ComponentTypeId, typeId));
+      item = this->dataPtr->componentsModel.AddComponentType(typeId);
     }
 
     if (nullptr == item)