Address reviewer feedback, support windows, add documentation

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

src/SignalHandler.cc

@@ -23,7 +23,11 @@
 #include <csignal> // NOLINT(*)
 #include <functional> // NOLINT(*)
 #include <fcntl.h>
-#include <unistd.h>
+#ifndef _WIN32
+  #include <unistd.h>
+#else
+  #include <io.h>
+#endif
 #include <map> // NOLINT(*)
 #include <mutex> // NOLINT(*)
 #include <utility> // NOLINT(*)
@@ -38,95 +42,129 @@ using namespace common;
 GZ_COMMON_VISIBLE std::map<int, std::function<void(int)>> gOnSignalWrappers;
 std::mutex gWrapperMutex;
 
+#ifdef _WIN32
+  define write _write
+  define read _read
+#endif
 namespace
 {
 
-class SelfPipe
-{
-  public:
-  static int pipeFd[2];
-
-  public:
-  static void Initialize();
-
-  public:
-  ~SelfPipe();
-
-  private:
-  SelfPipe();
-
-  private:
-  void CheckPipe();
-
-  private:
-  std::thread checkPipeThread;
-
-  private:
-  std::atomic<bool> running{false};
+/// \brief Index of the read file descriptor
+constexpr int kReadFd = 0;
+/// \brief Index of the write file descriptor
+constexpr int kWriteFd = 1;
+
+/// \brief Class to encalpsulate the self-pipe trick which is a way enable the user of
+/// non async-signal-safe functions in downstream signal handler
+/// callbacks. 
+/// 
+/// It works by creating a pipe between the actual signal handler and
+/// a servicing thread. When a signal is received the signal handler
+/// writes a byte to the pipe and returns. The servicing thread reads the
+/// byte from the pipe and calls all of the registered callbacks. Since
+/// the registered callbacks are called from a regular thread instead of
+/// an actual signal handler, the callbacks are free to use any function
+/// (e.g. call gzdbg).
+class SelfPipe {
+
+  /// \brief The two pipes the comprise the self-pipe
+  public: static int pipeFd[2];
+
+  /// \brief Static function to create a singleton SelfPipe object
+  public: static void Initialize();
+
+  /// \brief Destructor.
+  public: ~SelfPipe();
+
+  /// \brief Constructor
+  /// Creates the pipes, applies configuration flags and starts the servicing
+  /// thread
+  private: SelfPipe();
+
+  /// \brief Servicing thread
+  private: void CheckPipe();
+
+  /// \brief Handle for CheckPipe thread
+  private: std::thread checkPipeThread;
+
+  /// \brief Whether the program is running. This is set to true by the
+  /// Constructor and set to false by the destructor
+  private: std::atomic<bool> running{false};
 };
 
 int SelfPipe::pipeFd[2];
 
-void onSignalTopHalf(int _value)
-{
-  auto valueByte = static_cast<std::uint8_t>(_value);
-  if (write(SelfPipe::pipeFd[1], &valueByte, 1) == -1)
-  {
-    // TODO (azeey) Not clear what to do here.
-  }
-}
-
 /////////////////////////////////////////////////
 /// \brief Callback to execute when a signal is received.
+/// This simply writes a byte to a pipe and returns
 /// \param[in] _value Signal number.
-void onSignalBottomHalf(int _value)
+void onSignalWriteToSelfPipe(int _value)
 {
-  std::lock_guard<std::mutex> lock(gWrapperMutex);
-  // Send the signal to each wrapper
-  for (std::pair<int, std::function<void(int)>> func : gOnSignalWrappers)
+  auto valueByte = static_cast<std::uint8_t>(_value);
+  if (write(SelfPipe::pipeFd[kWriteFd], &valueByte, 1) == -1)
   {
-    func.second(_value);
+    // TODO (azeey) Not clear what to do here.
   }
 }
 
+/////////////////////////////////////////////////
 SelfPipe::SelfPipe()
 {
-  if (pipe(this->pipeFd))
+#ifdef _WIN32
+  if (_pipe(this->pipeFd, 256, O_BINARY) == -1)
+#else
+  if (pipe(this->pipeFd) == -1)
+#endif
   {
     gzerr << "Unable to create pipe\n";
   }
 
-  int flags = fcntl(this->pipeFd[1], F_GETFL);
-  fcntl(this->pipeFd[1], F_SETFL, flags | O_NONBLOCK);
-  // TODO(azeey) Handle errors
+#ifndef _WIN32
+  int flags = fcntl(this->pipeFd[kWriteFd], F_GETFL);
+  if (fcntl(this->pipeFd[kWriteFd], F_SETFL, flags | O_NONBLOCK) < 0)
+  {
+    gzerr << "Failed to set flags on pipe. Signal handling may not work properly" << std::endl;
+  }
+#endif
   this->running = true;
-  std::cout << "Initialized self pipe " << running << std::endl;
   this->checkPipeThread = std::thread(&SelfPipe::CheckPipe, this);
 }
 
+/////////////////////////////////////////////////
 SelfPipe::~SelfPipe()
 {
   this->running = false;
-  onSignalTopHalf(127);
+  // Write a dummy signal value to the pipe. This is not a real signal, but we
+  // need to wakeup the CheckPipe thread so it can cleanup properly. The value
+  // was chosen to make it clear that this is not one of the standard signals.
+  onSignalWriteToSelfPipe(127);
   this->checkPipeThread.join();
 }
+
+/////////////////////////////////////////////////
 void SelfPipe::Initialize()
 {
   // We actually need this object to be destructed in order to join the thread,
   // so we can't use gz::utils::NeverDestroyed here.
   static SelfPipe selfPipe;
 }
 
+/////////////////////////////////////////////////
 void SelfPipe::CheckPipe()
 {
   while (this->running)
   {
     std::uint8_t signal;
-    if (read(SelfPipe::pipeFd[0], &signal, 1) != -1)
+    if (read(SelfPipe::pipeFd[kReadFd], &signal, 1) != -1)
     {
       if (this->running)
       {
-        onSignalBottomHalf(signal);
+        std::lock_guard<std::mutex> lock(gWrapperMutex);
+        // Send the signal to each wrapper
+        for (std::pair<int, std::function<void(int)>> func : gOnSignalWrappers)
+        {
+          func.second(signal);
+        }
       }
     }
     else
@@ -166,14 +204,14 @@ SignalHandler::SignalHandler()
   std::lock_guard<std::mutex> lock(gWrapperMutex);
 
   SelfPipe::Initialize();
-  if (std::signal(SIGINT, onSignalTopHalf) == SIG_ERR)
+  if (std::signal(SIGINT, onSignalWriteToSelfPipe) == SIG_ERR)
   {
     gzerr << "Unable to catch SIGINT.\n"
            << " Please visit http://community.gazebosim.org for help.\n";
     return;
   }
 
-  if (std::signal(SIGTERM, onSignalTopHalf) == SIG_ERR)
+  if (std::signal(SIGTERM, onSignalWriteToSelfPipe) == SIG_ERR)
   {
     gzerr << "Unable to catch SIGTERM.\n"
            << " Please visit http://community.gazebosim.org for help.\n";

src/SignalHandler_TEST.cc

@@ -80,7 +80,7 @@ TEST(SignalHandler, Single)
   common::SignalHandler handler1;
   EXPECT_TRUE(handler1.AddCallback(handler1Cb));
   std::raise(SIGTERM);
-  std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  std::this_thread::sleep_for(std::chrono::milliseconds(11));
   EXPECT_EQ(SIGTERM, gHandler1Sig);
 }
 
@@ -100,7 +100,7 @@ TEST(SignalHandler, Multiple)
 
   std::raise(SIGINT);
 
-  std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  std::this_thread::sleep_for(std::chrono::milliseconds(11));
   EXPECT_EQ(-1, gHandler1Sig);
   EXPECT_EQ(-1, gHandler2Sig);
 
@@ -130,6 +130,7 @@ TEST(SignalHandler, InitFailure)
 
   std::raise(SIGINT);
 
+  std::this_thread::sleep_for(std::chrono::milliseconds(11));
   EXPECT_EQ(-1, gHandler1Sig);
   EXPECT_EQ(-1, gHandler2Sig);
 }