mesos git commit: Fixed race in configureSignal and installCtrlHandler.

[al...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master 54e0c82dc -> 1c51c9863


Fixed race in configureSignal and installCtrlHandler.

When multiple agents are spawned in a single process, they attempt to
install signal handler concurrently, causing tests, such as
`ExamplesTest.PersistentVolumeFramework` and
`ExamplesTest.DynamicReservationFramework` randomly fail. This adds
missing synchronization to `configureSignal` and `installCtrlHandler`
to resolve the issue.

Review: https://reviews.apache.org/r/60467/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/1c51c986
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/1c51c986
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/1c51c986

Branch: refs/heads/master
Commit: 1c51c98638bb9ea0e8ec6a3f284b33d6c1a4e8ef
Parents: 54e0c82
Author: Dmitry Zhuk <dz...@twopensource.com>
Authored: Fri Oct 13 20:54:46 2017 -0700
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Fri Oct 13 20:57:08 2017 -0700

----------------------------------------------------------------------
 src/slave/posix_signalhandler.hpp | 41 +++++++++++++++++++++-------------
 src/slave/windows_ctrlhandler.hpp | 33 ++++++++++++++++-----------
 2 files changed, 45 insertions(+), 29 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/1c51c986/src/slave/posix_signalhandler.hpp
----------------------------------------------------------------------
diff --git a/src/slave/posix_signalhandler.hpp b/src/slave/posix_signalhandler.hpp
index 49692b1..9867bdf 100644
--- a/src/slave/posix_signalhandler.hpp
+++ b/src/slave/posix_signalhandler.hpp
@@ -13,15 +13,19 @@
 #ifndef __POSIX_SIGNALHANDLER_HPP__
 #define __POSIX_SIGNALHANDLER_HPP__
 
+#include <signal.h>
+
 #include <functional>
+#include <mutex>
+
+#include <stout/synchronized.hpp>
 
 namespace os {
 namespace internal {
 
 // Not using extern as this is used only by the executable. The signal
 // handler should be configured once. Configuring it multiple times will
-// overwrite any previous handlers. Configuring from multiple threads
-// simultaneously has undefined behavior.
+// overwrite any previous handlers.
 std::function<void(int, int)>* signaledWrapper = nullptr;
 
 void signalHandler(int sig, siginfo_t* siginfo, void* context)
@@ -34,27 +38,32 @@ void signalHandler(int sig, siginfo_t* siginfo, void* context)
 
 int configureSignal(const std::function<void(int, int)>* signal)
 {
-  // NOTE: We only expect this function to be called called multiple
+  // NOTE: We only expect this function to be called multiple
   // times inside tests and `mesos-local`.
-  if (signaledWrapper != nullptr) {
-    delete signaledWrapper;
-  }
 
-  struct sigaction action;
-  memset(&action, 0, sizeof(struct sigaction));
+  static std::mutex mutex;
 
-  signaledWrapper = new std::function<void(int, int)>(*signal);
+  synchronized (mutex) {
+    if (signaledWrapper != nullptr) {
+      delete signaledWrapper;
+    }
 
-  // Do not block additional signals while in the handler.
-  sigemptyset(&action.sa_mask);
+    struct sigaction action;
+    memset(&action, 0, sizeof(struct sigaction));
 
-  // The SA_SIGINFO flag tells `sigaction()` to use
-  // the sa_sigaction field, not sa_handler.
-  action.sa_flags = SA_SIGINFO;
+    signaledWrapper = new std::function<void(int, int)>(*signal);
 
-  action.sa_sigaction = signalHandler;
+    // Do not block additional signals while in the handler.
+    sigemptyset(&action.sa_mask);
 
-  return sigaction(SIGUSR1, &action, nullptr);
+    // The SA_SIGINFO flag tells `sigaction()` to use
+    // the sa_sigaction field, not sa_handler.
+    action.sa_flags = SA_SIGINFO;
+
+    action.sa_sigaction = signalHandler;
+
+    return sigaction(SIGUSR1, &action, nullptr);
+  }
 }
 
 } // namespace internal {

http://git-wip-us.apache.org/repos/asf/mesos/blob/1c51c986/src/slave/windows_ctrlhandler.hpp
----------------------------------------------------------------------
diff --git a/src/slave/windows_ctrlhandler.hpp b/src/slave/windows_ctrlhandler.hpp
index 145a232..cd73d68 100644
--- a/src/slave/windows_ctrlhandler.hpp
+++ b/src/slave/windows_ctrlhandler.hpp
@@ -14,6 +14,9 @@
 #define __WINDOWS_CTRLHANDLER_HPP__
 
 #include <functional>
+#include <mutex>
+
+#include <stout/synchronized.hpp>
 
 namespace os {
 namespace internal {
@@ -22,8 +25,7 @@ namespace internal {
 
 // Not using extern as this is used only by the executable. The signal
 // handler should be configured once. Configuring it multiple times will
-// overwrite any previous handlers. Configuring from multiple threads
-// simultaneously has undefined behavior.
+// overwrite any previous handlers.
 static std::function<void(int, int)>* signaledWrapper = nullptr;
 
 inline BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
@@ -49,19 +51,24 @@ inline BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
 
 inline int installCtrlHandler(const std::function<void(int, int)>* signal)
 {
-  // NOTE: We only expect this function to be called called multiple
+  // NOTE: We only expect this function to be called multiple
   // times inside tests and `mesos-local`.
-  if (signaledWrapper != nullptr) {
-    delete signaledWrapper;
-  }
 
-  if (signal != nullptr) {
-    signaledWrapper = new std::function<void(int, int)>(*signal);
-    return SetConsoleCtrlHandler(CtrlHandler, TRUE);
-  } else {
-    delete signaledWrapper;
-    signaledWrapper = nullptr;
-    return SetConsoleCtrlHandler(CtrlHandler, FALSE);
+  static std::mutex mutex;
+
+  synchronized (mutex) {
+    if (signaledWrapper != nullptr) {
+      delete signaledWrapper;
+    }
+
+    if (signal != nullptr) {
+      signaledWrapper = new std::function<void(int, int)>(*signal);
+      return SetConsoleCtrlHandler(CtrlHandler, TRUE);
+    } else {
+      delete signaledWrapper;
+      signaledWrapper = nullptr;
+      return SetConsoleCtrlHandler(CtrlHandler, FALSE);
+    }
   }
 }