Skip to content

Commit fbd892b

Browse files
srajkosrajko
committed
Add support for thread safety on async actions only
1 parent 39dfc74 commit fbd892b

7 files changed

Lines changed: 71 additions & 45 deletions

File tree

‎generate/templates/manual/include/lock_master.h‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,15 @@
44
class LockMasterImpl;
55

66
class LockMaster {
7+
public:
8+
enum Status {
9+
Disabled = 0,
10+
EnabledForAsyncOnly,
11+
Enabled
12+
};
713

8-
static bool enabled;
14+
private:
15+
static Status status;
916

1017
LockMasterImpl *impl;
1118

@@ -34,8 +41,8 @@ class LockMaster {
3441
public:
3542

3643
// we lock on construction
37-
template<typename ...Types> LockMaster(bool emptyGuard, const Types*... types) {
38-
if(!enabled) {
44+
template<typename ...Types> LockMaster(bool asyncAction, const Types*... types) {
45+
if((status == Disabled) || ((status == EnabledForAsyncOnly) && !asyncAction)) {
3946
impl = NULL;
4047
return;
4148
}
@@ -62,7 +69,7 @@ class LockMaster {
6269
void DestructorImpl();
6370
public:
6471
TemporaryUnlock() {
65-
// We can't return here if enabled is false
72+
// We can't return here if disabled
6673
// It's possible that a LockMaster was fully constructed and registered
6774
// before the thread safety was disabled.
6875
// So we rely on ConstructorImpl to abort if there is no registered LockMaster
@@ -80,15 +87,19 @@ class LockMaster {
8087

8188
// Enables the thread safety system
8289
static void Enable() {
83-
enabled = true;
90+
status = Enabled;
91+
}
92+
93+
static void SetStatus(Status status) {
94+
LockMaster::status = status;
8495
}
8596

8697
static void Disable() {
87-
enabled = false;
98+
status = Disabled;
8899
}
89100

90-
static bool IsEnabled() {
91-
return enabled;
101+
static Status GetStatus() {
102+
return status;
92103
}
93104

94105
// Diagnostic information that can be provided to the JavaScript layer

‎generate/templates/manual/src/lock_master.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ NAN_GC_CALLBACK(LockMasterImpl::CleanupMutexes) {
8787
// this means that turning thread safety on and then off
8888
// could result in remaining mutexes - but they would get cleaned up
8989
// if thread safety is turned on again
90-
if (!LockMaster::IsEnabled()) {
90+
if (LockMaster::GetStatus() == LockMaster::Disabled) {
9191
return;
9292
}
9393

@@ -243,4 +243,4 @@ void LockMaster::TemporaryUnlock::DestructorImpl() {
243243
impl->Lock(false);
244244
}
245245

246-
bool LockMaster::enabled = false;
246+
LockMaster::Status LockMaster::status = LockMaster::Disabled;

‎generate/templates/partials/async_function.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) {
8080

8181
void {{ cppClassName }}::{{ cppFunctionName }}Worker::Execute() {
8282
giterr_clear();
83-
83+
8484
{
85-
LockMaster lockMaster(true{%each args|argsInfo as arg %}
85+
LockMaster lockMaster(/*asyncAction: */true{%each args|argsInfo as arg %}
8686
{%if arg.cType|isPointer%}{%if not arg.cType|isDoublePointer%}
8787
,baton->{{ arg.name }}
8888
{%endif%}{%endif%}

‎generate/templates/partials/sync_function.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@ if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL
3535
{% endif %}
3636

3737
giterr_clear();
38-
38+
3939
{
40-
LockMaster lockMaster(true{%each args|argsInfo as arg %}
40+
LockMaster lockMaster(/*asyncAction: */false{%each args|argsInfo as arg %}
4141
{%if arg.cType|isPointer%}{%if not arg.isReturn%}
4242
,{%if arg.isSelf %}
4343
Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue()

‎generate/templates/templates/nodegit.cc‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,25 @@ void LockMasterEnable(const FunctionCallbackInfo<Value>& info) {
2525
LockMaster::Enable();
2626
}
2727

28-
void LockMasterDisable(const FunctionCallbackInfo<Value>& info) {
29-
LockMaster::Disable();
28+
void LockMasterSetStatus(const FunctionCallbackInfo<Value>& info) {
29+
Nan::HandleScope scope;
30+
31+
// convert the first argument to Status
32+
if(info.Length() >= 0 && info[0]->IsNumber()) {
33+
v8::Local<v8::Int32> value = info[0]->ToInt32();
34+
LockMaster::Status status = static_cast<LockMaster::Status>(value->Value());
35+
if(status >= LockMaster::Status::Disabled && status <= LockMaster::Status::Enabled) {
36+
LockMaster::SetStatus(status);
37+
return;
38+
}
39+
}
40+
41+
// argument error
42+
Nan::ThrowError("Argument must be one 0, 1 or 2");
3043
}
3144

32-
void LockMasterIsEnabled(const FunctionCallbackInfo<Value>& info) {
33-
info.GetReturnValue().Set(Nan::New(LockMaster::IsEnabled()));
45+
void LockMasterGetStatus(const FunctionCallbackInfo<Value>& info) {
46+
info.GetReturnValue().Set(Nan::New(LockMaster::GetStatus()));
3447
}
3548

3649
void LockMasterGetDiagnostics(const FunctionCallbackInfo<Value>& info) {
@@ -88,8 +101,8 @@ extern "C" void init(Local<v8::Object> target) {
88101
ConvenientPatch::InitializeComponent(target);
89102

90103
NODE_SET_METHOD(target, "enableThreadSafety", LockMasterEnable);
91-
NODE_SET_METHOD(target, "disableThreadSafety", LockMasterDisable);
92-
NODE_SET_METHOD(target, "isThreadSafetyEnabled", LockMasterIsEnabled);
104+
NODE_SET_METHOD(target, "setThreadSafetyStatus", LockMasterSetStatus);
105+
NODE_SET_METHOD(target, "getThreadSafetyStatus", LockMasterGetStatus);
93106
NODE_SET_METHOD(target, "getThreadSafetyDiagnostics", LockMasterGetDiagnostics);
94107

95108
LockMaster::Initialize();

‎test/runner.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@ var local = path.join.bind(path, __dirname);
66
var NodeGit = require('..');
77

88
if(process.env.NODEGIT_TEST_THREADSAFETY) {
9+
console.log('Enabling thread safety in NodeGit');
910
NodeGit.enableThreadSafety();
11+
} else if (process.env.NODEGIT_TEST_THREADSAFETY_ASYNC) {
12+
console.log('Enabling thread safety for async actions only in NodeGit');
13+
NodeGit.setThreadSafetyStatus(1);
1014
}
1115

1216
// Have to wrap exec, since it has a weird callback signature.

‎test/tests/thread_safety.js‎

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -22,43 +22,41 @@ describe("ThreadSafety", function() {
2222
});
2323

2424
it("can enable and disable thread safety", function() {
25-
var originalValue = NodeGit.isThreadSafetyEnabled();
25+
var originalValue = NodeGit.getThreadSafetyStatus();
2626

2727
NodeGit.enableThreadSafety();
28-
assert.equal(true, NodeGit.isThreadSafetyEnabled());
28+
assert.equal(2, NodeGit.getThreadSafetyStatus());
2929

30-
NodeGit.disableThreadSafety();
31-
assert.equal(false, NodeGit.isThreadSafetyEnabled());
30+
NodeGit.setThreadSafetyStatus(1);
31+
assert.equal(1, NodeGit.getThreadSafetyStatus());
3232

33-
// flip the switch again, to make sure we test all transitions
34-
// (we could have started with thread safety enabled)
35-
NodeGit.enableThreadSafety();
36-
assert.equal(true, NodeGit.isThreadSafetyEnabled());
33+
NodeGit.setThreadSafetyStatus(0);
34+
assert.equal(0, NodeGit.getThreadSafetyStatus());
3735

38-
if (originalValue) {
39-
NodeGit.enableThreadSafety();
40-
} else {
41-
NodeGit.disableThreadSafety();
42-
}
36+
NodeGit.setThreadSafetyStatus(originalValue);
4337
});
4438

4539
it("can lock something and cleanup mutex", function() {
40+
var diagnostics = NodeGit.getThreadSafetyDiagnostics();
41+
var originalCount = diagnostics.storedMutexesCount;
4642
// call a sync method to guarantee that it stores a mutex,
4743
// and that it will clean up the mutex in a garbage collection cycle
4844
this.repository.headDetached();
4945

50-
var diagnostics = NodeGit.getThreadSafetyDiagnostics();
51-
if (NodeGit.isThreadSafetyEnabled()) {
52-
// this is a fairly vague test - it just tests that something
53-
// had a mutex created for it at some point (i.e., the thread safety
54-
// code is not completely dead)
55-
assert.ok(diagnostics.storedMutexesCount > 0);
56-
// now test that GC cleans up mutexes
57-
global.gc();
58-
diagnostics = NodeGit.getThreadSafetyDiagnostics();
59-
assert.equal(0, diagnostics.storedMutexesCount);
60-
} else {
61-
assert.equal(0, diagnostics.storedMutexesCount);
46+
diagnostics = NodeGit.getThreadSafetyDiagnostics();
47+
switch(NodeGit.getThreadSafetyStatus()) {
48+
case 2:
49+
// this is a fairly vague test - it just tests that something
50+
// had a mutex created for it at some point (i.e., the thread safety
51+
// code is not completely dead)
52+
assert.ok(diagnostics.storedMutexesCount > 0);
53+
break;
54+
case 1:
55+
assert.equal(originalCount, diagnostics.storedMutexesCount);
56+
break;
57+
58+
case 0:
59+
assert.equal(0, diagnostics.storedMutexesCount);
6260
}
6361
});
6462
});

0 commit comments

Comments
 (0)