diff --git a/generate/templates/manual/include/lock_master.h b/generate/templates/manual/include/lock_master.h new file mode 100644 index 000000000..5a321471c --- /dev/null +++ b/generate/templates/manual/include/lock_master.h @@ -0,0 +1,173 @@ +#ifndef LOCK_MASTER_H +#define LOCK_MASTER_H + +class LockMasterImpl; + +class LockMaster { + + static bool enabled; + + LockMasterImpl *impl; + + template + void AddLocks(const T *t) { + // by default, don't lock anything + } + + // 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); + } + AddParameters(args...); + } + + void ConstructorImpl(); + void DestructorImpl(); + void ObjectToLock(const void *); +public: + + // we lock on construction + template LockMaster(bool emptyGuard, const Types*... types) { + if(!enabled) { + impl = NULL; + return; + } + + ConstructorImpl(); + AddParameters(types...); + } + + // and unlock on destruction + ~LockMaster() { + if(!impl) { + return; + } + DestructorImpl(); + } + + // TemporaryUnlock unlocks the LockMaster currently registered on the thread, + // and re-locks it on destruction. + class TemporaryUnlock { + LockMasterImpl *impl; + + void ConstructorImpl(); + void DestructorImpl(); + public: + TemporaryUnlock() { + // 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(!impl) { + return; + } + DestructorImpl(); + } + }; + + static void Initialize(); + + // Enables the thread safety system + static void Enable() { + enabled = true; + } + + static void Disable() { + enabled = false; + } +}; + + +template<> inline void LockMaster::AddLocks(const git_repository *repo) { + // when using a repo, lock the repo + ObjectToLock(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; + } + 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); + ObjectToLock(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 diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc new file mode 100644 index 000000000..49fb9b4db --- /dev/null +++ b/generate/templates/manual/src/lock_master.cc @@ -0,0 +1,231 @@ +#include +#include +#include +#include +#include +#include + +#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; + // 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); +}; + +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 (auto it = mutexes.begin(); it != mutexes.end(); ) + { + 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; + it++; + mutexes.erase(to_erase); + } else { + it++; + } + } + + uv_mutex_unlock(&mapMutex); +} + +void LockMaster::Initialize() { + LockMasterImpl::Initialize(); +} + +std::vector LockMasterImpl::GetMutexes(int useCountDelta) { + std::vector objectMutexes; + + uv_mutex_lock(&mapMutex); + + for (auto object : objectsToLock) { + if(object) { + // ensure we have an initialized mutex for each object + auto mutexIt = mutexes.find(object); + if(mutexIt == mutexes.end()) { + mutexIt = mutexes.insert( + std::make_pair( + object, + ObjectInfo((uv_mutex_t *)malloc(sizeof(uv_mutex_t)), 0U) + ) + ).first; + uv_mutex_init(mutexIt->second.mutex); + } + + objectMutexes.push_back(mutexIt->second.mutex); + mutexIt->second.useCount += useCountDelta; + } + } + + uv_mutex_unlock(&mapMutex); + + return objectMutexes; +} + +void LockMasterImpl::Register() { + uv_key_set(¤tLockMasterKey, this); +} + +void LockMasterImpl::Unregister() { + uv_key_set(¤tLockMasterKey, NULL); +} + +void LockMasterImpl::Lock(bool acquireMutexes) { + std::vector objectMutexes = GetMutexes(acquireMutexes * 1); + + 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 + // 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 = 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 == 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 + 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 + alreadyLocked = it; + break; + } + } + } while(it != objectMutexes.end()); +} + +void LockMasterImpl::Unlock(bool releaseMutexes) { + std::vector objectMutexes = GetMutexes(releaseMutexes * -1); + + std::for_each(objectMutexes.begin(), objectMutexes.end(), uv_mutex_unlock); +} + +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(&cleanupMutexesHandle); +} + + +// LockMaster + +void LockMaster::ConstructorImpl() { + impl = new LockMasterImpl(); +} + +void LockMaster::DestructorImpl() { + delete impl; +} + +void LockMaster::ObjectToLock(const void *objectToLock) { + impl->ObjectToLock(objectToLock); +} + + +// LockMaster::TemporaryUnlock + +void LockMaster::TemporaryUnlock::ConstructorImpl() { + impl = LockMasterImpl::CurrentLockMasterImpl(); + if(impl) { + impl->Unlock(false); + } +} + +void LockMaster::TemporaryUnlock::DestructorImpl() { + impl->Lock(false); +} + +bool LockMaster::enabled = false; 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/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/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/binding.gyp b/generate/templates/templates/binding.gyp index 673d80446..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", @@ -58,7 +59,8 @@ "WARNING_CFLAGS": [ "-Wno-unused-variable", "-Wint-conversions", - "-Wmissing-field-initializers" + "-Wmissing-field-initializers", + "-Wno-c++11-extensions" ] } } @@ -71,6 +73,18 @@ "_HAS_EXCEPTIONS=1" ] } + ], [ + "OS=='linux' and ' #include #include +#include +#include +#include +#include "../include/lock_master.h" #include "../include/wrapper.h" #include "../include/functions/copy.h" {% each %} @@ -11,6 +15,14 @@ {% endif %} {% endeach %} +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(); @@ -23,6 +35,11 @@ extern "C" void init(Local target) { {{ cppClassName }}::InitializeComponent(target); {% endif %} {% endeach %} + + NODE_SET_METHOD(target, "enableThreadSafety", LockMasterEnable); + NODE_SET_METHOD(target, "disableThreadSafety", LockMasterDisable); + + LockMaster::Initialize(); } NODE_MODULE(nodegit, init) diff --git a/generate/templates/templates/struct_content.cc b/generate/templates/templates/struct_content.cc index 3520fa1a7..e2e399d98 100644 --- a/generate/templates/templates/struct_content.cc +++ b/generate/templates/templates/struct_content.cc @@ -15,6 +15,7 @@ extern "C" { } #include +#include "../include/lock_master.h" #include "../include/functions/copy.h" #include "../include/{{ filename }}.h" #include "../include/functions/sleep_for_ms.h" diff --git a/test/tests/index.js b/test/tests/index.js index a221c0b01..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() { @@ -47,6 +51,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 +60,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); });