From 045ec0e5f22fbf429d1cd6984486fac35a077ae4 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 24 Mar 2016 11:57:12 -0700 Subject: [PATCH] Add support for thread safety on async actions only --- .../templates/manual/include/lock_master.h | 27 +++++++--- generate/templates/manual/src/lock_master.cc | 4 +- generate/templates/partials/async_function.cc | 4 +- generate/templates/partials/sync_function.cc | 4 +- generate/templates/templates/nodegit.cc | 32 +++++++++--- test/runner.js | 4 ++ test/tests/index.js | 1 - test/tests/thread_safety.js | 51 ++++++++++--------- 8 files changed, 81 insertions(+), 46 deletions(-) diff --git a/generate/templates/manual/include/lock_master.h b/generate/templates/manual/include/lock_master.h index fd1a09b6f..94a622666 100644 --- a/generate/templates/manual/include/lock_master.h +++ b/generate/templates/manual/include/lock_master.h @@ -4,8 +4,15 @@ class LockMasterImpl; class LockMaster { +public: + enum Status { + Disabled = 0, + EnabledForAsyncOnly, + Enabled + }; - static bool enabled; +private: + static Status status; LockMasterImpl *impl; @@ -34,8 +41,8 @@ class LockMaster { public: // we lock on construction - template LockMaster(bool emptyGuard, const Types*... types) { - if(!enabled) { + template LockMaster(bool asyncAction, const Types*... types) { + if((status == Disabled) || ((status == EnabledForAsyncOnly) && !asyncAction)) { impl = NULL; return; } @@ -62,7 +69,7 @@ class LockMaster { void DestructorImpl(); public: TemporaryUnlock() { - // We can't return here if enabled is false + // We can't return here if disabled // 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 @@ -80,15 +87,19 @@ class LockMaster { // Enables the thread safety system static void Enable() { - enabled = true; + status = Enabled; + } + + static void SetStatus(Status status) { + LockMaster::status = status; } static void Disable() { - enabled = false; + status = Disabled; } - static bool IsEnabled() { - return enabled; + static Status GetStatus() { + return status; } // Diagnostic information that can be provided to the JavaScript layer diff --git a/generate/templates/manual/src/lock_master.cc b/generate/templates/manual/src/lock_master.cc index bc26b9900..30679b534 100644 --- a/generate/templates/manual/src/lock_master.cc +++ b/generate/templates/manual/src/lock_master.cc @@ -87,7 +87,7 @@ NAN_GC_CALLBACK(LockMasterImpl::CleanupMutexes) { // this means that turning thread safety on and then off // could result in remaining mutexes - but they would get cleaned up // if thread safety is turned on again - if (!LockMaster::IsEnabled()) { + if (LockMaster::GetStatus() == LockMaster::Disabled) { return; } @@ -243,4 +243,4 @@ void LockMaster::TemporaryUnlock::DestructorImpl() { impl->Lock(false); } -bool LockMaster::enabled = false; +LockMaster::Status LockMaster::status = LockMaster::Disabled; diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index 4861c7831..c38066b99 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -80,9 +80,9 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { void {{ cppClassName }}::{{ cppFunctionName }}Worker::Execute() { giterr_clear(); - + { - LockMaster lockMaster(true{%each args|argsInfo as arg %} + LockMaster lockMaster(/*asyncAction: */true{%each args|argsInfo as arg %} {%if arg.cType|isPointer%}{%if not arg.cType|isDoublePointer%} ,baton->{{ arg.name }} {%endif%}{%endif%} diff --git a/generate/templates/partials/sync_function.cc b/generate/templates/partials/sync_function.cc index ada0de62f..d53a5c2d0 100644 --- a/generate/templates/partials/sync_function.cc +++ b/generate/templates/partials/sync_function.cc @@ -35,9 +35,9 @@ if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL {% endif %} giterr_clear(); - + { - LockMaster lockMaster(true{%each args|argsInfo as arg %} + LockMaster lockMaster(/*asyncAction: */false{%each args|argsInfo as arg %} {%if arg.cType|isPointer%}{%if not arg.isReturn%} ,{%if arg.isSelf %} Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() diff --git a/generate/templates/templates/nodegit.cc b/generate/templates/templates/nodegit.cc index f165307b3..189a4d622 100644 --- a/generate/templates/templates/nodegit.cc +++ b/generate/templates/templates/nodegit.cc @@ -25,12 +25,25 @@ void LockMasterEnable(const FunctionCallbackInfo& info) { LockMaster::Enable(); } -void LockMasterDisable(const FunctionCallbackInfo& info) { - LockMaster::Disable(); +void LockMasterSetStatus(const FunctionCallbackInfo& info) { + Nan::HandleScope scope; + + // convert the first argument to Status + if(info.Length() >= 0 && info[0]->IsNumber()) { + v8::Local value = info[0]->ToInt32(); + LockMaster::Status status = static_cast(value->Value()); + if(status >= LockMaster::Disabled && status <= LockMaster::Enabled) { + LockMaster::SetStatus(status); + return; + } + } + + // argument error + Nan::ThrowError("Argument must be one 0, 1 or 2"); } -void LockMasterIsEnabled(const FunctionCallbackInfo& info) { - info.GetReturnValue().Set(Nan::New(LockMaster::IsEnabled())); +void LockMasterGetStatus(const FunctionCallbackInfo& info) { + info.GetReturnValue().Set(Nan::New(LockMaster::GetStatus())); } void LockMasterGetDiagnostics(const FunctionCallbackInfo& info) { @@ -88,10 +101,17 @@ extern "C" void init(Local target) { ConvenientPatch::InitializeComponent(target); NODE_SET_METHOD(target, "enableThreadSafety", LockMasterEnable); - NODE_SET_METHOD(target, "disableThreadSafety", LockMasterDisable); - NODE_SET_METHOD(target, "isThreadSafetyEnabled", LockMasterIsEnabled); + NODE_SET_METHOD(target, "setThreadSafetyStatus", LockMasterSetStatus); + NODE_SET_METHOD(target, "getThreadSafetyStatus", LockMasterGetStatus); NODE_SET_METHOD(target, "getThreadSafetyDiagnostics", LockMasterGetDiagnostics); + Local threadSafety = Nan::New(); + threadSafety->Set(Nan::New("DISABLED").ToLocalChecked(), Nan::New((int)LockMaster::Disabled)); + threadSafety->Set(Nan::New("ENABLED_FOR_ASYNC_ONLY").ToLocalChecked(), Nan::New((int)LockMaster::EnabledForAsyncOnly)); + threadSafety->Set(Nan::New("ENABLED").ToLocalChecked(), Nan::New((int)LockMaster::Enabled)); + + target->Set(Nan::New("THREAD_SAFETY").ToLocalChecked(), threadSafety); + LockMaster::Initialize(); } diff --git a/test/runner.js b/test/runner.js index 0b14a0c0f..ba50eb179 100644 --- a/test/runner.js +++ b/test/runner.js @@ -6,7 +6,11 @@ var local = path.join.bind(path, __dirname); var NodeGit = require('..'); if(process.env.NODEGIT_TEST_THREADSAFETY) { + console.log('Enabling thread safety in NodeGit'); NodeGit.enableThreadSafety(); +} else if (process.env.NODEGIT_TEST_THREADSAFETY_ASYNC) { + console.log('Enabling thread safety for async actions only in NodeGit'); + NodeGit.setThreadSafetyStatus(NodeGit.THREAD_SAFETY.ENABLED_FOR_ASYNC_ONLY); } // Have to wrap exec, since it has a weird callback signature. diff --git a/test/tests/index.js b/test/tests/index.js index ff0fb6d35..a8baa3a4d 100644 --- a/test/tests/index.js +++ b/test/tests/index.js @@ -31,7 +31,6 @@ describe("Index", function() { after(function() { this.index.clear(); - NodeGit.disableThreadSafety(); }); it("can get the index of a repo and examine entries", function() { diff --git a/test/tests/thread_safety.js b/test/tests/thread_safety.js index ee341a95a..c02952630 100644 --- a/test/tests/thread_safety.js +++ b/test/tests/thread_safety.js @@ -22,43 +22,44 @@ describe("ThreadSafety", function() { }); it("can enable and disable thread safety", function() { - var originalValue = NodeGit.isThreadSafetyEnabled(); + var originalValue = NodeGit.getThreadSafetyStatus(); NodeGit.enableThreadSafety(); - assert.equal(true, NodeGit.isThreadSafetyEnabled()); + assert.equal(NodeGit.THREAD_SAFETY.ENABLED, + NodeGit.getThreadSafetyStatus()); - NodeGit.disableThreadSafety(); - assert.equal(false, NodeGit.isThreadSafetyEnabled()); + NodeGit.setThreadSafetyStatus(NodeGit.THREAD_SAFETY.ENABLED_FOR_ASYNC_ONLY); + assert.equal(NodeGit.THREAD_SAFETY.ENABLED_FOR_ASYNC_ONLY, + NodeGit.getThreadSafetyStatus()); - // flip the switch again, to make sure we test all transitions - // (we could have started with thread safety enabled) - NodeGit.enableThreadSafety(); - assert.equal(true, NodeGit.isThreadSafetyEnabled()); + NodeGit.setThreadSafetyStatus(NodeGit.THREAD_SAFETY.DISABLED); + assert.equal(NodeGit.THREAD_SAFETY.DISABLED, + NodeGit.getThreadSafetyStatus()); - if (originalValue) { - NodeGit.enableThreadSafety(); - } else { - NodeGit.disableThreadSafety(); - } + NodeGit.setThreadSafetyStatus(originalValue); }); it("can lock something and cleanup mutex", function() { + var diagnostics = NodeGit.getThreadSafetyDiagnostics(); + var originalCount = diagnostics.storedMutexesCount; // call a sync method to guarantee that it stores a mutex, // and that it will clean up the mutex in a garbage collection cycle this.repository.headDetached(); - var diagnostics = NodeGit.getThreadSafetyDiagnostics(); - if (NodeGit.isThreadSafetyEnabled()) { - // this is a fairly vague test - it just tests that something - // had a mutex created for it at some point (i.e., the thread safety - // code is not completely dead) - assert.ok(diagnostics.storedMutexesCount > 0); - // now test that GC cleans up mutexes - global.gc(); - diagnostics = NodeGit.getThreadSafetyDiagnostics(); - assert.equal(0, diagnostics.storedMutexesCount); - } else { - assert.equal(0, diagnostics.storedMutexesCount); + diagnostics = NodeGit.getThreadSafetyDiagnostics(); + switch(NodeGit.getThreadSafetyStatus()) { + case NodeGit.THREAD_SAFETY.ENABLED: + // this is a fairly vague test - it just tests that something + // had a mutex created for it at some point (i.e., the thread safety + // code is not completely dead) + assert.ok(diagnostics.storedMutexesCount > 0); + break; + case NodeGit.THREAD_SAFETY.ENABLED_FOR_ASYNC_ONLY: + assert.equal(originalCount, diagnostics.storedMutexesCount); + break; + + case NodeGit.THREAD_SAFETY.DISABLED: + assert.equal(0, diagnostics.storedMutexesCount); } }); });