src: use RAII for mutexes and condition variables

We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.

PR-URL: https://github.com/nodejs/node/pull/7334
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

node.gyp

@@ -175,6 +175,7 @@
         'src/node_http_parser.h',
         'src/node_internals.h',
         'src/node_javascript.h',
+        'src/node_mutex.h',
         'src/node_root_certs.h',
         'src/node_version.h',
         'src/node_watchdog.h',

src/debug-agent.cc

@@ -55,13 +55,7 @@ Agent::Agent(Environment* env) : state_(kNone),
                                  parent_env_(env),
                                  child_env_(nullptr),
                                  dispatch_handler_(nullptr) {
-  int err;
-
-  err = uv_sem_init(&start_sem_, 0);
-  CHECK_EQ(err, 0);
-
-  err = uv_mutex_init(&message_mutex_);
-  CHECK_EQ(err, 0);
+  CHECK_EQ(0, uv_sem_init(&start_sem_, 0));
 }
 
 
@@ -69,7 +63,6 @@ Agent::~Agent() {
   Stop();
 
   uv_sem_destroy(&start_sem_);
-  uv_mutex_destroy(&message_mutex_);
 
   while (AgentMessage* msg = messages_.PopFront())
     delete msg;
@@ -270,7 +263,7 @@ void Agent::ChildSignalCb(uv_async_t* signal) {
   HandleScope scope(isolate);
   Local<Object> api = PersistentToLocal(isolate, a->api_);
 
-  uv_mutex_lock(&a->message_mutex_);
+  Mutex::ScopedLock scoped_lock(a->message_mutex_);
   while (AgentMessage* msg = a->messages_.PopFront()) {
     // Time to close everything
     if (msg->data() == nullptr) {
@@ -301,14 +294,12 @@ void Agent::ChildSignalCb(uv_async_t* signal) {
                  argv);
     delete msg;
   }
-  uv_mutex_unlock(&a->message_mutex_);
 }
 
 
 void Agent::EnqueueMessage(AgentMessage* message) {
-  uv_mutex_lock(&message_mutex_);
+  Mutex::ScopedLock scoped_lock(message_mutex_);
   messages_.PushBack(message);
-  uv_mutex_unlock(&message_mutex_);
   uv_async_send(&child_signal_);
 }
 

src/debug-agent.h

@@ -24,6 +24,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#include "node_mutex.h"
 #include "util.h"
 #include "util-inl.h"
 #include "uv.h"
@@ -117,7 +118,7 @@ class Agent {
   bool wait_;
 
   uv_sem_t start_sem_;
-  uv_mutex_t message_mutex_;
+  node::Mutex message_mutex_;
   uv_async_t child_signal_;
 
   uv_thread_t thread_;

src/inspector_agent.cc

@@ -4,6 +4,7 @@
 #include "env.h"
 #include "env-inl.h"
 #include "node.h"
+#include "node_mutex.h"
 #include "node_version.h"
 #include "v8-platform.h"
 #include "util.h"
@@ -189,9 +190,9 @@ class AgentImpl {
   void Write(const std::string& message);
 
   uv_sem_t start_sem_;
-  uv_cond_t pause_cond_;
-  uv_mutex_t queue_lock_;
-  uv_mutex_t pause_lock_;
+  ConditionVariable pause_cond_;
+  Mutex pause_lock_;
+  Mutex queue_lock_;
   uv_thread_t thread_;
   uv_loop_t child_loop_;
 
@@ -290,9 +291,10 @@ class V8NodeInspector : public blink::V8Inspector {
     terminated_ = false;
     running_nested_loop_ = true;
     do {
-      uv_mutex_lock(&agent_->pause_lock_);
-      uv_cond_wait(&agent_->pause_cond_, &agent_->pause_lock_);
-      uv_mutex_unlock(&agent_->pause_lock_);
+      {
+        Mutex::ScopedLock scoped_lock(agent_->pause_lock_);
+        agent_->pause_cond_.Wait(scoped_lock);
+      }
       while (v8::platform::PumpMessageLoop(platform_, isolate_))
         {}
     } while (!terminated_);
@@ -321,9 +323,7 @@ AgentImpl::AgentImpl(Environment* env) : port_(0),
                                          inspector_(nullptr),
                                          platform_(nullptr),
                                          dispatching_messages_(false) {
-  int err;
-  err = uv_sem_init(&start_sem_, 0);
-  CHECK_EQ(err, 0);
+  CHECK_EQ(0, uv_sem_init(&start_sem_, 0));
   memset(&data_written_, 0, sizeof(data_written_));
   memset(&io_thread_req_, 0, sizeof(io_thread_req_));
 }
@@ -331,9 +331,6 @@ AgentImpl::AgentImpl(Environment* env) : port_(0),
 AgentImpl::~AgentImpl() {
   if (!inspector_)
     return;
-  uv_mutex_destroy(&queue_lock_);
-  uv_mutex_destroy(&pause_lock_);
-  uv_cond_destroy(&pause_cond_);
   uv_close(reinterpret_cast<uv_handle_t*>(&data_written_), nullptr);
 }
 
@@ -349,12 +346,6 @@ void AgentImpl::Start(v8::Platform* platform, int port, bool wait) {
   CHECK_EQ(err, 0);
   err = uv_async_init(env->event_loop(), &data_written_, nullptr);
   CHECK_EQ(err, 0);
-  err = uv_mutex_init(&queue_lock_);
-  CHECK_EQ(err, 0);
-  err = uv_mutex_init(&pause_lock_);
-  CHECK_EQ(err, 0);
-  err = uv_cond_init(&pause_cond_);
-  CHECK_EQ(err, 0);
 
   uv_unref(reinterpret_cast<uv_handle_t*>(&data_written_));
 
@@ -441,6 +432,7 @@ void AgentImpl::OnRemoteDataIO(uv_stream_t* stream,
                            const uv_buf_t* b) {
   inspector_socket_t* socket = static_cast<inspector_socket_t*>(stream->data);
   AgentImpl* agent = static_cast<AgentImpl*>(socket->data);
+  Mutex::ScopedLock scoped_lock(agent->pause_lock_);
   if (read > 0) {
     std::string str(b->base, read);
     agent->PushPendingMessage(&agent->message_queue_, str);
@@ -470,21 +462,19 @@ void AgentImpl::OnRemoteDataIO(uv_stream_t* stream,
     }
     DisconnectAndDisposeIO(socket);
   }
-  uv_cond_broadcast(&agent->pause_cond_);
+  agent->pause_cond_.Broadcast(scoped_lock);
 }
 
 void AgentImpl::PushPendingMessage(std::vector<std::string>* queue,
                                    const std::string& message) {
-  uv_mutex_lock(&queue_lock_);
+  Mutex::ScopedLock scoped_lock(queue_lock_);
   queue->push_back(message);
-  uv_mutex_unlock(&queue_lock_);
 }
 
 void AgentImpl::SwapBehindLock(std::vector<std::string> AgentImpl::*queue,
                                std::vector<std::string>* output) {
-  uv_mutex_lock(&queue_lock_);
+  Mutex::ScopedLock scoped_lock(queue_lock_);
   (this->*queue).swap(*output);
-  uv_mutex_unlock(&queue_lock_);
 }
 
 // static

src/node.cc

@@ -181,7 +181,7 @@ static double prog_start_time;
 static bool debugger_running;
 static uv_async_t dispatch_debug_messages_async;
 
-static uv_mutex_t node_isolate_mutex;
+static Mutex node_isolate_mutex;
 static v8::Isolate* node_isolate;
 static v8::Platform* default_platform;
 
@@ -3698,18 +3698,17 @@ static void EnableDebug(Environment* env) {
 
 // Called from an arbitrary thread.
 static void TryStartDebugger() {
-  uv_mutex_lock(&node_isolate_mutex);
+  Mutex::ScopedLock scoped_lock(node_isolate_mutex);
   if (auto isolate = node_isolate) {
     v8::Debug::DebugBreak(isolate);
     uv_async_send(&dispatch_debug_messages_async);
   }
-  uv_mutex_unlock(&node_isolate_mutex);
 }
 
 
 // Called from the main thread.
 static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
-  uv_mutex_lock(&node_isolate_mutex);
+  Mutex::ScopedLock scoped_lock(node_isolate_mutex);
   if (auto isolate = node_isolate) {
     if (debugger_running == false) {
       fprintf(stderr, "Starting debugger agent.\n");
@@ -3725,7 +3724,6 @@ static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
     Isolate::Scope isolate_scope(isolate);
     v8::Debug::ProcessDebugMessages(isolate);
   }
-  uv_mutex_unlock(&node_isolate_mutex);
 }
 
 
@@ -4059,8 +4057,6 @@ void Init(int* argc,
   // Make inherited handles noninheritable.
   uv_disable_stdio_inheritance();
 
-  CHECK_EQ(0, uv_mutex_init(&node_isolate_mutex));
-
   // init async debug messages dispatching
   // Main thread uses uv_default_loop
   uv_async_init(uv_default_loop(),
@@ -4254,12 +4250,13 @@ static void StartNodeInstance(void* arg) {
 #endif
   Isolate* isolate = Isolate::New(params);
 
-  uv_mutex_lock(&node_isolate_mutex);
+  {
+    Mutex::ScopedLock scoped_lock(node_isolate_mutex);
     if (instance_data->is_main()) {
       CHECK_EQ(node_isolate, nullptr);
       node_isolate = isolate;
     }
-  uv_mutex_unlock(&node_isolate_mutex);
+  }
 
   if (track_heap_objects) {
     isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
@@ -4331,10 +4328,11 @@ static void StartNodeInstance(void* arg) {
 #endif
   }
 
-  uv_mutex_lock(&node_isolate_mutex);
+  {
+    Mutex::ScopedLock scoped_lock(node_isolate_mutex);
     if (node_isolate == isolate)
       node_isolate = nullptr;
-  uv_mutex_unlock(&node_isolate_mutex);
+  }
 
   CHECK_NE(isolate, nullptr);
   isolate->Dispose();

src/node_crypto.cc

@@ -115,7 +115,7 @@ static X509_NAME *cnnic_ev_name =
     d2i_X509_NAME(nullptr, &cnnic_ev_p,
                   sizeof(CNNIC_EV_ROOT_CA_SUBJECT_DATA)-1);
 
-static uv_mutex_t* locks;
+static Mutex* mutexes;
 
 const char* const root_certs[] = {
 #include "node_root_certs.h"  // NOLINT(build/include_order)
@@ -182,14 +182,7 @@ static void crypto_threadid_cb(CRYPTO_THREADID* tid) {
 
 
 static void crypto_lock_init(void) {
-  int i, n;
-
-  n = CRYPTO_num_locks();
-  locks = new uv_mutex_t[n];
-
-  for (i = 0; i < n; i++)
-    if (uv_mutex_init(locks + i))
-      ABORT();
+  mutexes = new Mutex[CRYPTO_num_locks()];
 }
 
 
@@ -197,10 +190,11 @@ static void crypto_lock_cb(int mode, int n, const char* file, int line) {
   CHECK(!(mode & CRYPTO_LOCK) ^ !(mode & CRYPTO_UNLOCK));
   CHECK(!(mode & CRYPTO_READ) ^ !(mode & CRYPTO_WRITE));
 
+  auto mutex = &mutexes[n];
   if (mode & CRYPTO_LOCK)
-    uv_mutex_lock(locks + n);
+    mutex->Lock();
   else
-    uv_mutex_unlock(locks + n);
+    mutex->Unlock();
 }
 
 

src/node_mutex.h

@@ -0,0 +1,187 @@
+#ifndef SRC_NODE_MUTEX_H_
+#define SRC_NODE_MUTEX_H_
+
+#include "util.h"
+#include "uv.h"
+
+namespace node {
+
+template <typename Traits> class ConditionVariableBase;
+template <typename Traits> class MutexBase;
+struct LibuvMutexTraits;
+
+using ConditionVariable = ConditionVariableBase<LibuvMutexTraits>;
+using Mutex = MutexBase<LibuvMutexTraits>;
+
+template <typename Traits>
+class MutexBase {
+ public:
+  inline MutexBase();
+  inline ~MutexBase();
+  inline void Lock();
+  inline void Unlock();
+
+  class ScopedLock;
+  class ScopedUnlock;
+
+  class ScopedLock {
+   public:
+    inline explicit ScopedLock(const MutexBase& mutex);
+    inline explicit ScopedLock(const ScopedUnlock& scoped_unlock);
+    inline ~ScopedLock();
+
+   private:
+    template <typename> friend class ConditionVariableBase;
+    friend class ScopedUnlock;
+    const MutexBase& mutex_;
+    DISALLOW_COPY_AND_ASSIGN(ScopedLock);
+  };
+
+  class ScopedUnlock {
+   public:
+    inline explicit ScopedUnlock(const ScopedLock& scoped_lock);
+    inline ~ScopedUnlock();
+
+   private:
+    friend class ScopedLock;
+    const MutexBase& mutex_;
+    DISALLOW_COPY_AND_ASSIGN(ScopedUnlock);
+  };
+
+ private:
+  template <typename> friend class ConditionVariableBase;
+  mutable typename Traits::MutexT mutex_;
+  DISALLOW_COPY_AND_ASSIGN(MutexBase);
+};
+
+template <typename Traits>
+class ConditionVariableBase {
+ public:
+  using ScopedLock = typename MutexBase<Traits>::ScopedLock;
+
+  inline ConditionVariableBase();
+  inline ~ConditionVariableBase();
+  inline void Broadcast(const ScopedLock&);
+  inline void Signal(const ScopedLock&);
+  inline void Wait(const ScopedLock& scoped_lock);
+
+ private:
+  typename Traits::CondT cond_;
+  DISALLOW_COPY_AND_ASSIGN(ConditionVariableBase);
+};
+
+struct LibuvMutexTraits {
+  using CondT = uv_cond_t;
+  using MutexT = uv_mutex_t;
+
+  static inline int cond_init(CondT* cond) {
+    return uv_cond_init(cond);
+  }
+
+  static inline int mutex_init(MutexT* mutex) {
+    return uv_mutex_init(mutex);
+  }
+
+  static inline void cond_broadcast(CondT* cond) {
+    uv_cond_broadcast(cond);
+  }
+
+  static inline void cond_destroy(CondT* cond) {
+    uv_cond_destroy(cond);
+  }
+
+  static inline void cond_signal(CondT* cond) {
+    uv_cond_signal(cond);
+  }
+
+  static inline void cond_wait(CondT* cond, MutexT* mutex) {
+    uv_cond_wait(cond, mutex);
+  }
+
+  static inline void mutex_destroy(MutexT* mutex) {
+    uv_mutex_destroy(mutex);
+  }
+
+  static inline void mutex_lock(MutexT* mutex) {
+    uv_mutex_lock(mutex);
+  }
+
+  static inline void mutex_unlock(MutexT* mutex) {
+    uv_mutex_unlock(mutex);
+  }
+};
+
+template <typename Traits>
+ConditionVariableBase<Traits>::ConditionVariableBase() {
+  CHECK_EQ(0, Traits::cond_init(&cond_));
+}
+
+template <typename Traits>
+ConditionVariableBase<Traits>::~ConditionVariableBase() {
+  Traits::cond_destroy(&cond_);
+}
+
+template <typename Traits>
+void ConditionVariableBase<Traits>::Broadcast(const ScopedLock&) {
+  Traits::cond_broadcast(&cond_);
+}
+
+template <typename Traits>
+void ConditionVariableBase<Traits>::Signal(const ScopedLock&) {
+  Traits::cond_signal(&cond_);
+}
+
+template <typename Traits>
+void ConditionVariableBase<Traits>::Wait(const ScopedLock& scoped_lock) {
+  Traits::cond_wait(&cond_, &scoped_lock.mutex_.mutex_);
+}
+
+template <typename Traits>
+MutexBase<Traits>::MutexBase() {
+  CHECK_EQ(0, Traits::mutex_init(&mutex_));
+}
+
+template <typename Traits>
+MutexBase<Traits>::~MutexBase() {
+  Traits::mutex_destroy(&mutex_);
+}
+
+template <typename Traits>
+void MutexBase<Traits>::Lock() {
+  Traits::mutex_lock(&mutex_);
+}
+
+template <typename Traits>
+void MutexBase<Traits>::Unlock() {
+  Traits::mutex_unlock(&mutex_);
+}
+
+template <typename Traits>
+MutexBase<Traits>::ScopedLock::ScopedLock(const MutexBase& mutex)
+    : mutex_(mutex) {
+  Traits::mutex_lock(&mutex_.mutex_);
+}
+
+template <typename Traits>
+MutexBase<Traits>::ScopedLock::ScopedLock(const ScopedUnlock& scoped_unlock)
+    : MutexBase(scoped_unlock.mutex_) {}
+
+template <typename Traits>
+MutexBase<Traits>::ScopedLock::~ScopedLock() {
+  Traits::mutex_unlock(&mutex_.mutex_);
+}
+
+template <typename Traits>
+MutexBase<Traits>::ScopedUnlock::ScopedUnlock(const ScopedLock& scoped_lock)
+    : mutex_(scoped_lock.mutex_) {
+  Traits::mutex_unlock(&mutex_.mutex_);
+}
+
+template <typename Traits>
+MutexBase<Traits>::ScopedUnlock::~ScopedUnlock() {
+  Traits::mutex_lock(&mutex_.mutex_);
+}
+
+}  // namespace node
+
+#endif  // SRC_NODE_MUTEX_H_