Merge pull request #489 from gazebosim/merge_13_main_20240322

Merge 13 -> main
gazebosim · Mar 25, 2024 · a624bf9 · a624bf9
2 parents 63ee52c + b8cb54a
commit a624bf9
Show file tree

Hide file tree

Showing 14 changed files with 300 additions and 131 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -4,6 +4,62 @@
 
 ## Gazebo Transport 13.X
 
+### Gazebo Transport 13.2.0 (2024-xx-xx)
+
+1. No input service request from the command line
+    * [Pull request #487](https://github.com/gazebosim/gz-transport/pull/487)
+
+1. Use `std::shared_ptr` for `gz::transport::NodeShared`
+    * [Pull request #484](https://github.com/gazebosim/gz-transport/pull/484)
+
+1. Use a default timeout when requesting a service from CLI.
+    * [Pull request #486](https://github.com/gazebosim/gz-transport/pull/486)
+
+1. Fix test failures when run under `colcon test`
+    * [Pull request #483](https://github.com/gazebosim/gz-transport/pull/483)
+
+### Gazebo Transport 13.1.0 (2024-03-14)
+
+1. Oneway service request from the command line
+    * [Pull request #477](https://github.com/gazebosim/gz-transport/pull/477)
+
+1. Re-enable tests of bash completion functions for gz
+    * [Pull request #481](https://github.com/gazebosim/gz-transport/pull/481)
+
+1. Find Python3 directly, not with GzPython
+    * [Pull request #472](https://github.com/gazebosim/gz-transport/pull/472)
+
+1. Fix issue #468
+    * [Pull request #470](https://github.com/gazebosim/gz-transport/pull/470)
+
+1. Test refactoring part 2
+    * [Pull request #463](https://github.com/gazebosim/gz-transport/pull/463)
+
+1. Update CI badges in README
+    * [Pull request #465](https://github.com/gazebosim/gz-transport/pull/465)
+
+1. Support for bazel on garden
+    * [Pull request #399](https://github.com/gazebosim/gz-transport/pull/399)
+
+1. Use subprocess rather than custom code
+    * [Pull request #429](https://github.com/gazebosim/gz-transport/pull/429)
+
+1. Infrastructure
+    * [Pull request #460](https://github.com/gazebosim/gz-transport/pull/460)
+
+1. Remove duplicated functionality from test_config
+    * [Pull request #457](https://github.com/gazebosim/gz-transport/pull/457)
+    * [Pull request #458](https://github.com/gazebosim/gz-transport/pull/458)
+
+1. Make empty constructor as peer to explicit
+    * [Pull request #453](https://github.com/gazebosim/gz-transport/pull/453)
+
+1. Adds the subcommands for the log command for bash completion
+    * [Pull request #451](https://github.com/gazebosim/gz-transport/pull/451)
+
+1. Adds the python bindings tutorial
+    * [Pull request #450](https://github.com/gazebosim/gz-transport/pull/450)
+
 ### Gazebo Transport 13.0.0 (2023-09-29)
 
 1. Fix Docker in Harmonic

diff --git a/include/gz/transport/NodeShared.hh b/include/gz/transport/NodeShared.hh
@@ -68,8 +68,14 @@ namespace gz
     {
       /// \brief NodeShared is a singleton. This method gets the
       /// NodeShared instance shared between all the nodes.
+      /// Note: This is deprecated. Please use \sa SharedInstance
       /// \return Pointer to the current NodeShared instance.
-      public: static NodeShared *Instance();
+      public: static NodeShared GZ_DEPRECATED(13) *Instance();
+
+      /// \brief NodeShared is a singleton. This method gets the
+      /// a reference counted NodeShared instance shared between all the nodes.
+      /// \return A shared_ptr to the current NodeShared instance.
+      public: static std::shared_ptr<NodeShared> SharedInstance();
 
       /// \brief Receive data and control messages.
       public: void RunReceptionTask();

diff --git a/log/src/Recorder.cc b/log/src/Recorder.cc
@@ -188,7 +188,7 @@ Recorder::Implementation::Implementation()
     this->OnMessageReceived(_data, _len, _info);
   };
 
-  auto shared = NodeShared::Instance();
+  auto shared = NodeShared::SharedInstance();
 
   this->discovery = std::make_unique<MsgDiscovery>(
       Uuid().ToString(), shared->discoveryIP, shared->msgDiscPort);

diff --git a/src/Node.cc b/src/Node.cc
@@ -77,13 +77,13 @@ namespace gz
     //////////////////////////////////////////////////
     int rcvHwm()
     {
-      return NodeShared::Instance()->RcvHwm();
+      return NodeShared::SharedInstance()->RcvHwm();
     }
 
     //////////////////////////////////////////////////
     int sndHwm()
     {
-      return NodeShared::Instance()->SndHwm();
+      return NodeShared::SharedInstance()->SndHwm();
     }
 
     //////////////////////////////////////////////////
@@ -104,14 +104,14 @@ namespace gz
     {
       /// \brief Default constructor.
       public: PublisherPrivate()
-        : shared(NodeShared::Instance())
+        : shared(NodeShared::SharedInstance())
       {
       }
 
       /// \brief Constructor
       /// \param[in] _publisher The message publisher.
       public: explicit PublisherPrivate(const MessagePublisher &_publisher)
-        : shared(NodeShared::Instance()),
+        : shared(NodeShared::SharedInstance()),
           publisher(_publisher)
       {
       }
@@ -189,7 +189,7 @@ namespace gz
 
       /// \brief Pointer to the object shared between all the nodes within the
       /// same process.
-      public: NodeShared *shared = nullptr;
+      public: std::shared_ptr<NodeShared> shared = nullptr;
 
       /// \brief The message publisher.
       public: MessagePublisher publisher;
@@ -864,7 +864,7 @@ bool Node::EnableStats(const std::string &_topic, bool _enable,
 //////////////////////////////////////////////////
 NodeShared *Node::Shared() const
 {
-  return this->dataPtr->shared;
+  return this->dataPtr->shared.get();
 }
 
 //////////////////////////////////////////////////

diff --git a/src/NodePrivate.hh b/src/NodePrivate.hh
@@ -18,6 +18,7 @@
 #ifndef GZ_TRANSPORT_NODEPRIVATE_HH_
 #define GZ_TRANSPORT_NODEPRIVATE_HH_
 
+#include <memory>
 #include <string>
 #include <unordered_set>
 
@@ -67,7 +68,7 @@ namespace gz
 
       /// \brief Pointer to the object shared between all the nodes within the
       /// same process.
-      public: NodeShared *shared = NodeShared::Instance();
+      public: std::shared_ptr<NodeShared> shared = NodeShared::SharedInstance();
 
       /// \brief Partition for this node.
       public: std::string partition = hostname() + ":" + username();

diff --git a/src/NodeShared.cc b/src/NodeShared.cc
@@ -152,47 +152,47 @@ void sendAuthErrorHelper(zmq::socket_t &_socket, const std::string &_err)
 }
 
 //////////////////////////////////////////////////
+// LCOV_EXCL_START
 NodeShared *NodeShared::Instance()
+{
+  // This is a deprecated function, but since it's public, the following ensures
+  // backward compatibility by instantiating a shared_ptr that never gets
+  // deleted.
+  static std::shared_ptr<NodeShared> nodeShared = NodeShared::SharedInstance();
+  return nodeShared.get();
+}
+// LCOV_EXCL_STOP
+
+//////////////////////////////////////////////////
+std::shared_ptr<NodeShared> NodeShared::SharedInstance()
 {
   // Create an instance of NodeShared per process so the ZMQ context
   // is not shared between different processes.
-
-  static std::shared_mutex mutex;
-  static std::unordered_map<unsigned int, NodeShared*> nodeSharedMap;
-
-  // Get current process ID.
-  auto pid = getProcessId();
+  static std::weak_ptr<NodeShared> nodeSharedWeak;
+  static std::mutex mutex;
 
   // Check if there's already a NodeShared instance for this process.
-  // Use a shared_lock so multiple threads can read simultaneously.
-  // This will only block if there's another thread locking exclusively
-  // for writing. Since most of the time threads will be reading,
-  // we make the read operation faster at the expense of making the write
-  // operation slower. Use exceptions for their zero-cost when successful.
-  try
-  {
-    std::shared_lock readLock(mutex);
-    return nodeSharedMap.at(pid);
-  }
-  catch (...)
-  {
-    // Multiple threads from the same process could have arrived here
-    // simultaneously, so after locking, we need to make sure that there's
-    // not an already constructed NodeShared instance for this process.
-    std::lock_guard writeLock(mutex);
-
-    auto iter = nodeSharedMap.find(pid);
-    if (iter != nodeSharedMap.end())
-    {
-      // There's already an instance for this process, return it.
-      return iter->second;
-    }
-
-    // No instance, construct a new one.
-    auto ret = nodeSharedMap.insert({pid, new NodeShared});
-    assert(ret.second);  // Insert operation should be successful.
-    return ret.first->second;
-  }
+  std::shared_ptr<NodeShared> nodeShared = nodeSharedWeak.lock();
+  if (nodeShared)
+    return nodeShared;
+
+  // Multiple threads from the same process could have arrived here
+  // simultaneously, so after locking, we need to make sure that there's
+  // not an already constructed NodeShared instance for this process.
+  std::lock_guard lock(mutex);
+  nodeShared = nodeSharedWeak.lock();
+  if (nodeShared)
+    return nodeShared;
+
+  // Class used to enable use of std::shared_ptr. This is needed because the
+  // constructor and destructor of NodeShared are protected.
+  class MakeSharedEnabler : public NodeShared {};
+  // No instance, construct a new one.
+  nodeShared = std::make_shared<MakeSharedEnabler>();
+  // Assign to weak_ptr so next time SharedInstance is called, we can return the
+  // instance we just created.
+  nodeSharedWeak = nodeShared;
+  return nodeShared;
 }
 
 //////////////////////////////////////////////////

diff --git a/src/cmd/CMakeLists.txt b/src/cmd/CMakeLists.txt
@@ -65,7 +65,7 @@ if (TARGET UNIT_gz_TEST)
     UNIT_gz_TEST
     PROPERTIES
     ENVIRONMENT
-      "GZ_CONFIG_PATH=${CMAKE_BINARY_DIR}/test/conf/$<CONFIG>;LD_LIBRARY_PATH=\"$<TARGET_FILE_DIR:${PROJECT_LIBRARY_TARGET_NAME}>\":${LD_LIBRARY_PATH}"
+      "GZ_CONFIG_PATH=${CMAKE_BINARY_DIR}/test/conf/$<CONFIG>;LD_LIBRARY_PATH=\"$<TARGET_FILE_DIR:${PROJECT_LIBRARY_TARGET_NAME}>\":$ENV{LD_LIBRARY_PATH}"
   )
 endif()
 

diff --git a/src/cmd/gz.cc b/src/cmd/gz.cc
@@ -251,17 +251,25 @@ extern "C" void cmdServiceReq(const char *_service,
   Node node;
   bool result;
 
-  // Request the service.
-  bool executed = node.Request(_service, *req, _timeout, *rep, result);
-  if (executed)
+  if (!strcmp(_repType, "gz.msgs.Empty"))
   {
-    if (result)
-      std::cout << rep->DebugString() << std::endl;
-    else
-      std::cout << "Service call failed" << std::endl;
+    // One-way service.
+    node.Request(_service, *req, 1000, *rep, result);
   }
   else
-    std::cerr << "Service call timed out" << std::endl;
+  {
+    // Two-way service.
+    bool executed = node.Request(_service, *req, _timeout, *rep, result);
+    if (executed)
+    {
+      if (result)
+        std::cout << rep->DebugString() << std::endl;
+      else
+        std::cout << "Service call failed" << std::endl;
+    }
+    else
+      std::cerr << "Service call timed out" << std::endl;
+  }
 }
 
 //////////////////////////////////////////////////

diff --git a/src/cmd/gz.hh b/src/cmd/gz.hh
@@ -44,24 +44,26 @@ extern "C" void cmdServiceList();
 /// E.g.: cmdTopicPub("/foo", "gz.msgs.StringMsg",
 ///                   "'data:\"Custom data\"');
 extern "C" void cmdTopicPub(const char *_topic,
-                                                       const char *_msgType,
-                                                       const char *_msgData);
+                            const char *_msgType,
+                            const char *_msgData);
 
 /// \brief External hook to execute 'gz service -r' from the command line.
 /// \param[in] _service Service name.
 /// \param[in] _reqType Message type used in the request.
 /// \param[in] _repType Message type used in the response.
+///                     If "gz.msgs.Empty" is used, the request will be one-way
+///                     and _repType and _timeout will be ignored.
 /// \param[in] _timeout The request will timeout after '_timeout' ms.
 /// \param[in] _reqData Input data sent in the request.
 /// The format expected is the same used by Protobuf DebugString().
 /// E.g.: cmdServiceReq("/bar", "gz.msgs.StringMsg",
 ///                     "gz.msgs.StringMsg", 1000,
 ///                     "'data:\"Custom data\"');
 extern "C" void cmdServiceReq(const char *_service,
-                                                         const char *_reqType,
-                                                         const char *_repType,
-                                                         const int _timeout,
-                                                         const char *_reqData);
+                              const char *_reqType,
+                              const char *_repType,
+                              const int _timeout,
+                              const char *_reqData);
 
 extern "C" {
   /// \brief Enum used for specifing the message output format for functions