One NodeShared per process (#152)

* Remove Windows warnings (#151) Signed-off-by: Carlos Aguero <[email protected]> Signed-off-by: Juan Oxoby <[email protected]> * One NodeShared per process Signed-off-by: Juan Oxoby <[email protected]> * Remove warnings on Homebrew (#150) Signed-off-by: Carlos Aguero <[email protected]> Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Juan Oxoby <[email protected]> * Make NodeShared thread-safe Signed-off-by: Juan Oxoby <[email protected]> * General fixes Signed-off-by: Juan Oxoby <[email protected]> * Fix comments Signed-off-by: Juan Oxoby <[email protected]> * Remove blank line Signed-off-by: Juan Oxoby <[email protected]> * code_check fixes Signed-off-by: Juan Oxoby <[email protected]> * Prevent cpplint from complaining because of <shared_mutex> Signed-off-by: Juan Oxoby <[email protected]> * Rename variables Signed-off-by: Juan Oxoby <[email protected]> * Make it portable Signed-off-by: Juan Oxoby <[email protected]> * Use header windows.h Signed-off-by: Juan Oxoby <[email protected]> Co-authored-by: Carlos Agüero <[email protected]> Co-authored-by: Juan Oxoby <[email protected]> Co-authored-by: Louise Poubel <[email protected]>
gazebosim · Sep 4, 2020 · cd083e3 · cd083e3
1 parent 37f3176
commit cd083e3
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 21 deletions.
diff --git a/include/ignition/transport/Helpers.hh b/include/ignition/transport/Helpers.hh
@@ -68,6 +68,10 @@ namespace ignition
         const std::string &_orig,
         char _delim);
 
+    /// \brief Portable function to get the id of the current process.
+    /// \returns id of current process
+    unsigned int IGNITION_TRANSPORT_VISIBLE getProcessId();
+
     // Use safer functions on Windows
     #ifdef _MSC_VER
       #define ign_strcat strcat_s

diff --git a/src/Helpers.cc b/src/Helpers.cc
@@ -18,6 +18,12 @@
 #include <cstdlib>
 #include <string>
 
+#ifdef _WIN32
+#include <windows.h>
+#else
+#include <unistd.h>
+#endif
+
 #include "ignition/transport/Helpers.hh"
 
 namespace ignition
@@ -59,6 +65,16 @@ namespace ignition
       pieces.push_back(_orig.substr(pos1, _orig.size()-pos1));
       return pieces;
     }
+
+    //////////////////////////////////////////////////
+    unsigned int getProcessId()
+    {
+#ifdef _WIN32
+      return ::GetCurrentProcessId();
+#else
+      return ::getpid();
+#endif
+    }
     }
   }
 }
diff --git a/src/NodeShared.cc b/src/NodeShared.cc
@@ -28,9 +28,11 @@
 #include <iostream>
 #include <map>
 #include <mutex>
+#include <shared_mutex>  //NOLINT
 #include <string>
 #include <thread>
 #include <vector>
+#include <unordered_map>
 
 // TODO(anyone): Remove after fixing the warnings.
 #ifdef _MSC_VER
@@ -168,27 +170,45 @@ void sendAuthErrorHelper(zmq::socket_t &_socket, const std::string &_err)
 //////////////////////////////////////////////////
 NodeShared *NodeShared::Instance()
 {
-#ifdef _MSC_VER
-  // If we compile ign-transport as a shared library on Windows, we should
-  // never destruct NodeShared, unfortunately. It seems that WinSock does
-  // not behave well during the DLL teardown phase as a program exits, and
-  // this will confuse the ZeroMQ library into thinking that WinSock
-  // misbehaved, causing an assertion in ZeroMQ to fail and throw an exception
-  // while the program exits. This is a known issue:
-  //
-  // https://github.com/zeromq/libzmq/issues/1144
-  //
-  // An easy way of dodging this issue is to never destruct NodeShared. The
-  // Operating System will take care of cleaning up its resources when the
-  // application exits. We may want to consider a more elegant solution in
-  // the future. The zsys_shutdown() function in the czmq library may be able
-  // to provide some inspiration for solving this more cleanly.
-  static NodeShared *instance = new NodeShared();
-  return instance;
-#else
-  static NodeShared instance;
-  return &instance;
-#endif
+  // 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();
+
+  // 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;
+  }
 }
 
 //////////////////////////////////////////////////