Further threading fixes

This builds on the earlier thread safety work to make it so that
setting the odb, index, refdb, or config for a repository is done
in a threadsafe manner with minimized locking time.  This is done
by adding a lock to the repository object and using it to guard
the assignment of the above listed pointers.  The lock is only
held to assign the pointer value.

This also contains some minor fixes to the other work with pack
files to reduce the time that locks are being held to and fix an
apparently memory leak.

src/global.c

@@ -135,6 +135,12 @@ int git_threads_init(void)
 
 void git_threads_shutdown(void)
 {
+	if (_tls_init) {
+		void *ptr = pthread_getspecific(_tls_key);
+		pthread_setspecific(_tls_key, NULL);
+		git__free(ptr);
+	}
+
 	pthread_key_delete(_tls_key);
 	_tls_init = 0;
 	git_mutex_free(&git__mwindow_mutex);

src/pack.c

@@ -299,29 +299,27 @@ static int pack_index_open(struct git_pack_file *p)
 	int error = 0;
 	size_t name_len, base_len;
 
-	if ((error = git_mutex_lock(&p->lock)) < 0)
-		return error;
-
 	if (p->index_map.data)
-		goto done;
+		return 0;
 
 	name_len = strlen(p->pack_name);
 	assert(name_len > strlen(".pack")); /* checked by git_pack_file alloc */
 
-	if ((idx_name = git__malloc(name_len)) == NULL) {
-		error = -1;
-		goto done;
-	}
+	if ((idx_name = git__malloc(name_len)) == NULL)
+		return -1;
 
 	base_len = name_len - strlen(".pack");
 	memcpy(idx_name, p->pack_name, base_len);
 	memcpy(idx_name + base_len, ".idx", sizeof(".idx"));
 
+	if ((error = git_mutex_lock(&p->lock)) < 0)
+		return error;
+
+	if (!p->index_map.data)
 		error = pack_index_check(idx_name, p);
 
 	git__free(idx_name);
 
-done:
 	git_mutex_unlock(&p->lock);
 
 	return error;
@@ -820,7 +818,6 @@ void git_packfile_free(struct git_pack_file *p)
 	cache_free(&p->bases);
 
 	git_mwindow_free_all(&p->mwf);
-	git_mwindow_file_deregister(&p->mwf);
 
 	if (p->mwf.fd != -1)
 		p_close(p->mwf.fd);

src/pack.h

@@ -79,7 +79,7 @@ typedef struct {
 struct git_pack_file {
 	git_mwindow_file mwf;
 	git_map index_map;
-	git_mutex lock;
+	git_mutex lock; /* protect updates to mwf and index_map */
 
 	uint32_t num_objects;
 	uint32_t num_bad_objects;

src/refdb_fs.c

@@ -11,7 +11,6 @@
 #include "fileops.h"
 #include "pack.h"
 #include "reflog.h"
-#include "config.h"
 #include "refdb.h"
 #include "refdb_fs.h"
 

src/repository.c

@@ -32,41 +32,43 @@
 
 #define GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
 
+#define repo_swap_ptr(repo,item_ptr,new_value) \
+	git__swap(&repo->lock, (void **)item_ptr, new_value)
+
 static void drop_odb(git_repository *repo)
 {
-	if (repo->_odb != NULL) {
-		GIT_REFCOUNT_OWN(repo->_odb, NULL);
-		git_odb_free(repo->_odb);
-		repo->_odb = NULL;
+	git_odb *dropme = repo_swap_ptr(repo, &repo->_odb, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_odb_free(dropme);
 	}
 }
 
 static void drop_refdb(git_repository *repo)
 {
-	if (repo->_refdb != NULL) {
-		GIT_REFCOUNT_OWN(repo->_refdb, NULL);
-		git_refdb_free(repo->_refdb);
-		repo->_refdb = NULL;
+	git_refdb *dropme = repo_swap_ptr(repo, &repo->_refdb, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_refdb_free(dropme);
 	}
 }
 
 static void drop_config(git_repository *repo)
 {
-	if (repo->_config != NULL) {
-		GIT_REFCOUNT_OWN(repo->_config, NULL);
-		git_config_free(repo->_config);
-		repo->_config = NULL;
-	}
-
+	git_config *dropme = repo_swap_ptr(repo, &repo->_config, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_config_free(dropme);
 		git_repository__cvar_cache_clear(repo);
+	}
 }
 
 static void drop_index(git_repository *repo)
 {
-	if (repo->_index != NULL) {
-		GIT_REFCOUNT_OWN(repo->_index, NULL);
-		git_index_free(repo->_index);
-		repo->_index = NULL;
+	git_index *dropme = repo_swap_ptr(repo, &repo->_index, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_index_free(dropme);
 	}
 }
 
@@ -79,14 +81,15 @@ void git_repository_free(git_repository *repo)
 	git_attr_cache_flush(repo);
 	git_submodule_config_free(repo);
 
-	git__free(repo->path_repository);
-	git__free(repo->workdir);
-
 	drop_config(repo);
 	drop_index(repo);
 	drop_odb(repo);
 	drop_refdb(repo);
 
+	git__free(repo->path_repository);
+	git__free(repo->workdir);
+
+	git_mutex_free(&repo->lock);
 	git__free(repo);
 }
 
@@ -119,6 +122,8 @@ static git_repository *repository_alloc(void)
 
 	memset(repo, 0x0, sizeof(git_repository));
 
+	git_mutex_init(&repo->lock);
+
 	if (git_cache_init(&repo->objects) < 0) {
 		git__free(repo);
 		return NULL;
@@ -549,39 +554,47 @@ on_error:
 	return error;
 }
 
-int git_repository_config__weakptr(git_config **out, git_repository *repo)
+static const char *path_unless_empty(git_buf *buf)
 {
-	if (repo->_config == NULL) {
-		git_buf global_buf = GIT_BUF_INIT, xdg_buf = GIT_BUF_INIT, system_buf = GIT_BUF_INIT;
-		int res;
+	return git_buf_len(buf) > 0 ? git_buf_cstr(buf) : NULL;
+}
 
-		const char *global_config_path = NULL;
-		const char *xdg_config_path = NULL;
-		const char *system_config_path = NULL;
+int git_repository_config__weakptr(git_config **out, git_repository *repo)
+{
+	int error = 0;
 
-		if (git_config_find_global_r(&global_buf) == 0)
-			global_config_path = global_buf.ptr;
+	if (repo->_config == NULL) {
+		git_buf global_buf = GIT_BUF_INIT;
+		git_buf xdg_buf = GIT_BUF_INIT;
+		git_buf system_buf = GIT_BUF_INIT;
+		git_config *config;
 
-		if (git_config_find_xdg_r(&xdg_buf) == 0)
-			xdg_config_path = xdg_buf.ptr;
+		git_config_find_global_r(&global_buf);
+		git_config_find_xdg_r(&xdg_buf);
+		git_config_find_system_r(&system_buf);
 
-		if (git_config_find_system_r(&system_buf) == 0)
-			system_config_path = system_buf.ptr;
+		error = load_config(
+			&config, repo,
+			path_unless_empty(&global_buf),
+			path_unless_empty(&xdg_buf),
+			path_unless_empty(&system_buf));
+		if (!error) {
+			GIT_REFCOUNT_OWN(config, repo);
 
-		res = load_config(&repo->_config, repo, global_config_path, xdg_config_path, system_config_path);
+			config = repo_swap_ptr(repo, &repo->_config, config);
+			if (config != NULL) {
+				GIT_REFCOUNT_OWN(config, NULL);
+				git_config_free(config);
+			}
+		}
 
 		git_buf_free(&global_buf);
 		git_buf_free(&xdg_buf);
 		git_buf_free(&system_buf);
-
-		if (res < 0)
-			return -1;
-
-		GIT_REFCOUNT_OWN(repo->_config, repo);
 	}
 
 	*out = repo->_config;
-	return 0;
+	return error;
 }
 
 int git_repository_config(git_config **out, git_repository *repo)
@@ -597,35 +610,46 @@ void git_repository_set_config(git_repository *repo, git_config *config)
 {
 	assert(repo && config);
 
-	drop_config(repo);
+	GIT_REFCOUNT_OWN(config, repo);
+	GIT_REFCOUNT_INC(config);
 
-	repo->_config = config;
-	GIT_REFCOUNT_OWN(repo->_config, repo);
-	GIT_REFCOUNT_INC(repo->_config);
+	config = repo_swap_ptr(repo, &repo->_config, config);
+	if (config != NULL) {
+		GIT_REFCOUNT_OWN(config, NULL);
+		git_config_free(config);
+	}
+
+	git_repository__cvar_cache_clear(repo);
 }
 
 int git_repository_odb__weakptr(git_odb **out, git_repository *repo)
 {
+	int error = 0;
+
 	assert(repo && out);
 
 	if (repo->_odb == NULL) {
 		git_buf odb_path = GIT_BUF_INIT;
-		int res;
+		git_odb *odb;
 
-		if (git_buf_joinpath(&odb_path, repo->path_repository, GIT_OBJECTS_DIR) < 0)
-			return -1;
+		git_buf_joinpath(&odb_path, repo->path_repository, GIT_OBJECTS_DIR);
 
-		res = git_odb_open(&repo->_odb, odb_path.ptr);
-		git_buf_free(&odb_path); /* done with path */
+		error = git_odb_open(&odb, odb_path.ptr);
+		if (!error) {
+			GIT_REFCOUNT_OWN(odb, repo);
 
-		if (res < 0)
-			return -1;
+			odb = repo_swap_ptr(repo, &repo->_odb, odb);
+			if (odb != NULL) {
+				GIT_REFCOUNT_OWN(odb, NULL);
+				git_odb_free(odb);
+			}
+		}
 
-		GIT_REFCOUNT_OWN(repo->_odb, repo);
+		git_buf_free(&odb_path);
 	}
 
 	*out = repo->_odb;
-	return 0;
+	return error;
 }
 
 int git_repository_odb(git_odb **out, git_repository *repo)
@@ -641,30 +665,39 @@ void git_repository_set_odb(git_repository *repo, git_odb *odb)
 {
 	assert(repo && odb);
 
-	drop_odb(repo);
-
-	repo->_odb = odb;
-	GIT_REFCOUNT_OWN(repo->_odb, repo);
+	GIT_REFCOUNT_OWN(odb, repo);
 	GIT_REFCOUNT_INC(odb);
+
+	odb = repo_swap_ptr(repo, &repo->_odb, odb);
+	if (odb != NULL) {
+		GIT_REFCOUNT_OWN(odb, NULL);
+		git_odb_free(odb);
+	}
 }
 
 int git_repository_refdb__weakptr(git_refdb **out, git_repository *repo)
 {
+	int error = 0;
+
 	assert(out && repo);
 
 	if (repo->_refdb == NULL) {
-		int res;
-
-		res = git_refdb_open(&repo->_refdb, repo);
+		git_refdb *refdb;
 
-		if (res < 0)
-			return -1;
+		error = git_refdb_open(&refdb, repo);
+		if (!error) {
+			GIT_REFCOUNT_OWN(refdb, repo);
 
-		GIT_REFCOUNT_OWN(repo->_refdb, repo);
+			refdb = repo_swap_ptr(repo, &repo->_refdb, refdb);
+			if (refdb != NULL) {
+				GIT_REFCOUNT_OWN(refdb, NULL);
+				git_refdb_free(refdb);
+			}
+		}
 	}
 
 	*out = repo->_refdb;
-	return 0;
+	return error;
 }
 
 int git_repository_refdb(git_refdb **out, git_repository *repo)
@@ -678,40 +711,48 @@ int git_repository_refdb(git_refdb **out, git_repository *repo)
 
 void git_repository_set_refdb(git_repository *repo, git_refdb *refdb)
 {
-	assert (repo && refdb);
+	assert(repo && refdb);
 
-	drop_refdb(repo);
-
-	repo->_refdb = refdb;
-	GIT_REFCOUNT_OWN(repo->_refdb, repo);
+	GIT_REFCOUNT_OWN(refdb, repo);
 	GIT_REFCOUNT_INC(refdb);
+
+	refdb = repo_swap_ptr(repo, &repo->_refdb, refdb);
+	if (refdb != NULL) {
+		GIT_REFCOUNT_OWN(refdb, NULL);
+		git_refdb_free(refdb);
+	}
 }
 
 int git_repository_index__weakptr(git_index **out, git_repository *repo)
 {
+	int error = 0;
+
 	assert(out && repo);
 
 	if (repo->_index == NULL) {
-		int res;
 		git_buf index_path = GIT_BUF_INIT;
+		git_index *index;
 
-		if (git_buf_joinpath(&index_path, repo->path_repository, GIT_INDEX_FILE) < 0)
-			return -1;
+		git_buf_joinpath(&index_path, repo->path_repository, GIT_INDEX_FILE);
 
-		res = git_index_open(&repo->_index, index_path.ptr);
-		git_buf_free(&index_path); /* done with path */
+		error = git_index_open(&index, index_path.ptr);
+		if (!error) {
+			GIT_REFCOUNT_OWN(index, repo);
 
-		if (res < 0)
-			return -1;
+			index = repo_swap_ptr(repo, &repo->_index, index);
+			if (index != NULL) {
+				GIT_REFCOUNT_OWN(index, NULL);
+				git_index_free(index);
+			}
 
-		GIT_REFCOUNT_OWN(repo->_index, repo);
+			error = git_index_set_caps(repo->_index, GIT_INDEXCAP_FROM_OWNER);
+		}
 
-		if (git_index_set_caps(repo->_index, GIT_INDEXCAP_FROM_OWNER) < 0)
-			return -1;
+		git_buf_free(&index_path);
 	}
 
 	*out = repo->_index;
-	return 0;
+	return error;
 }
 
 int git_repository_index(git_index **out, git_repository *repo)
@@ -727,11 +768,14 @@ void git_repository_set_index(git_repository *repo, git_index *index)
 {
 	assert(repo && index);
 
-	drop_index(repo);
-
-	repo->_index = index;
-	GIT_REFCOUNT_OWN(repo->_index, repo);
+	GIT_REFCOUNT_OWN(index, repo);
 	GIT_REFCOUNT_INC(index);
+
+	index = repo_swap_ptr(repo, &repo->_index, index);
+	if (index != NULL) {
+		GIT_REFCOUNT_OWN(index, NULL);
+		git_index_free(index);
+	}
 }
 
 static int check_repositoryformatversion(git_config *config)

src/repository.h

@@ -83,6 +83,7 @@ struct git_repository {
 	git_refdb *_refdb;
 	git_config *_config;
 	git_index *_index;
+	git_mutex lock;
 
 	git_cache objects;
 	git_attr_cache attrcache;

src/util.h

@@ -313,4 +313,23 @@ int git__date_parse(git_time_t *out, const char *date);
  */
 extern size_t git__unescape(char *str);
 
+/*
+ * Swap a pointer with thread safety, returning old value.
+ */
+GIT_INLINE(void *) git__swap(git_mutex *lock, void **ptr_ptr, void *new_ptr)
+{
+	void *old_ptr;
+
+	if (*ptr_ptr == new_ptr)
+		return NULL;
+	if (git_mutex_lock(lock) < 0)
+		return new_ptr;
+
+	old_ptr = *ptr_ptr;
+	*ptr_ptr = new_ptr;
+
+	git_mutex_unlock(lock);
+	return old_ptr;
+}
+
 #endif /* INCLUDE_util_h__ */