Use std::shared_ptr for gz::transport::NodeShared

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();

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);

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();
 }
 
 //////////////////////////////////////////////////

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();

src/NodeShared.cc

@@ -152,47 +152,47 @@
 }
 
 //////////////////////////////////////////////////
+// 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;
 }
 
 //////////////////////////////////////////////////

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()