Skip to content

Commit

Permalink
src: use RAII for mutexes and condition variables
Browse files Browse the repository at this point in the history
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: #7334
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
  • Loading branch information
bnoordhuis authored and MylesBorins committed Oct 26, 2016
1 parent 9f1b790 commit a5b6c2c
Show file tree
Hide file tree
Showing 6 changed files with 212 additions and 40 deletions.
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,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',
Expand Down
15 changes: 3 additions & 12 deletions src/debug-agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,14 @@ 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));
}


Agent::~Agent() {
Stop();

uv_sem_destroy(&start_sem_);
uv_mutex_destroy(&message_mutex_);

while (AgentMessage* msg = messages_.PopFront())
delete msg;
Expand Down Expand Up @@ -274,7 +267,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) {
Expand Down Expand Up @@ -305,14 +298,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_);
}

Expand Down
3 changes: 2 additions & 1 deletion src/debug-agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#ifndef SRC_DEBUG_AGENT_H_
#define SRC_DEBUG_AGENT_H_

#include "node_mutex.h"
#include "util.h"
#include "util-inl.h"
#include "uv.h"
Expand Down Expand Up @@ -115,7 +116,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_;
Expand Down
30 changes: 14 additions & 16 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,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;

Expand Down Expand Up @@ -3535,18 +3535,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");
Expand All @@ -3562,7 +3561,6 @@ static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
Isolate::Scope isolate_scope(isolate);
v8::Debug::ProcessDebugMessages();
}
uv_mutex_unlock(&node_isolate_mutex);
}


Expand Down Expand Up @@ -3888,8 +3886,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
CHECK_EQ(0, uv_async_init(uv_default_loop(),
Expand Down Expand Up @@ -4177,12 +4173,13 @@ static void StartNodeInstance(void* arg) {
#endif
Isolate* isolate = Isolate::New(params);

uv_mutex_lock(&node_isolate_mutex);
if (instance_data->is_main()) {
CHECK_EQ(node_isolate, nullptr);
node_isolate = isolate;
{
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);
Expand Down Expand Up @@ -4251,10 +4248,11 @@ static void StartNodeInstance(void* arg) {
env = nullptr;
}

uv_mutex_lock(&node_isolate_mutex);
if (node_isolate == isolate)
node_isolate = nullptr;
uv_mutex_unlock(&node_isolate_mutex);
{
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
if (node_isolate == isolate)
node_isolate = nullptr;
}

CHECK_NE(isolate, nullptr);
isolate->Dispose();
Expand Down
16 changes: 5 additions & 11 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,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)
Expand Down Expand Up @@ -172,25 +172,19 @@ 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()];
}


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


Expand Down
187 changes: 187 additions & 0 deletions src/node_mutex.h
Original file line number Diff line number Diff line change
@@ -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_

0 comments on commit a5b6c2c

Please sign in to comment.