From f07b0989a0803dc143687cf7f67637db951af2f3 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 16 Dec 2015 18:13:09 -0700 Subject: [PATCH 01/20] Add LockMaster --- generate/templates/manual/include/nodegit.h | 132 ++++++++++++++++++++ generate/templates/templates/nodegit.cc | 49 ++++++++ 2 files changed, 181 insertions(+) create mode 100644 generate/templates/manual/include/nodegit.h diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h new file mode 100644 index 000000000..57b6d8f81 --- /dev/null +++ b/generate/templates/manual/include/nodegit.h @@ -0,0 +1,132 @@ +#ifndef NODEGIT_H +#define NODEGIT_H + +#include +#include +#include + +class LockMaster { + + std::set objects_to_lock; + std::vector object_mutexes; + + template + void AddLocks(const T *t) { + // by default, don't lock anything + } + + template<> + void AddLocks(const git_repository *repo) { + // when using a repo, lock the repo + objects_to_lock.insert(repo); + } + + template<> + void AddLocks(const git_index *index) { + // when using an index, lock the repo, or if there isn't one lock the index + const void *owner = git_index_owner(index); + if(!owner) { + owner = index; + } + objects_to_lock.insert(owner); + } + + // ... more locking rules would go here. According to an analysis of idefs.json, + // the following types are passed as non-const * and may require locking + // (some likely, some probably not): + // 'git_annotated_commit', + // 'git_blame_options', + // 'git_blob', + // 'git_buf', + // 'git_checkout_options', + // 'git_cherrypick_options', + // 'git_clone_options', + // 'git_commit', + // 'git_config', + // 'git_diff', + // 'git_diff_perfdata', + // 'git_error', + // 'git_fetch_options', + // 'git_fetch_options', + // 'git_filter', + // 'git_filter_list', + // 'git_hashsig', + // 'git_index', + // 'git_merge_file_input', + // 'git_merge_options', + // 'git_merge_options', + // 'git_note', + // 'git_note_iterator', + // 'git_object', + // 'git_odb', + // 'git_odb_object', + // 'git_oid', + // 'git_oidarray', + // 'git_packbuilder', + // 'git_patch', + // 'git_pathspec', + // 'git_push_options', + // 'git_rebase', + // 'git_rebase_options', + // 'git_refdb', + // 'git_reference', + // 'git_reflog', + // 'git_remote', + // 'git_remote_callbacks', + // 'git_remote_callbacks', + // 'git_repository', + // 'git_repository_init_options', + // 'git_revwalk', + // 'git_signature', + // 'git_stash_apply_options', + // 'git_status_list', + // 'git_strarray', + // 'git_submodule', + // 'git_submodule_update_options', + // 'git_tag', + // 'git_transfer_progress', + // 'git_transport', + // 'git_tree', + // 'git_treebuilder', + // 'git_writestream' + // + // Other types are always passed as const * and perhaps don't require locking + // (it's not a guarantee though) + + // base case for variadic template unwinding + void AddParameters() + { + } + + // processes a single parameter, then calls recursively on the rest + template + void AddParameters(const T *t, const Types*... args) + { + if(t) { + AddLocks(t); + } else { + } + AddParameters(args...); + } + + void Lock(); + void Unlock(); + +public: + + // we lock on construction + template LockMaster(bool emptyGuard, const Types*... types) + { + AddParameters(types...); + Lock(); + } + + // and unlock on destruction + ~LockMaster() + { + Unlock(); + } + +}; + +#endif diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index ae0946704..f198b0744 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -2,7 +2,12 @@ #include #include #include +#include +#include +#include +#include +#include "../include/nodegit.h" #include "../include/wrapper.h" #include "../include/functions/copy.h" {% each %} @@ -11,6 +16,9 @@ {% endif %} {% endeach %} +std::map mutexes; +uv_mutex_t map_mutex; + extern "C" void init(Local target) { // Initialize libgit2. git_libgit2_init(); @@ -23,6 +31,47 @@ extern "C" void init(Local target) { {{ cppClassName }}::InitializeComponent(target); {% endif %} {% endeach %} + + uv_mutex_init(&map_mutex); +} + +std::vector get_mutexes(const std::set libgit2_objects) +{ + std::vector result; + + uv_mutex_lock(&map_mutex); + + for (auto it = libgit2_objects.begin(); it != libgit2_objects.end(); it++) { + const void *object = *it; + if(object) { + // ensure we have an initialized mutex for each object + if(!mutexes[object]) { + mutexes[object] = (uv_mutex_t *)malloc(sizeof(uv_mutex_t)); + uv_mutex_init(mutexes[object]); + } + + result.push_back(mutexes[object]); + } + } + + uv_mutex_unlock(&map_mutex); + + return result; +} + +void LockMaster::Lock() +{ + object_mutexes = get_mutexes(objects_to_lock); + + // lock the mutex (note locking the mutexes one by one can lead to + // deadlocks... we might be better off locking them all at once + // (using trylock, then unlocking if they can't all be locked, then lock & wait, repeat...) + std::for_each(object_mutexes.begin(), object_mutexes.end(), uv_mutex_lock); +} + +void LockMaster::Unlock() +{ + std::for_each(object_mutexes.begin(), object_mutexes.end(), uv_mutex_unlock); } NODE_MODULE(nodegit, init) From 64ba08a781b18c56d0517f62b34029f43e938821 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 16 Dec 2015 18:13:50 -0700 Subject: [PATCH 02/20] Use LockMaster for sync and async functions --- generate/templates/partials/async_function.cc | 35 +++-- generate/templates/partials/sync_function.cc | 142 ++++++++++-------- generate/templates/templates/class_content.cc | 1 + 3 files changed, 100 insertions(+), 78 deletions(-) diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index 9ef4daede..4861c7831 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -80,6 +80,14 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { void {{ cppClassName }}::{{ cppFunctionName }}Worker::Execute() { giterr_clear(); + + { + LockMaster lockMaster(true{%each args|argsInfo as arg %} + {%if arg.cType|isPointer%}{%if not arg.cType|isDoublePointer%} + ,baton->{{ arg.name }} + {%endif%}{%endif%} + {%endeach%}); + {%if .|hasReturnType %} {{ return.cType }} result = {{ cFunctionName }}( {%else%} @@ -93,24 +101,25 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::Execute() { {%endeach%} ); - {%if return.isResultOrError %} - baton->error_code = result; - if (result < GIT_OK && giterr_last() != NULL) { - baton->error = git_error_dup(giterr_last()); - } + {%if return.isResultOrError %} + baton->error_code = result; + if (result < GIT_OK && giterr_last() != NULL) { + baton->error = git_error_dup(giterr_last()); + } - {%elsif return.isErrorCode %} - baton->error_code = result; + {%elsif return.isErrorCode %} + baton->error_code = result; - if (result != GIT_OK && giterr_last() != NULL) { - baton->error = git_error_dup(giterr_last()); - } + if (result != GIT_OK && giterr_last() != NULL) { + baton->error = git_error_dup(giterr_last()); + } - {%elsif not return.cType == 'void' %} + {%elsif not return.cType == 'void' %} - baton->result = result; + baton->result = result; - {%endif%} + {%endif%} + } } void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { diff --git a/generate/templates/partials/sync_function.cc b/generate/templates/partials/sync_function.cc index 8151fa114..ada0de62f 100644 --- a/generate/templates/partials/sync_function.cc +++ b/generate/templates/partials/sync_function.cc @@ -34,83 +34,95 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL) { {% endif %} -giterr_clear(); + giterr_clear(); + + { + LockMaster lockMaster(true{%each args|argsInfo as arg %} + {%if arg.cType|isPointer%}{%if not arg.isReturn%} + ,{%if arg.isSelf %} + Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() + {%else%} + from_{{ arg.name }} + {%endif%} + {%endif%}{%endif%} + {%endeach%}); -{%if .|hasReturnValue %} - {{ return.cType }} result = {%endif%}{{ cFunctionName }}( - {%each args|argsInfo as arg %} - {%if arg.isReturn %} - {%if not arg.shouldAlloc %}&{%endif%} - {%endif%} - {%if arg.isSelf %} -Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() - {%elsif arg.isReturn %} -{{ arg.name }} - {%else%} -from_{{ arg.name }} - {%endif%} - {%if not arg.lastArg %},{%endif%} - {%endeach%} - ); + {%if .|hasReturnValue %} + {{ return.cType }} result = {%endif%}{{ cFunctionName }}( + {%each args|argsInfo as arg %} + {%if arg.isReturn %} + {%if not arg.shouldAlloc %}&{%endif%} + {%endif%} + {%if arg.isSelf %} + Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() + {%elsif arg.isReturn %} + {{ arg.name }} + {%else%} + from_{{ arg.name }} + {%endif%} + {%if not arg.lastArg %},{%endif%} + {%endeach%} + ); -{%if .|hasReturnValue |and return.isErrorCode %} - if (result != GIT_OK) { - {%each args|argsInfo as arg %} - {%if arg.shouldAlloc %} - free({{ arg.name }}); - {%elsif arg | isOid %} - if (info[{{ arg.jsArg }}]->IsString()) { + {%if .|hasReturnValue |and return.isErrorCode %} + if (result != GIT_OK) { + {%each args|argsInfo as arg %} + {%if arg.shouldAlloc %} free({{ arg.name }}); - } - {%endif%} - {%endeach%} + {%elsif arg | isOid %} + if (info[{{ arg.jsArg }}]->IsString()) { + free({{ arg.name }}); + } + {%endif%} + {%endeach%} - if (giterr_last()) { - return Nan::ThrowError(giterr_last()->message); - } else { - return Nan::ThrowError("Unknown Error"); + if (giterr_last()) { + return Nan::ThrowError(giterr_last()->message); + } else { + return Nan::ThrowError("Unknown Error"); + } } - } -{%endif%} + {%endif%} -{% if cppFunctionName == "Free" %} - Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->ClearValue(); -} -{% endif %} + {% if cppFunctionName == "Free" %} + Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->ClearValue(); + } + {% endif %} -{%each args|argsInfo as arg %} - {%if arg | isOid %} - if (info[{{ arg.jsArg }}]->IsString()) { - free((void *)from_{{ arg.name }}); - } - {%endif%} -{%endeach%} + {%each args|argsInfo as arg %} + {%if arg | isOid %} + if (info[{{ arg.jsArg }}]->IsString()) { + free((void *)from_{{ arg.name }}); + } + {%endif%} + {%endeach%} -{%if not .|returnsCount %} - return info.GetReturnValue().Set(scope.Escape(Nan::Undefined())); -{%else%} - {%if return.cType | isPointer %} - // null checks on pointers - if (!result) { + {%if not .|returnsCount %} return info.GetReturnValue().Set(scope.Escape(Nan::Undefined())); - } - {%endif%} + {%else%} + {%if return.cType | isPointer %} + // null checks on pointers + if (!result) { + return info.GetReturnValue().Set(scope.Escape(Nan::Undefined())); + } + {%endif%} - Local to; - {%if .|returnsCount > 1 %} - Local toReturn = Nan::New(); - {%endif%} - {%each .|returnsInfo as _return %} - {%partial convertToV8 _return %} + Local to; {%if .|returnsCount > 1 %} - Nan::Set(toReturn, Nan::New("{{ _return.returnNameOrName }}").ToLocalChecked(), to); + Local toReturn = Nan::New(); + {%endif%} + {%each .|returnsInfo as _return %} + {%partial convertToV8 _return %} + {%if .|returnsCount > 1 %} + Nan::Set(toReturn, Nan::New("{{ _return.returnNameOrName }}").ToLocalChecked(), to); + {%endif%} + {%endeach%} + {%if .|returnsCount == 1 %} + return info.GetReturnValue().Set(scope.Escape(to)); + {%else%} + return info.GetReturnValue().Set(scope.Escape(toReturn)); {%endif%} - {%endeach%} - {%if .|returnsCount == 1 %} - return info.GetReturnValue().Set(scope.Escape(to)); - {%else%} - return info.GetReturnValue().Set(scope.Escape(toReturn)); {%endif%} -{%endif%} + } } diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 4d4c19ff7..63d1d6e42 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -9,6 +9,7 @@ extern "C" { {% endeach %} } +#include "../include/nodegit.h" #include "../include/functions/copy.h" #include "../include/{{ filename }}.h" #include "../include/functions/sleep_for_ms.h" From cb92954d480b38b00e0526e44a22e6c49b56a5bd Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Dec 2015 10:32:22 -0700 Subject: [PATCH 03/20] Move template specializations out of class definition --- generate/templates/manual/include/nodegit.h | 156 ++++++++++---------- 1 file changed, 78 insertions(+), 78 deletions(-) diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h index 57b6d8f81..80ad01898 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/nodegit.h @@ -15,84 +15,6 @@ class LockMaster { // by default, don't lock anything } - template<> - void AddLocks(const git_repository *repo) { - // when using a repo, lock the repo - objects_to_lock.insert(repo); - } - - template<> - void AddLocks(const git_index *index) { - // when using an index, lock the repo, or if there isn't one lock the index - const void *owner = git_index_owner(index); - if(!owner) { - owner = index; - } - objects_to_lock.insert(owner); - } - - // ... more locking rules would go here. According to an analysis of idefs.json, - // the following types are passed as non-const * and may require locking - // (some likely, some probably not): - // 'git_annotated_commit', - // 'git_blame_options', - // 'git_blob', - // 'git_buf', - // 'git_checkout_options', - // 'git_cherrypick_options', - // 'git_clone_options', - // 'git_commit', - // 'git_config', - // 'git_diff', - // 'git_diff_perfdata', - // 'git_error', - // 'git_fetch_options', - // 'git_fetch_options', - // 'git_filter', - // 'git_filter_list', - // 'git_hashsig', - // 'git_index', - // 'git_merge_file_input', - // 'git_merge_options', - // 'git_merge_options', - // 'git_note', - // 'git_note_iterator', - // 'git_object', - // 'git_odb', - // 'git_odb_object', - // 'git_oid', - // 'git_oidarray', - // 'git_packbuilder', - // 'git_patch', - // 'git_pathspec', - // 'git_push_options', - // 'git_rebase', - // 'git_rebase_options', - // 'git_refdb', - // 'git_reference', - // 'git_reflog', - // 'git_remote', - // 'git_remote_callbacks', - // 'git_remote_callbacks', - // 'git_repository', - // 'git_repository_init_options', - // 'git_revwalk', - // 'git_signature', - // 'git_stash_apply_options', - // 'git_status_list', - // 'git_strarray', - // 'git_submodule', - // 'git_submodule_update_options', - // 'git_tag', - // 'git_transfer_progress', - // 'git_transport', - // 'git_tree', - // 'git_treebuilder', - // 'git_writestream' - // - // Other types are always passed as const * and perhaps don't require locking - // (it's not a guarantee though) - // base case for variadic template unwinding void AddParameters() { @@ -129,4 +51,82 @@ class LockMaster { }; + +template<> inline void LockMaster::AddLocks(const git_repository *repo) { + // when using a repo, lock the repo + objects_to_lock.insert(repo); +} + +template<> inline void LockMaster::AddLocks(const git_index *index) { + // when using an index, lock the repo, or if there isn't one lock the index + const void *owner = git_index_owner(index); + if(!owner) { + owner = index; + } + objects_to_lock.insert(owner); +} + +// ... more locking rules would go here. According to an analysis of idefs.json, +// the following types are passed as non-const * and may require locking +// (some likely, some probably not): +// 'git_annotated_commit', +// 'git_blame_options', +// 'git_blob', +// 'git_buf', +// 'git_checkout_options', +// 'git_cherrypick_options', +// 'git_clone_options', +// 'git_commit', +// 'git_config', +// 'git_diff', +// 'git_diff_perfdata', +// 'git_error', +// 'git_fetch_options', +// 'git_fetch_options', +// 'git_filter', +// 'git_filter_list', +// 'git_hashsig', +// 'git_index', +// 'git_merge_file_input', +// 'git_merge_options', +// 'git_merge_options', +// 'git_note', +// 'git_note_iterator', +// 'git_object', +// 'git_odb', +// 'git_odb_object', +// 'git_oid', +// 'git_oidarray', +// 'git_packbuilder', +// 'git_patch', +// 'git_pathspec', +// 'git_push_options', +// 'git_rebase', +// 'git_rebase_options', +// 'git_refdb', +// 'git_reference', +// 'git_reflog', +// 'git_remote', +// 'git_remote_callbacks', +// 'git_remote_callbacks', +// 'git_repository', +// 'git_repository_init_options', +// 'git_revwalk', +// 'git_signature', +// 'git_stash_apply_options', +// 'git_status_list', +// 'git_strarray', +// 'git_submodule', +// 'git_submodule_update_options', +// 'git_tag', +// 'git_transfer_progress', +// 'git_transport', +// 'git_tree', +// 'git_treebuilder', +// 'git_writestream' +// +// Other types are always passed as const * and perhaps don't require locking +// (it's not a guarantee though) + + #endif From 81740e3ba55df8160c07b34d8f0da207e66e0382 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Dec 2015 10:33:00 -0700 Subject: [PATCH 04/20] Add repo locking for commits --- generate/templates/manual/include/nodegit.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h index 80ad01898..a70525bb9 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/nodegit.h @@ -66,6 +66,12 @@ template<> inline void LockMaster::AddLocks(const git_index *index) { objects_to_lock.insert(owner); } +template<> inline void LockMaster::AddLocks(const git_commit *commit) { + // when using a commit, lock the repo + const void *owner = git_commit_owner(commit); + objects_to_lock.insert(owner); +} + // ... more locking rules would go here. According to an analysis of idefs.json, // the following types are passed as non-const * and may require locking // (some likely, some probably not): From 12eca233b86e37aeac0f3c7d1f0951385832ad8c Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Dec 2015 13:33:46 -0700 Subject: [PATCH 05/20] Remove use of auto --- generate/templates/templates/nodegit.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index f198b0744..ecfa12d48 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -41,7 +41,7 @@ std::vector get_mutexes(const std::set libgit2_objec uv_mutex_lock(&map_mutex); - for (auto it = libgit2_objects.begin(); it != libgit2_objects.end(); it++) { + for (std::set::const_iterator it = libgit2_objects.begin(); it != libgit2_objects.end(); it++) { const void *object = *it; if(object) { // ensure we have an initialized mutex for each object From 7579e3ac1335db2475c334f882fa45628eac9568 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Dec 2015 16:09:55 -0700 Subject: [PATCH 06/20] Add temporary unlock during callbacks --- generate/templates/manual/include/nodegit.h | 13 ++++++++ .../templates/partials/callback_helpers.cc | 10 ++++-- .../templates/partials/field_accessors.cc | 10 ++++-- generate/templates/templates/nodegit.cc | 31 ++++++++++++++----- .../templates/templates/struct_content.cc | 1 + 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h index a70525bb9..9c05cba9b 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/nodegit.h @@ -31,6 +31,9 @@ class LockMaster { AddParameters(args...); } + void GetMutexes(); + void Register(); + void Unregister(); void Lock(); void Unlock(); @@ -40,15 +43,25 @@ class LockMaster { template LockMaster(bool emptyGuard, const Types*... types) { AddParameters(types...); + GetMutexes(); + Register(); Lock(); } // and unlock on destruction ~LockMaster() { + Unregister(); Unlock(); } + class TemporaryUnlock + { + LockMaster *lockMaster; + public: + TemporaryUnlock(); + ~TemporaryUnlock(); + }; }; diff --git a/generate/templates/partials/callback_helpers.cc b/generate/templates/partials/callback_helpers.cc index bf2a2080b..9298042a3 100644 --- a/generate/templates/partials/callback_helpers.cc +++ b/generate/templates/partials/callback_helpers.cc @@ -17,10 +17,14 @@ baton->done = false; uv_async_init(uv_default_loop(), &baton->req, (uv_async_cb) {{ cppFunctionName }}_{{ cbFunction.name }}_async); - uv_async_send(&baton->req); + { + LockMaster::TemporaryUnlock temporaryUnlock; + + uv_async_send(&baton->req); - while(!baton->done) { - sleep_for_ms(1); + while(!baton->done) { + sleep_for_ms(1); + } } {% each cbFunction|returnsInfo false true as _return %} diff --git a/generate/templates/partials/field_accessors.cc b/generate/templates/partials/field_accessors.cc index 8a9ef5a70..267baa83e 100644 --- a/generate/templates/partials/field_accessors.cc +++ b/generate/templates/partials/field_accessors.cc @@ -98,10 +98,14 @@ baton->done = false; uv_async_init(uv_default_loop(), &baton->req, (uv_async_cb) {{ field.name }}_async); - uv_async_send(&baton->req); + { + LockMaster::TemporaryUnlock temporaryUnlock; + + uv_async_send(&baton->req); - while(!baton->done) { - sleep_for_ms(1); + while(!baton->done) { + sleep_for_ms(1); + } } {% each field|returnsInfo false true as _return %} diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index ecfa12d48..86547f31c 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -18,6 +18,7 @@ std::map mutexes; uv_mutex_t map_mutex; +uv_key_t current_lock_master_key; extern "C" void init(Local target) { // Initialize libgit2. @@ -33,15 +34,14 @@ extern "C" void init(Local target) { {% endeach %} uv_mutex_init(&map_mutex); + uv_key_create(¤t_lock_master_key); } -std::vector get_mutexes(const std::set libgit2_objects) +void LockMaster::GetMutexes() { - std::vector result; - uv_mutex_lock(&map_mutex); - for (std::set::const_iterator it = libgit2_objects.begin(); it != libgit2_objects.end(); it++) { + for (std::set::const_iterator it = objects_to_lock.begin(); it != objects_to_lock.end(); it++) { const void *object = *it; if(object) { // ensure we have an initialized mutex for each object @@ -50,19 +50,25 @@ std::vector get_mutexes(const std::set libgit2_objec uv_mutex_init(mutexes[object]); } - result.push_back(mutexes[object]); + object_mutexes.push_back(mutexes[object]); } } uv_mutex_unlock(&map_mutex); +} - return result; +void LockMaster::Register() +{ + uv_key_set(¤t_lock_master_key, this); } -void LockMaster::Lock() +void LockMaster::Unregister() { - object_mutexes = get_mutexes(objects_to_lock); + uv_key_set(¤t_lock_master_key, NULL); +} +void LockMaster::Lock() +{ // lock the mutex (note locking the mutexes one by one can lead to // deadlocks... we might be better off locking them all at once // (using trylock, then unlocking if they can't all be locked, then lock & wait, repeat...) @@ -74,4 +80,13 @@ void LockMaster::Unlock() std::for_each(object_mutexes.begin(), object_mutexes.end(), uv_mutex_unlock); } +LockMaster::TemporaryUnlock::TemporaryUnlock() { + lockMaster = (LockMaster *)uv_key_get(¤t_lock_master_key); + lockMaster->Unlock(); +} + +LockMaster::TemporaryUnlock::~TemporaryUnlock() { + lockMaster->Lock(); +} + NODE_MODULE(nodegit, init) diff --git a/generate/templates/templates/struct_content.cc b/generate/templates/templates/struct_content.cc index 3520fa1a7..3389e3f19 100644 --- a/generate/templates/templates/struct_content.cc +++ b/generate/templates/templates/struct_content.cc @@ -15,6 +15,7 @@ extern "C" { } #include +#include "../include/nodegit.h" #include "../include/functions/copy.h" #include "../include/{{ filename }}.h" #include "../include/functions/sleep_for_ms.h" From 7d31f5275e4f3b197326d24bb727101613c2007d Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Fri, 18 Dec 2015 06:31:32 -0700 Subject: [PATCH 07/20] Test callback deadlock --- test/tests/index.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/tests/index.js b/test/tests/index.js index a221c0b01..abb3509e6 100644 --- a/test/tests/index.js +++ b/test/tests/index.js @@ -47,6 +47,8 @@ describe("Index", function() { newFile2: "and this will have more content" }; var fileNames = Object.keys(fileContent); + var test = this; + var addCallbacksCount = 0; return Promise.all(fileNames.map(function(fileName) { return writeFile( @@ -54,9 +56,19 @@ describe("Index", function() { fileContent[fileName]); })) .then(function() { - return index.addAll(); + return index.addAll(undefined, undefined, function() { + // ensure that the add callback is called, + // and that there is no deadlock if we call + // a sync libgit2 function from the callback + addCallbacksCount++; + test.repository.path(); + + return 0; // confirm add + }); }) .then(function() { + assert.equal(addCallbacksCount, 2); + var newFiles = index.entries().filter(function(entry) { return ~fileNames.indexOf(entry.path); }); From 908b8b59e371fecdeef85a019d028616a2a55726 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Fri, 18 Dec 2015 09:28:41 -0700 Subject: [PATCH 08/20] Clean up mutexes --- generate/templates/manual/include/nodegit.h | 5 ++- generate/templates/templates/nodegit.cc | 44 ++++++++++++++++++--- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h index 9c05cba9b..40b804174 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/nodegit.h @@ -4,11 +4,12 @@ #include #include #include +#include class LockMaster { std::set objects_to_lock; - std::vector object_mutexes; + std::vector > object_mutexes; template void AddLocks(const T *t) { @@ -36,6 +37,7 @@ class LockMaster { void Unregister(); void Lock(); void Unlock(); + void CleanupMutexes(); public: @@ -53,6 +55,7 @@ class LockMaster { { Unregister(); Unlock(); + CleanupMutexes(); } class TemporaryUnlock diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index 86547f31c..5fa4ef4d0 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include "../include/nodegit.h" #include "../include/wrapper.h" @@ -16,9 +17,30 @@ {% endif %} {% endeach %} -std::map mutexes; +std::map> mutexes; uv_mutex_t map_mutex; uv_key_t current_lock_master_key; +uv_async_t cleanup_mutexes_handle; + +void cleanup_mutexes(uv_async_t *async) { + uv_mutex_lock(&map_mutex); + + for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) + { + std::shared_ptr &mutex = it->second; + // if the mutex is only referenced by the mutexes map, + // we can destroy it (because any LockMaster that is using the mutex would + // hold it in its object_mutexes) + if (mutex.unique()) { + uv_mutex_destroy(mutex.get()); + it = mutexes.erase(it); + } else { + it++; + } + } + + uv_mutex_unlock(&map_mutex); +} extern "C" void init(Local target) { // Initialize libgit2. @@ -35,6 +57,7 @@ extern "C" void init(Local target) { uv_mutex_init(&map_mutex); uv_key_create(¤t_lock_master_key); + uv_async_init(uv_default_loop(), &cleanup_mutexes_handle, cleanup_mutexes); } void LockMaster::GetMutexes() @@ -46,8 +69,8 @@ void LockMaster::GetMutexes() if(object) { // ensure we have an initialized mutex for each object if(!mutexes[object]) { - mutexes[object] = (uv_mutex_t *)malloc(sizeof(uv_mutex_t)); - uv_mutex_init(mutexes[object]); + mutexes[object] = std::shared_ptr((uv_mutex_t *)malloc(sizeof(uv_mutex_t))); + uv_mutex_init(mutexes[object].get()); } object_mutexes.push_back(mutexes[object]); @@ -72,12 +95,23 @@ void LockMaster::Lock() // lock the mutex (note locking the mutexes one by one can lead to // deadlocks... we might be better off locking them all at once // (using trylock, then unlocking if they can't all be locked, then lock & wait, repeat...) - std::for_each(object_mutexes.begin(), object_mutexes.end(), uv_mutex_lock); + for(std::shared_ptr &mutex : object_mutexes) { + uv_mutex_lock(mutex.get()); + } } void LockMaster::Unlock() { - std::for_each(object_mutexes.begin(), object_mutexes.end(), uv_mutex_unlock); + for(std::shared_ptr &mutex : object_mutexes) { + uv_mutex_unlock(mutex.get()); + } +} + +void LockMaster::CleanupMutexes() +{ + // schedule mutex cleanup on the main event loop + // this somewhat delays and debounces cleanup (uv_async_send coalesces calls) + uv_async_send(&cleanup_mutexes_handle); } LockMaster::TemporaryUnlock::TemporaryUnlock() { From c33fa10cdbf0cf3363436cb5d2bfbfabfae562b4 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Fri, 18 Dec 2015 14:29:10 -0700 Subject: [PATCH 09/20] Ensure mutexes are locked all at the same time --- generate/templates/templates/nodegit.cc | 40 +++++++++++++++++++++---- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index 5fa4ef4d0..cafe883a7 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -92,12 +92,40 @@ void LockMaster::Unregister() void LockMaster::Lock() { - // lock the mutex (note locking the mutexes one by one can lead to - // deadlocks... we might be better off locking them all at once - // (using trylock, then unlocking if they can't all be locked, then lock & wait, repeat...) - for(std::shared_ptr &mutex : object_mutexes) { - uv_mutex_lock(mutex.get()); - } + std::vector >::iterator already_locked = object_mutexes.end(); + + // we will attempt to lock all the mutexes at the same time to avoid deadlocks + // note in most cases we are locking 0 or 1 mutexes. more than 1 implies + // passing objects with different repos/owners in the same call. + std::vector >::iterator it; + do + { + // go through all the mutexes and try to lock them + for(it = object_mutexes.begin(); it != object_mutexes.end(); it++) { + // if we already locked this mutex in a previous pass via uv_mutex_lock, + // we don't need to lock it again + if(it == already_locked) { + continue; + } + // first, try to lock (non-blocking) + bool success = uv_mutex_trylock(it->get()); + if(!success) { + // we have failed to lock a mutex... unlock everything we have locked + for(std::vector >::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { + uv_mutex_unlock(unlock_it->get()); + } + if(already_locked > it && already_locked != object_mutexes.end()) { + uv_mutex_unlock(already_locked->get()); + } + // now do a blocking lock on what we couldn't lock + uv_mutex_lock(it->get()); + // mark that we have already locked this one + // if there are more mutexes than this one, we will go back to locking everything + already_locked = it; + break; + } + } + } while(it != object_mutexes.end()); } void LockMaster::Unlock() From 65a998d37bbd1209621d1612c17adecdf6c690d8 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Fri, 18 Dec 2015 15:14:27 -0700 Subject: [PATCH 10/20] Add opt-in / opt-out mechanism for thread safety --- generate/templates/manual/include/nodegit.h | 20 ++++++++++++++++++++ generate/templates/templates/nodegit.cc | 19 +++++++++++++++++++ test/tests/index.js | 4 ++++ 3 files changed, 43 insertions(+) diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h index 40b804174..399654bb3 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/nodegit.h @@ -8,6 +8,8 @@ class LockMaster { + static bool enabled; + std::set objects_to_lock; std::vector > object_mutexes; @@ -44,6 +46,10 @@ class LockMaster { // we lock on construction template LockMaster(bool emptyGuard, const Types*... types) { + if(!enabled) { + return; + } + AddParameters(types...); GetMutexes(); Register(); @@ -53,6 +59,10 @@ class LockMaster { // and unlock on destruction ~LockMaster() { + if(!enabled) { + return; + } + Unregister(); Unlock(); CleanupMutexes(); @@ -65,6 +75,16 @@ class LockMaster { TemporaryUnlock(); ~TemporaryUnlock(); }; + + static void Enable() + { + enabled = true; + } + + static void Disable() + { + enabled = false; + } }; diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index cafe883a7..44583ce85 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -42,6 +42,14 @@ void cleanup_mutexes(uv_async_t *async) { uv_mutex_unlock(&map_mutex); } +void LockMasterEnable(const FunctionCallbackInfo& args) { + LockMaster::Enable(); +} + +void LockMasterDisable(const FunctionCallbackInfo& args) { + LockMaster::Disable(); +} + extern "C" void init(Local target) { // Initialize libgit2. git_libgit2_init(); @@ -55,6 +63,9 @@ extern "C" void init(Local target) { {% endif %} {% endeach %} + NODE_SET_METHOD(target, "enableThreadSafety", LockMasterEnable); + NODE_SET_METHOD(target, "disableThreadSafety", LockMasterDisable); + uv_mutex_init(&map_mutex); uv_key_create(¤t_lock_master_key); uv_async_init(uv_default_loop(), &cleanup_mutexes_handle, cleanup_mutexes); @@ -143,12 +154,20 @@ void LockMaster::CleanupMutexes() } LockMaster::TemporaryUnlock::TemporaryUnlock() { + if(!enabled) { + return; + } lockMaster = (LockMaster *)uv_key_get(¤t_lock_master_key); lockMaster->Unlock(); } LockMaster::TemporaryUnlock::~TemporaryUnlock() { + if(!enabled) { + return; + } lockMaster->Lock(); } +bool LockMaster::enabled = false; + NODE_MODULE(nodegit, init) diff --git a/test/tests/index.js b/test/tests/index.js index abb3509e6..ff88c727f 100644 --- a/test/tests/index.js +++ b/test/tests/index.js @@ -17,6 +17,9 @@ describe("Index", function() { var reposPath = local("../repos/workdir"); beforeEach(function() { + // enable thread safety for this test suite to test the deadlock scenario + NodeGit.enableThreadSafety(); + var test = this; return Repository.open(reposPath) @@ -31,6 +34,7 @@ describe("Index", function() { after(function() { this.index.clear(); + NodeGit.disableThreadSafety(); }); it("can get the index of a repo and examine entries", function() { From 040a4eee93c521f51f151a266a4ba16240d14bca Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 23 Dec 2015 14:37:39 -0700 Subject: [PATCH 11/20] Fix uv_mutex_trylock response handling --- generate/templates/templates/nodegit.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index 44583ce85..50d17ce97 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -119,8 +119,8 @@ void LockMaster::Lock() continue; } // first, try to lock (non-blocking) - bool success = uv_mutex_trylock(it->get()); - if(!success) { + bool failure = uv_mutex_trylock(it->get()); + if(failure) { // we have failed to lock a mutex... unlock everything we have locked for(std::vector >::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { uv_mutex_unlock(unlock_it->get()); From 5efab3f13fdf8dd02813d1923ec9423f20990e8f Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 30 Dec 2015 10:19:42 -0700 Subject: [PATCH 12/20] Remove use of shared_ptr --- generate/templates/manual/include/nodegit.h | 13 ++-- generate/templates/templates/nodegit.cc | 68 +++++++++++++-------- 2 files changed, 48 insertions(+), 33 deletions(-) diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/nodegit.h index 399654bb3..bdb1db714 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/nodegit.h @@ -4,14 +4,12 @@ #include #include #include -#include class LockMaster { static bool enabled; std::set objects_to_lock; - std::vector > object_mutexes; template void AddLocks(const T *t) { @@ -34,11 +32,11 @@ class LockMaster { AddParameters(args...); } - void GetMutexes(); + std::vector GetMutexes(int use_count_delta); void Register(); void Unregister(); - void Lock(); - void Unlock(); + void Lock(bool acquire_mutexes); + void Unlock(bool release_mutexes); void CleanupMutexes(); public: @@ -51,9 +49,8 @@ class LockMaster { } AddParameters(types...); - GetMutexes(); Register(); - Lock(); + Lock(true); } // and unlock on destruction @@ -64,7 +61,7 @@ class LockMaster { } Unregister(); - Unlock(); + Unlock(true); CleanupMutexes(); } diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index 50d17ce97..82b0d6b71 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -6,7 +6,6 @@ #include #include #include -#include #include "../include/nodegit.h" #include "../include/wrapper.h" @@ -17,7 +16,7 @@ {% endif %} {% endeach %} -std::map> mutexes; +std::map > mutexes; uv_mutex_t map_mutex; uv_key_t current_lock_master_key; uv_async_t cleanup_mutexes_handle; @@ -25,15 +24,19 @@ uv_async_t cleanup_mutexes_handle; void cleanup_mutexes(uv_async_t *async) { uv_mutex_lock(&map_mutex); - for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) + for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) { - std::shared_ptr &mutex = it->second; + uv_mutex_t *mutex = it->second.first; + unsigned use_count = it->second.second; // if the mutex is only referenced by the mutexes map, // we can destroy it (because any LockMaster that is using the mutex would // hold it in its object_mutexes) - if (mutex.unique()) { - uv_mutex_destroy(mutex.get()); - it = mutexes.erase(it); + if (!use_count) { + uv_mutex_destroy(mutex); + free(mutex); + std::map >::iterator to_erase = it; + it++; + mutexes.erase(to_erase); } else { it++; } @@ -71,24 +74,35 @@ extern "C" void init(Local target) { uv_async_init(uv_default_loop(), &cleanup_mutexes_handle, cleanup_mutexes); } -void LockMaster::GetMutexes() +std::vector LockMaster::GetMutexes(int use_count_delta) { + std::vector object_mutexes; + uv_mutex_lock(&map_mutex); for (std::set::const_iterator it = objects_to_lock.begin(); it != objects_to_lock.end(); it++) { const void *object = *it; if(object) { // ensure we have an initialized mutex for each object - if(!mutexes[object]) { - mutexes[object] = std::shared_ptr((uv_mutex_t *)malloc(sizeof(uv_mutex_t))); - uv_mutex_init(mutexes[object].get()); + std::map >::iterator mutex_it = mutexes.find(object); + if(mutex_it == mutexes.end()) { + mutex_it = mutexes.insert( + std::make_pair( + object, + std::make_pair((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) + ) + ).first; + uv_mutex_init(mutex_it->second.first); } - object_mutexes.push_back(mutexes[object]); + object_mutexes.push_back(mutex_it->second.first); + mutex_it->second.second += use_count_delta; } } uv_mutex_unlock(&map_mutex); + + return object_mutexes; } void LockMaster::Register() @@ -101,14 +115,16 @@ void LockMaster::Unregister() uv_key_set(¤t_lock_master_key, NULL); } -void LockMaster::Lock() +void LockMaster::Lock(bool acquire_mutexes) { - std::vector >::iterator already_locked = object_mutexes.end(); + std::vector object_mutexes = GetMutexes(acquire_mutexes * 1); + + std::vector::iterator already_locked = object_mutexes.end(); // we will attempt to lock all the mutexes at the same time to avoid deadlocks // note in most cases we are locking 0 or 1 mutexes. more than 1 implies // passing objects with different repos/owners in the same call. - std::vector >::iterator it; + std::vector::iterator it; do { // go through all the mutexes and try to lock them @@ -119,17 +135,17 @@ void LockMaster::Lock() continue; } // first, try to lock (non-blocking) - bool failure = uv_mutex_trylock(it->get()); + bool failure = uv_mutex_trylock(*it); if(failure) { // we have failed to lock a mutex... unlock everything we have locked - for(std::vector >::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { - uv_mutex_unlock(unlock_it->get()); + for(std::vector::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { + uv_mutex_unlock(*unlock_it); } if(already_locked > it && already_locked != object_mutexes.end()) { - uv_mutex_unlock(already_locked->get()); + uv_mutex_unlock(*already_locked); } // now do a blocking lock on what we couldn't lock - uv_mutex_lock(it->get()); + uv_mutex_lock(*it); // mark that we have already locked this one // if there are more mutexes than this one, we will go back to locking everything already_locked = it; @@ -139,10 +155,12 @@ void LockMaster::Lock() } while(it != object_mutexes.end()); } -void LockMaster::Unlock() +void LockMaster::Unlock(bool release_mutexes) { - for(std::shared_ptr &mutex : object_mutexes) { - uv_mutex_unlock(mutex.get()); + std::vector object_mutexes = GetMutexes(release_mutexes * -1); + + for(std::vector::iterator it=object_mutexes.begin(); it != object_mutexes.end(); it++) { + uv_mutex_unlock(*it); } } @@ -158,14 +176,14 @@ LockMaster::TemporaryUnlock::TemporaryUnlock() { return; } lockMaster = (LockMaster *)uv_key_get(¤t_lock_master_key); - lockMaster->Unlock(); + lockMaster->Unlock(false); } LockMaster::TemporaryUnlock::~TemporaryUnlock() { if(!enabled) { return; } - lockMaster->Lock(); + lockMaster->Lock(false); } bool LockMaster::enabled = false; From 4b506f7cbd0d36b825aa20f1a1d991b9f632b496 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 30 Dec 2015 17:38:33 -0700 Subject: [PATCH 13/20] Address c++11 warnings --- generate/templates/templates/binding.gyp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/generate/templates/templates/binding.gyp b/generate/templates/templates/binding.gyp index 673d80446..4d28d28e1 100644 --- a/generate/templates/templates/binding.gyp +++ b/generate/templates/templates/binding.gyp @@ -58,7 +58,8 @@ "WARNING_CFLAGS": [ "-Wno-unused-variable", "-Wint-conversions", - "-Wmissing-field-initializers" + "-Wmissing-field-initializers", + "-Wno-c++11-extensions" ] } } @@ -71,6 +72,18 @@ "_HAS_EXCEPTIONS=1" ] } + ], [ + "OS=='linux' and ' Date: Thu, 31 Dec 2015 08:12:24 -0700 Subject: [PATCH 14/20] Move LockMaster to its own files --- .../include/{nodegit.h => lock_master.h} | 6 +- generate/templates/manual/src/lock_master.cc | 153 ++++++++++++++++++ generate/templates/templates/binding.gyp | 1 + generate/templates/templates/class_content.cc | 2 +- generate/templates/templates/nodegit.cc | 150 +---------------- .../templates/templates/struct_content.cc | 2 +- 6 files changed, 162 insertions(+), 152 deletions(-) rename generate/templates/manual/include/{nodegit.h => lock_master.h} (97%) create mode 100644 generate/templates/manual/src/lock_master.cc diff --git a/generate/templates/manual/include/nodegit.h b/generate/templates/manual/include/lock_master.h similarity index 97% rename from generate/templates/manual/include/nodegit.h rename to generate/templates/manual/include/lock_master.h index bdb1db714..b24cf2470 100644 --- a/generate/templates/manual/include/nodegit.h +++ b/generate/templates/manual/include/lock_master.h @@ -1,5 +1,5 @@ -#ifndef NODEGIT_H -#define NODEGIT_H +#ifndef LOCK_MASTER_H +#define LOCK_MASTER_H #include #include @@ -73,6 +73,8 @@ class LockMaster { ~TemporaryUnlock(); }; + static void Initialize(); + static void Enable() { enabled = true; diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc new file mode 100644 index 000000000..c453c86a3 --- /dev/null +++ b/generate/templates/manual/src/lock_master.cc @@ -0,0 +1,153 @@ +#include +#include +#include +#include "../include/lock_master.h" + +std::map > mutexes; +uv_mutex_t map_mutex; +uv_key_t current_lock_master_key; +uv_async_t cleanup_mutexes_handle; + +void cleanup_mutexes(uv_async_t *async) { + uv_mutex_lock(&map_mutex); + + for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) + { + uv_mutex_t *mutex = it->second.first; + unsigned use_count = it->second.second; + // if the mutex is only referenced by the mutexes map, + // we can destroy it (because any LockMaster that is using the mutex would + // hold it in its object_mutexes) + if (!use_count) { + uv_mutex_destroy(mutex); + free(mutex); + std::map >::iterator to_erase = it; + it++; + mutexes.erase(to_erase); + } else { + it++; + } + } + + uv_mutex_unlock(&map_mutex); +} + +void LockMaster::Initialize() { + uv_mutex_init(&map_mutex); + uv_key_create(¤t_lock_master_key); + uv_async_init(uv_default_loop(), &cleanup_mutexes_handle, cleanup_mutexes); +} + +std::vector LockMaster::GetMutexes(int use_count_delta) +{ + std::vector object_mutexes; + + uv_mutex_lock(&map_mutex); + + for (std::set::const_iterator it = objects_to_lock.begin(); it != objects_to_lock.end(); it++) { + const void *object = *it; + if(object) { + // ensure we have an initialized mutex for each object + std::map >::iterator mutex_it = mutexes.find(object); + if(mutex_it == mutexes.end()) { + mutex_it = mutexes.insert( + std::make_pair( + object, + std::make_pair((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) + ) + ).first; + uv_mutex_init(mutex_it->second.first); + } + + object_mutexes.push_back(mutex_it->second.first); + mutex_it->second.second += use_count_delta; + } + } + + uv_mutex_unlock(&map_mutex); + + return object_mutexes; +} + +void LockMaster::Register() +{ + uv_key_set(¤t_lock_master_key, this); +} + +void LockMaster::Unregister() +{ + uv_key_set(¤t_lock_master_key, NULL); +} + +void LockMaster::Lock(bool acquire_mutexes) +{ + std::vector object_mutexes = GetMutexes(acquire_mutexes * 1); + + std::vector::iterator already_locked = object_mutexes.end(); + + // we will attempt to lock all the mutexes at the same time to avoid deadlocks + // note in most cases we are locking 0 or 1 mutexes. more than 1 implies + // passing objects with different repos/owners in the same call. + std::vector::iterator it; + do + { + // go through all the mutexes and try to lock them + for(it = object_mutexes.begin(); it != object_mutexes.end(); it++) { + // if we already locked this mutex in a previous pass via uv_mutex_lock, + // we don't need to lock it again + if(it == already_locked) { + continue; + } + // first, try to lock (non-blocking) + bool failure = uv_mutex_trylock(*it); + if(failure) { + // we have failed to lock a mutex... unlock everything we have locked + for(std::vector::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { + uv_mutex_unlock(*unlock_it); + } + if(already_locked > it && already_locked != object_mutexes.end()) { + uv_mutex_unlock(*already_locked); + } + // now do a blocking lock on what we couldn't lock + uv_mutex_lock(*it); + // mark that we have already locked this one + // if there are more mutexes than this one, we will go back to locking everything + already_locked = it; + break; + } + } + } while(it != object_mutexes.end()); +} + +void LockMaster::Unlock(bool release_mutexes) +{ + std::vector object_mutexes = GetMutexes(release_mutexes * -1); + + for(std::vector::iterator it=object_mutexes.begin(); it != object_mutexes.end(); it++) { + uv_mutex_unlock(*it); + } +} + +void LockMaster::CleanupMutexes() +{ + // schedule mutex cleanup on the main event loop + // this somewhat delays and debounces cleanup (uv_async_send coalesces calls) + uv_async_send(&cleanup_mutexes_handle); +} + +LockMaster::TemporaryUnlock::TemporaryUnlock() { + if(!enabled) { + return; + } + lockMaster = (LockMaster *)uv_key_get(¤t_lock_master_key); + lockMaster->Unlock(false); +} + +LockMaster::TemporaryUnlock::~TemporaryUnlock() { + if(!enabled) { + return; + } + lockMaster->Lock(false); +} + +bool LockMaster::enabled = false; diff --git a/generate/templates/templates/binding.gyp b/generate/templates/templates/binding.gyp index 4d28d28e1..2d2111aac 100644 --- a/generate/templates/templates/binding.gyp +++ b/generate/templates/templates/binding.gyp @@ -14,6 +14,7 @@ }, "sources": [ + "src/lock_master.cc", "src/nodegit.cc", "src/wrapper.cc", "src/functions/copy.cc", diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 63d1d6e42..969e88473 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -9,7 +9,7 @@ extern "C" { {% endeach %} } -#include "../include/nodegit.h" +#include "../include/lock_master.h" #include "../include/functions/copy.h" #include "../include/{{ filename }}.h" #include "../include/functions/sleep_for_ms.h" diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index 82b0d6b71..f3024f443 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -2,12 +2,11 @@ #include #include #include -#include #include #include #include -#include "../include/nodegit.h" +#include "../include/lock_master.h" #include "../include/wrapper.h" #include "../include/functions/copy.h" {% each %} @@ -16,35 +15,6 @@ {% endif %} {% endeach %} -std::map > mutexes; -uv_mutex_t map_mutex; -uv_key_t current_lock_master_key; -uv_async_t cleanup_mutexes_handle; - -void cleanup_mutexes(uv_async_t *async) { - uv_mutex_lock(&map_mutex); - - for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) - { - uv_mutex_t *mutex = it->second.first; - unsigned use_count = it->second.second; - // if the mutex is only referenced by the mutexes map, - // we can destroy it (because any LockMaster that is using the mutex would - // hold it in its object_mutexes) - if (!use_count) { - uv_mutex_destroy(mutex); - free(mutex); - std::map >::iterator to_erase = it; - it++; - mutexes.erase(to_erase); - } else { - it++; - } - } - - uv_mutex_unlock(&map_mutex); -} - void LockMasterEnable(const FunctionCallbackInfo& args) { LockMaster::Enable(); } @@ -69,123 +39,7 @@ extern "C" void init(Local target) { NODE_SET_METHOD(target, "enableThreadSafety", LockMasterEnable); NODE_SET_METHOD(target, "disableThreadSafety", LockMasterDisable); - uv_mutex_init(&map_mutex); - uv_key_create(¤t_lock_master_key); - uv_async_init(uv_default_loop(), &cleanup_mutexes_handle, cleanup_mutexes); -} - -std::vector LockMaster::GetMutexes(int use_count_delta) -{ - std::vector object_mutexes; - - uv_mutex_lock(&map_mutex); - - for (std::set::const_iterator it = objects_to_lock.begin(); it != objects_to_lock.end(); it++) { - const void *object = *it; - if(object) { - // ensure we have an initialized mutex for each object - std::map >::iterator mutex_it = mutexes.find(object); - if(mutex_it == mutexes.end()) { - mutex_it = mutexes.insert( - std::make_pair( - object, - std::make_pair((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) - ) - ).first; - uv_mutex_init(mutex_it->second.first); - } - - object_mutexes.push_back(mutex_it->second.first); - mutex_it->second.second += use_count_delta; - } - } - - uv_mutex_unlock(&map_mutex); - - return object_mutexes; -} - -void LockMaster::Register() -{ - uv_key_set(¤t_lock_master_key, this); -} - -void LockMaster::Unregister() -{ - uv_key_set(¤t_lock_master_key, NULL); -} - -void LockMaster::Lock(bool acquire_mutexes) -{ - std::vector object_mutexes = GetMutexes(acquire_mutexes * 1); - - std::vector::iterator already_locked = object_mutexes.end(); - - // we will attempt to lock all the mutexes at the same time to avoid deadlocks - // note in most cases we are locking 0 or 1 mutexes. more than 1 implies - // passing objects with different repos/owners in the same call. - std::vector::iterator it; - do - { - // go through all the mutexes and try to lock them - for(it = object_mutexes.begin(); it != object_mutexes.end(); it++) { - // if we already locked this mutex in a previous pass via uv_mutex_lock, - // we don't need to lock it again - if(it == already_locked) { - continue; - } - // first, try to lock (non-blocking) - bool failure = uv_mutex_trylock(*it); - if(failure) { - // we have failed to lock a mutex... unlock everything we have locked - for(std::vector::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { - uv_mutex_unlock(*unlock_it); - } - if(already_locked > it && already_locked != object_mutexes.end()) { - uv_mutex_unlock(*already_locked); - } - // now do a blocking lock on what we couldn't lock - uv_mutex_lock(*it); - // mark that we have already locked this one - // if there are more mutexes than this one, we will go back to locking everything - already_locked = it; - break; - } - } - } while(it != object_mutexes.end()); -} - -void LockMaster::Unlock(bool release_mutexes) -{ - std::vector object_mutexes = GetMutexes(release_mutexes * -1); - - for(std::vector::iterator it=object_mutexes.begin(); it != object_mutexes.end(); it++) { - uv_mutex_unlock(*it); - } + LockMaster::Initialize(); } -void LockMaster::CleanupMutexes() -{ - // schedule mutex cleanup on the main event loop - // this somewhat delays and debounces cleanup (uv_async_send coalesces calls) - uv_async_send(&cleanup_mutexes_handle); -} - -LockMaster::TemporaryUnlock::TemporaryUnlock() { - if(!enabled) { - return; - } - lockMaster = (LockMaster *)uv_key_get(¤t_lock_master_key); - lockMaster->Unlock(false); -} - -LockMaster::TemporaryUnlock::~TemporaryUnlock() { - if(!enabled) { - return; - } - lockMaster->Lock(false); -} - -bool LockMaster::enabled = false; - NODE_MODULE(nodegit, init) diff --git a/generate/templates/templates/struct_content.cc b/generate/templates/templates/struct_content.cc index 3389e3f19..e2e399d98 100644 --- a/generate/templates/templates/struct_content.cc +++ b/generate/templates/templates/struct_content.cc @@ -15,7 +15,7 @@ extern "C" { } #include -#include "../include/nodegit.h" +#include "../include/lock_master.h" #include "../include/functions/copy.h" #include "../include/{{ filename }}.h" #include "../include/functions/sleep_for_ms.h" From fb4603dfc71378fdb77aaf6aa667beb3a89dd035 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Fri, 1 Jan 2016 11:05:21 -0700 Subject: [PATCH 15/20] Reorganize and clean up code --- .../templates/manual/include/lock_master.h | 70 +++---- generate/templates/manual/src/lock_master.cc | 188 ++++++++++++------ 2 files changed, 162 insertions(+), 96 deletions(-) diff --git a/generate/templates/manual/include/lock_master.h b/generate/templates/manual/include/lock_master.h index b24cf2470..a98f44303 100644 --- a/generate/templates/manual/include/lock_master.h +++ b/generate/templates/manual/include/lock_master.h @@ -1,15 +1,13 @@ #ifndef LOCK_MASTER_H #define LOCK_MASTER_H -#include -#include -#include +class LockMasterImpl; class LockMaster { static bool enabled; - std::set objects_to_lock; + LockMasterImpl *impl; template void AddLocks(const T *t) { @@ -17,14 +15,12 @@ class LockMaster { } // base case for variadic template unwinding - void AddParameters() - { + void AddParameters() { } // processes a single parameter, then calls recursively on the rest template - void AddParameters(const T *t, const Types*... args) - { + void AddParameters(const T *t, const Types*... args) { if(t) { AddLocks(t); } else { @@ -32,56 +28,56 @@ class LockMaster { AddParameters(args...); } - std::vector GetMutexes(int use_count_delta); - void Register(); - void Unregister(); - void Lock(bool acquire_mutexes); - void Unlock(bool release_mutexes); - void CleanupMutexes(); - + void ConstructorImpl(); + void DestructorImpl(); + void ObjectToLock(const void *); public: // we lock on construction - template LockMaster(bool emptyGuard, const Types*... types) - { + template LockMaster(bool emptyGuard, const Types*... types) { if(!enabled) { return; } AddParameters(types...); - Register(); - Lock(true); + ConstructorImpl(); } // and unlock on destruction - ~LockMaster() - { + ~LockMaster() { if(!enabled) { return; } - - Unregister(); - Unlock(true); - CleanupMutexes(); + DestructorImpl(); } - class TemporaryUnlock - { - LockMaster *lockMaster; + class TemporaryUnlock { + LockMasterImpl *impl; + + void ConstructorImpl(); + void DestructorImpl(); public: - TemporaryUnlock(); - ~TemporaryUnlock(); + TemporaryUnlock() { + if(!enabled) { + return; + } + ConstructorImpl(); + } + ~TemporaryUnlock() { + if(!enabled) { + return; + } + DestructorImpl(); + } }; static void Initialize(); - static void Enable() - { + static void Enable() { enabled = true; } - static void Disable() - { + static void Disable() { enabled = false; } }; @@ -89,7 +85,7 @@ class LockMaster { template<> inline void LockMaster::AddLocks(const git_repository *repo) { // when using a repo, lock the repo - objects_to_lock.insert(repo); + ObjectToLock(repo); } template<> inline void LockMaster::AddLocks(const git_index *index) { @@ -98,13 +94,13 @@ template<> inline void LockMaster::AddLocks(const git_index *index) { if(!owner) { owner = index; } - objects_to_lock.insert(owner); + ObjectToLock(owner); } template<> inline void LockMaster::AddLocks(const git_commit *commit) { // when using a commit, lock the repo const void *owner = git_commit_owner(commit); - objects_to_lock.insert(owner); + ObjectToLock(owner); } // ... more locking rules would go here. According to an analysis of idefs.json, diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc index c453c86a3..11c888ede 100644 --- a/generate/templates/manual/src/lock_master.cc +++ b/generate/templates/manual/src/lock_master.cc @@ -1,15 +1,82 @@ #include #include +#include +#include #include + #include "../include/lock_master.h" -std::map > mutexes; -uv_mutex_t map_mutex; -uv_key_t current_lock_master_key; -uv_async_t cleanup_mutexes_handle; +// LockMaster implementation details +// implemented in a separate class to keep LockMaster opaque +class LockMasterImpl { + // STATIC variables / methods + + // A map from objects that are locked (or were locked), to information on their mutex + static std::map > mutexes; + // A mutex used for the mutexes map + static uv_mutex_t mapMutex; + + // A libuv key used to store the current thread-specific LockMasterImpl instance + static uv_key_t currentLockMasterKey; + + // A libuv async handle used to trigger / debounce mutex cleanup + static uv_async_t cleanupMutexesHandle; + // Cleans up any mutexes that are not currently used + static void CleanupMutexes(uv_async_t *async); + +public: + static void Initialize(); + + // INSTANCE variables / methods + +private: + // The set of objects this LockMaster is responsible for locking + std::set objectsToLock; + + // Mutexes locked by this LockMaster on construction and unlocked on destruction + std::vector GetMutexes(int useCountDelta); + void Register(); + void Unregister(); + void CleanupMutexes(); + +public: + static LockMasterImpl *CurrentLockMasterImpl() { + return (LockMasterImpl *)uv_key_get(¤tLockMasterKey); + } + + LockMasterImpl() { + Register(); + Lock(true); + } + + ~LockMasterImpl() { + Unregister(); + Unlock(true); + CleanupMutexes(); + } + + void ObjectToLock(const void *objectToLock) { + objectsToLock.insert(objectToLock); + } + + void Lock(bool acquireMutexes); + void Unlock(bool releaseMutexes); +}; -void cleanup_mutexes(uv_async_t *async) { - uv_mutex_lock(&map_mutex); +std::map > LockMasterImpl::mutexes; +uv_mutex_t LockMasterImpl::mapMutex; +uv_key_t LockMasterImpl::currentLockMasterKey; +uv_async_t LockMasterImpl::cleanupMutexesHandle; + + +void LockMasterImpl::Initialize() { + uv_mutex_init(&mapMutex); + uv_key_create(¤tLockMasterKey); + uv_async_init(uv_default_loop(), &cleanupMutexesHandle, CleanupMutexes); +} + +void LockMasterImpl::CleanupMutexes(uv_async_t *async) { + uv_mutex_lock(&mapMutex); for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) { @@ -17,7 +84,7 @@ void cleanup_mutexes(uv_async_t *async) { unsigned use_count = it->second.second; // if the mutex is only referenced by the mutexes map, // we can destroy it (because any LockMaster that is using the mutex would - // hold it in its object_mutexes) + // hold it in its objectMutexes) if (!use_count) { uv_mutex_destroy(mutex); free(mutex); @@ -29,70 +96,63 @@ void cleanup_mutexes(uv_async_t *async) { } } - uv_mutex_unlock(&map_mutex); + uv_mutex_unlock(&mapMutex); } void LockMaster::Initialize() { - uv_mutex_init(&map_mutex); - uv_key_create(¤t_lock_master_key); - uv_async_init(uv_default_loop(), &cleanup_mutexes_handle, cleanup_mutexes); + LockMasterImpl::Initialize(); } -std::vector LockMaster::GetMutexes(int use_count_delta) -{ - std::vector object_mutexes; +std::vector LockMasterImpl::GetMutexes(int useCountDelta) { + std::vector objectMutexes; - uv_mutex_lock(&map_mutex); + uv_mutex_lock(&mapMutex); - for (std::set::const_iterator it = objects_to_lock.begin(); it != objects_to_lock.end(); it++) { + for (std::set::const_iterator it = objectsToLock.begin(); it != objectsToLock.end(); it++) { const void *object = *it; if(object) { // ensure we have an initialized mutex for each object - std::map >::iterator mutex_it = mutexes.find(object); - if(mutex_it == mutexes.end()) { - mutex_it = mutexes.insert( + std::map >::iterator mutexIt = mutexes.find(object); + if(mutexIt == mutexes.end()) { + mutexIt = mutexes.insert( std::make_pair( object, std::make_pair((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) ) ).first; - uv_mutex_init(mutex_it->second.first); + uv_mutex_init(mutexIt->second.first); } - object_mutexes.push_back(mutex_it->second.first); - mutex_it->second.second += use_count_delta; + objectMutexes.push_back(mutexIt->second.first); + mutexIt->second.second += useCountDelta; } } - uv_mutex_unlock(&map_mutex); + uv_mutex_unlock(&mapMutex); - return object_mutexes; + return objectMutexes; } -void LockMaster::Register() -{ - uv_key_set(¤t_lock_master_key, this); +void LockMasterImpl::Register() { + uv_key_set(¤tLockMasterKey, this); } -void LockMaster::Unregister() -{ - uv_key_set(¤t_lock_master_key, NULL); +void LockMasterImpl::Unregister() { + uv_key_set(¤tLockMasterKey, NULL); } -void LockMaster::Lock(bool acquire_mutexes) -{ - std::vector object_mutexes = GetMutexes(acquire_mutexes * 1); +void LockMasterImpl::Lock(bool acquireMutexes) { + std::vector objectMutexes = GetMutexes(acquireMutexes * 1); - std::vector::iterator already_locked = object_mutexes.end(); + std::vector::iterator already_locked = objectMutexes.end(); // we will attempt to lock all the mutexes at the same time to avoid deadlocks // note in most cases we are locking 0 or 1 mutexes. more than 1 implies // passing objects with different repos/owners in the same call. std::vector::iterator it; - do - { + do { // go through all the mutexes and try to lock them - for(it = object_mutexes.begin(); it != object_mutexes.end(); it++) { + for(it = objectMutexes.begin(); it != objectMutexes.end(); it++) { // if we already locked this mutex in a previous pass via uv_mutex_lock, // we don't need to lock it again if(it == already_locked) { @@ -102,10 +162,10 @@ void LockMaster::Lock(bool acquire_mutexes) bool failure = uv_mutex_trylock(*it); if(failure) { // we have failed to lock a mutex... unlock everything we have locked - for(std::vector::iterator unlock_it = object_mutexes.begin(); unlock_it != it; unlock_it++) { - uv_mutex_unlock(*unlock_it); + for(std::vector::iterator unlockIt = objectMutexes.begin(); unlockIt != it; unlockIt++) { + uv_mutex_unlock(*unlockIt); } - if(already_locked > it && already_locked != object_mutexes.end()) { + if(already_locked > it && already_locked != objectMutexes.end()) { uv_mutex_unlock(*already_locked); } // now do a blocking lock on what we couldn't lock @@ -116,38 +176,48 @@ void LockMaster::Lock(bool acquire_mutexes) break; } } - } while(it != object_mutexes.end()); + } while(it != objectMutexes.end()); } -void LockMaster::Unlock(bool release_mutexes) -{ - std::vector object_mutexes = GetMutexes(release_mutexes * -1); +void LockMasterImpl::Unlock(bool releaseMutexes) { + std::vector objectMutexes = GetMutexes(releaseMutexes * -1); - for(std::vector::iterator it=object_mutexes.begin(); it != object_mutexes.end(); it++) { + for(std::vector::iterator it=objectMutexes.begin(); it != objectMutexes.end(); it++) { uv_mutex_unlock(*it); } } -void LockMaster::CleanupMutexes() -{ +void LockMasterImpl::CleanupMutexes() { // schedule mutex cleanup on the main event loop // this somewhat delays and debounces cleanup (uv_async_send coalesces calls) - uv_async_send(&cleanup_mutexes_handle); + uv_async_send(&cleanupMutexesHandle); } -LockMaster::TemporaryUnlock::TemporaryUnlock() { - if(!enabled) { - return; - } - lockMaster = (LockMaster *)uv_key_get(¤t_lock_master_key); - lockMaster->Unlock(false); + +// LockMaster + +void LockMaster::ConstructorImpl() { + impl = new LockMasterImpl(); } -LockMaster::TemporaryUnlock::~TemporaryUnlock() { - if(!enabled) { - return; - } - lockMaster->Lock(false); +void LockMaster::DestructorImpl() { + delete impl; +} + +void LockMaster::ObjectToLock(const void *objectToLock) { + impl->ObjectToLock(objectToLock); +} + + +// LockMaster::TemporaryUnlock + +void LockMaster::TemporaryUnlock::ConstructorImpl() { + impl = LockMasterImpl::CurrentLockMasterImpl(); + impl->Unlock(false); +} + +void LockMaster::TemporaryUnlock::DestructorImpl() { + impl->Lock(false); } bool LockMaster::enabled = false; From 90d80732f1b337d01127048c0e9216a7e5d41ed1 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Mon, 4 Jan 2016 07:58:46 -0700 Subject: [PATCH 16/20] Use auto, range-based for loops, and for_each --- generate/templates/manual/src/lock_master.cc | 28 +++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc index 11c888ede..d13106e81 100644 --- a/generate/templates/manual/src/lock_master.cc +++ b/generate/templates/manual/src/lock_master.cc @@ -3,6 +3,7 @@ #include #include #include +#include #include "../include/lock_master.h" @@ -78,7 +79,7 @@ void LockMasterImpl::Initialize() { void LockMasterImpl::CleanupMutexes(uv_async_t *async) { uv_mutex_lock(&mapMutex); - for(std::map >::iterator it=mutexes.begin(); it != mutexes.end(); ) + for (auto it = mutexes.begin(); it != mutexes.end(); ) { uv_mutex_t *mutex = it->second.first; unsigned use_count = it->second.second; @@ -88,7 +89,7 @@ void LockMasterImpl::CleanupMutexes(uv_async_t *async) { if (!use_count) { uv_mutex_destroy(mutex); free(mutex); - std::map >::iterator to_erase = it; + auto to_erase = it; it++; mutexes.erase(to_erase); } else { @@ -108,11 +109,10 @@ std::vector LockMasterImpl::GetMutexes(int useCountDelta) { uv_mutex_lock(&mapMutex); - for (std::set::const_iterator it = objectsToLock.begin(); it != objectsToLock.end(); it++) { - const void *object = *it; + for (auto object : objectsToLock) { if(object) { // ensure we have an initialized mutex for each object - std::map >::iterator mutexIt = mutexes.find(object); + auto mutexIt = mutexes.find(object); if(mutexIt == mutexes.end()) { mutexIt = mutexes.insert( std::make_pair( @@ -144,7 +144,7 @@ void LockMasterImpl::Unregister() { void LockMasterImpl::Lock(bool acquireMutexes) { std::vector objectMutexes = GetMutexes(acquireMutexes * 1); - std::vector::iterator already_locked = objectMutexes.end(); + auto alreadyLocked = objectMutexes.end(); // we will attempt to lock all the mutexes at the same time to avoid deadlocks // note in most cases we are locking 0 or 1 mutexes. more than 1 implies @@ -155,24 +155,22 @@ void LockMasterImpl::Lock(bool acquireMutexes) { for(it = objectMutexes.begin(); it != objectMutexes.end(); it++) { // if we already locked this mutex in a previous pass via uv_mutex_lock, // we don't need to lock it again - if(it == already_locked) { + if (it == alreadyLocked) { continue; } // first, try to lock (non-blocking) bool failure = uv_mutex_trylock(*it); if(failure) { // we have failed to lock a mutex... unlock everything we have locked - for(std::vector::iterator unlockIt = objectMutexes.begin(); unlockIt != it; unlockIt++) { - uv_mutex_unlock(*unlockIt); - } - if(already_locked > it && already_locked != objectMutexes.end()) { - uv_mutex_unlock(*already_locked); + std::for_each(objectMutexes.begin(), it, uv_mutex_unlock); + if (alreadyLocked > it && alreadyLocked != objectMutexes.end()) { + uv_mutex_unlock(*alreadyLocked); } // now do a blocking lock on what we couldn't lock uv_mutex_lock(*it); // mark that we have already locked this one // if there are more mutexes than this one, we will go back to locking everything - already_locked = it; + alreadyLocked = it; break; } } @@ -182,9 +180,7 @@ void LockMasterImpl::Lock(bool acquireMutexes) { void LockMasterImpl::Unlock(bool releaseMutexes) { std::vector objectMutexes = GetMutexes(releaseMutexes * -1); - for(std::vector::iterator it=objectMutexes.begin(); it != objectMutexes.end(); it++) { - uv_mutex_unlock(*it); - } + std::for_each(objectMutexes.begin(), objectMutexes.end(), uv_mutex_unlock); } void LockMasterImpl::CleanupMutexes() { From ab3d2da58d6d2e64fb8eeadb15d2d431bd15d781 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Mon, 4 Jan 2016 08:41:32 -0700 Subject: [PATCH 17/20] Use ObjectInfo instead of pair for readability --- generate/templates/manual/src/lock_master.cc | 34 +++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc index d13106e81..ad9a901f1 100644 --- a/generate/templates/manual/src/lock_master.cc +++ b/generate/templates/manual/src/lock_master.cc @@ -7,13 +7,24 @@ #include "../include/lock_master.h" +// information about a lockable object +// - the mutex used to lock it and the number of outstanding locks +struct ObjectInfo { + uv_mutex_t *mutex; + unsigned useCount; + + ObjectInfo(uv_mutex_t *mutex, unsigned useCount) + : mutex(mutex), useCount(useCount) + {} +}; + // LockMaster implementation details // implemented in a separate class to keep LockMaster opaque class LockMasterImpl { // STATIC variables / methods // A map from objects that are locked (or were locked), to information on their mutex - static std::map > mutexes; + static std::map mutexes; // A mutex used for the mutexes map static uv_mutex_t mapMutex; @@ -64,7 +75,7 @@ class LockMasterImpl { void Unlock(bool releaseMutexes); }; -std::map > LockMasterImpl::mutexes; +std::map LockMasterImpl::mutexes; uv_mutex_t LockMasterImpl::mapMutex; uv_key_t LockMasterImpl::currentLockMasterKey; uv_async_t LockMasterImpl::cleanupMutexesHandle; @@ -81,12 +92,11 @@ void LockMasterImpl::CleanupMutexes(uv_async_t *async) { for (auto it = mutexes.begin(); it != mutexes.end(); ) { - uv_mutex_t *mutex = it->second.first; - unsigned use_count = it->second.second; - // if the mutex is only referenced by the mutexes map, - // we can destroy it (because any LockMaster that is using the mutex would - // hold it in its objectMutexes) - if (!use_count) { + uv_mutex_t *mutex = it->second.mutex; + unsigned useCount = it->second.useCount; + // if the mutex is not used by any LockMasters, + // we can destroy it + if (!useCount) { uv_mutex_destroy(mutex); free(mutex); auto to_erase = it; @@ -117,14 +127,14 @@ std::vector LockMasterImpl::GetMutexes(int useCountDelta) { mutexIt = mutexes.insert( std::make_pair( object, - std::make_pair((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) + ObjectInfo((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) ) ).first; - uv_mutex_init(mutexIt->second.first); + uv_mutex_init(mutexIt->second.mutex); } - objectMutexes.push_back(mutexIt->second.first); - mutexIt->second.second += useCountDelta; + objectMutexes.push_back(mutexIt->second.mutex); + mutexIt->second.useCount += useCountDelta; } } From dbce34f9f6298c6df80f2e0771dcafef51a6c35f Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Tue, 5 Jan 2016 10:56:13 -0700 Subject: [PATCH 18/20] Initialize LockMaster before using it --- generate/templates/manual/include/lock_master.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generate/templates/manual/include/lock_master.h b/generate/templates/manual/include/lock_master.h index a98f44303..fcd419fdc 100644 --- a/generate/templates/manual/include/lock_master.h +++ b/generate/templates/manual/include/lock_master.h @@ -39,8 +39,8 @@ class LockMaster { return; } - AddParameters(types...); ConstructorImpl(); + AddParameters(types...); } // and unlock on destruction From 1c6ea17510dc155242b1db2b55e3c5be5fb81e36 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 6 Jan 2016 06:59:06 -0700 Subject: [PATCH 19/20] Remove unused else --- generate/templates/manual/include/lock_master.h | 1 - 1 file changed, 1 deletion(-) diff --git a/generate/templates/manual/include/lock_master.h b/generate/templates/manual/include/lock_master.h index fcd419fdc..76d3ca055 100644 --- a/generate/templates/manual/include/lock_master.h +++ b/generate/templates/manual/include/lock_master.h @@ -23,7 +23,6 @@ class LockMaster { void AddParameters(const T *t, const Types*... args) { if(t) { AddLocks(t); - } else { } AddParameters(args...); } From 7a5a37f48c0fa027fa420317d1ddb814512f5177 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 6 Jan 2016 10:51:46 -0700 Subject: [PATCH 20/20] Make thread safety enable / disable more robust to toggling --- generate/templates/manual/include/lock_master.h | 15 ++++++++++----- generate/templates/manual/src/lock_master.cc | 4 +++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/generate/templates/manual/include/lock_master.h b/generate/templates/manual/include/lock_master.h index 76d3ca055..5a321471c 100644 --- a/generate/templates/manual/include/lock_master.h +++ b/generate/templates/manual/include/lock_master.h @@ -35,6 +35,7 @@ class LockMaster { // we lock on construction template LockMaster(bool emptyGuard, const Types*... types) { if(!enabled) { + impl = NULL; return; } @@ -44,12 +45,14 @@ class LockMaster { // and unlock on destruction ~LockMaster() { - if(!enabled) { + if(!impl) { return; } DestructorImpl(); } + // TemporaryUnlock unlocks the LockMaster currently registered on the thread, + // and re-locks it on destruction. class TemporaryUnlock { LockMasterImpl *impl; @@ -57,13 +60,14 @@ class LockMaster { void DestructorImpl(); public: TemporaryUnlock() { - if(!enabled) { - return; - } + // We can't return here if enabled is false + // It's possible that a LockMaster was fully constructed and registered + // before the thread safety was disabled. + // So we rely on ConstructorImpl to abort if there is no registered LockMaster ConstructorImpl(); } ~TemporaryUnlock() { - if(!enabled) { + if(!impl) { return; } DestructorImpl(); @@ -72,6 +76,7 @@ class LockMaster { static void Initialize(); + // Enables the thread safety system static void Enable() { enabled = true; } diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc index ad9a901f1..49fb9b4db 100644 --- a/generate/templates/manual/src/lock_master.cc +++ b/generate/templates/manual/src/lock_master.cc @@ -219,7 +219,9 @@ void LockMaster::ObjectToLock(const void *objectToLock) { void LockMaster::TemporaryUnlock::ConstructorImpl() { impl = LockMasterImpl::CurrentLockMasterImpl(); - impl->Unlock(false); + if(impl) { + impl->Unlock(false); + } } void LockMaster::TemporaryUnlock::DestructorImpl() {