moar lock

3 files changed, 85 insertions, 16 deletions

diff --git a/include/git2/cancellable.h b/include/git2/cancellable.h
index d7079596b..555fa66bc 100644
--- a/include/git2/cancellable.h
+++ b/include/git2/cancellable.h
@@ -13,13 +13,13 @@
 typedef struct git_cancellable git_cancellable;
 typedef struct git_cancellable_source git_cancellable_source;
 
-typedef void (*git_cancellable_cb)(git_cancellable *cancellable, void *payload);
+typedef int (*git_cancellable_cb)(git_cancellable *cancellable, void *payload);
 
 GIT_EXTERN(int) git_cancellable_source_new(git_cancellable_source **out);
-GIT_EXTERN(int) git_cancellable_source_free(git_cancellable_source *cs);
+GIT_EXTERN(void) git_cancellable_source_free(git_cancellable_source *cs);
 GIT_EXTERN(int) git_cancellable_is_cancelled(git_cancellable *c);
 GIT_EXTERN(int) git_cancellable_register(git_cancellable *c, git_cancellable_cb cb, void *payload);
 GIT_EXTERN(git_cancellable *) git_cancellable_source_token(git_cancellable_source *cs);
-GIT_EXTERN(void) git_cancellable_source_cancel(git_cancellable_source *cs);
+GIT_EXTERN(int) git_cancellable_source_cancel(git_cancellable_source *cs);
 
 #endif
diff --git a/src/cancellable.c b/src/cancellable.c
index 8daaee61b..c3da91ab1 100644
--- a/src/cancellable.c
+++ b/src/cancellable.c
@@ -17,7 +17,8 @@ typedef struct {
 
 struct  git_cancellable {
 	git_array_t(registration) registrations;
-	git_atomic cancelled;
+	git_mutex lock;
+	bool cancelled;
 };
 
 struct git_cancellable_source {
@@ -32,11 +33,17 @@ int git_cancellable_source_new(git_cancellable_source **out)
 	cs = git__calloc(1, sizeof(git_cancellable_source));
 	GITERR_CHECK_ALLOC(cs);
 
+	if (git_mutex_init(&cs->token.lock) < 0) {
+		git__free(cs);
+		giterr_set(GITERR_OS, "failed to initialize cancellable lock");
+		return -1;
+	}
+
 	*out= cs;
 	return 0;
 }
 
-int git_cancellable_source_free(git_cancellable_source *cs)
+void git_cancellable_source_free(git_cancellable_source *cs)
 {
 	git_array_clear(cs->token.registrations);
 	git__free(cs);
@@ -44,7 +51,8 @@ int git_cancellable_source_free(git_cancellable_source *cs)
 
 int git_cancellable_is_cancelled(git_cancellable *c)
 {
-	return c->cancelled.val;
+	/* Should we have a memory barrier here? a full lock? */
+	return c->cancelled;
 }
 
 int git_cancellable_register(git_cancellable *c, git_cancellable_cb cb, void *payload)
@@ -63,16 +71,36 @@ git_cancellable *git_cancellable_source_token(git_cancellable_source *cs)
 	return &cs->token;
 }
 
-void git_cancellable_source_cancel(git_cancellable_source *cs)
+int git_cancellable_source_cancel(git_cancellable_source *cs)
 {
 	size_t i;
+	int error = 0;
 	registration *reg;
 
-	git_atomic_set(&cs->token.cancelled, 1);
+	/* You can only cancel once, short-circuit when already done */
+	if (cs->token.cancelled)
+		return 0;
+
+	if (git_mutex_lock(&cs->token.lock) < 0) {
+		giterr_set(GITERR_OS, "failed to lock cancellable token");
+		return -1;
+	}
 
+	/* We lost the race for the mutex */
+	if (cs->token.cancelled)
+		goto unlock;
+
+	cs->token.cancelled = true;
+
+	/* Run the registered hooks if it's the first cancellation */
 	for (i = 0; i < git_array_size(cs->token.registrations); i++) {
 		reg = git_array_get(cs->token.registrations, i);
 		assert(reg);
-		reg->cb(&cs->token, reg->payload);
+		if ((error = reg->cb(&cs->token, reg->payload)) < 0)
+			goto unlock;
 	}
+
+ unlock:
+	git_mutex_unlock(&cs->token.lock);
+	return error;
 }
diff --git a/tests/core/cancellable.c b/tests/core/cancellable.c
index 38848bb2b..319c9c260 100644
--- a/tests/core/cancellable.c
+++ b/tests/core/cancellable.c
@@ -10,20 +10,22 @@ void test_core_cancellable__can_cancel(void)
 	token = git_cancellable_source_token(cs);
 
 	cl_assert(!git_cancellable_is_cancelled(token));
-	git_cancellable_source_cancel(cs);
+	cl_git_pass(git_cancellable_source_cancel(cs));
 	cl_assert(git_cancellable_is_cancelled(token));
 
 	git_cancellable_source_free(cs);
 }
 
-static void cancel_second(git_cancellable *cancellable, void *payload)
+static int cancel_second(git_cancellable *cancellable, void *payload)
 {
 	git_cancellable_source *cs;
 
 	GIT_UNUSED(cancellable);
 
 	cs = (git_cancellable_source *) payload;
-	git_cancellable_source_cancel(cs);
+	cl_git_pass(git_cancellable_source_cancel(cs));
+
+	return 0;
 }
 
 void test_core_cancellable__can_register(void)
@@ -42,7 +44,7 @@ void test_core_cancellable__can_register(void)
 	cl_assert(!git_cancellable_is_cancelled(token1));
 	cl_assert(!git_cancellable_is_cancelled(token2));
 
-	git_cancellable_source_cancel(cs1);
+	cl_git_pass(git_cancellable_source_cancel(cs1));
 
 	cl_assert(git_cancellable_is_cancelled(token1));
 	cl_assert(git_cancellable_is_cancelled(token2));
@@ -51,13 +53,15 @@ void test_core_cancellable__can_register(void)
 	git_cancellable_source_free(cs2);
 }
 
-static void cancel_count(git_cancellable *cancellable, void *payload)
+static int cancel_count(git_cancellable *cancellable, void *payload)
 {
 	int *i;
 	GIT_UNUSED(cancellable);
 
 	i = (int *) payload;
 	*i += 1;
+
+	return 0;
 }
 
 void test_core_cancellable__registration_fires_once(void)
@@ -73,10 +77,47 @@ void test_core_cancellable__registration_fires_once(void)
 
 	cl_assert(!git_cancellable_is_cancelled(token));
 
-	git_cancellable_source_cancel(cs);
+	cl_git_pass(git_cancellable_source_cancel(cs));
+	cl_assert(git_cancellable_is_cancelled(token));
+
+	cl_git_pass(git_cancellable_source_cancel(cs));
+	cl_assert(git_cancellable_is_cancelled(token));
+
+	cl_assert_equal_i(1, cancelled_times);
+
+	git_cancellable_source_free(cs);
+}
+
+static int cancel_fail(git_cancellable *cancellable, void *payload)
+{
+	int *i;
+	GIT_UNUSED(cancellable);
+
+	i = (int *) payload;
+	*i += 1;
+
+	return GIT_EUSER;
+}
+
+void test_core_cancellable__trigger_failure(void)
+{
+	git_cancellable_source *cs;
+	git_cancellable *token;
+	int cancelled_times = 0;
+
+	cl_git_pass(git_cancellable_source_new(&cs));
+
+	token = git_cancellable_source_token(cs);
+	/* Register twice, but we fail on the first time, so we should only inc once */
+	cl_git_pass(git_cancellable_register(token, cancel_fail, &cancelled_times));
+	cl_git_pass(git_cancellable_register(token, cancel_fail, &cancelled_times));
+
+	cl_assert(!git_cancellable_is_cancelled(token));
+
+	cl_git_fail_with(GIT_EUSER, git_cancellable_source_cancel(cs));
 	cl_assert(git_cancellable_is_cancelled(token));
 
-	git_cancellable_source_cancel(cs);
+	cl_git_pass(git_cancellable_source_cancel(cs));
 	cl_assert(git_cancellable_is_cancelled(token));
 
 	cl_assert_equal_i(1, cancelled_times);