[RFC PATCH 2/3] Make ce_compare_gitlink thread-safe

From

ANAtneya Nair <atneya@google.com>

Date

Mar 5, 2024, 01:21 UTC

Message-ID

<20240305012112.1598053-4-atneya@google.com>

In-Reply-To

<20240305012112.1598053-2-atneya@google.com>

Signed-off-by: Atneya Nair <atneya@google.com>
---

Notes:
    Chasing down thread unsafe code was done using tsan.
    
    Open questions:
     - Is there any additional thread-unsafety that was missed?
     - Is it safe to strdup in do_config_from (do the filenames need static
       lifetime)? What is the performance implication (it seems small)?
     - Is the locking around ref_store_map appropriate and/or can it be made
       more granular?

 config.c | 3 ++-
 config.h | 2 +-
 refs.c   | 9 +++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 3cfeb3d8bd..d7f73d8745 100644
--- a/config.c
+++ b/config.c
@@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
 			    enum config_scope scope,
 			    struct key_value_info *out)
 {
-	out->filename = strintern(cs->name);
+	out->filename = strdup(cs->name);
 	out->origin_type = cs->origin_type;
 	out->linenr = cs->linenr;
 	out->scope = scope;
@@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
 
 	strbuf_release(&top->value);
 	strbuf_release(&top->var);
+	free(kvi.filename);
 
 	return ret;
 }
diff --git a/config.h b/config.h
index 5dba984f77..b78f1b6667 100644
--- a/config.h
+++ b/config.h
@@ -118,7 +118,7 @@ struct config_options {
 
 /* Config source metadata for a given config key-value pair */
 struct key_value_info {
-	const char *filename;
+	char *filename;
 	int linenr;
 	enum config_origin_type origin_type;
 	enum config_scope scope;
diff --git a/refs.c b/refs.c
index c633abf284..cce8a31b22 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	size_t len;
 	struct repository *subrepo;
 
+	// TODO is this locking tolerable, and/or can we get any finer
+	static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
+
 	if (!submodule)
 		return NULL;
 
@@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 		/* We need to strip off one or more trailing slashes */
 		submodule = to_free = xmemdupz(submodule, len);
 
+	pthread_mutex_lock(&lock);
 	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
+	pthread_mutex_unlock(&lock);
 	if (refs)
 		goto done;
 
@@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 		free(subrepo);
 		goto done;
 	}
+
+	pthread_mutex_lock(&lock);
+	// TODO maybe lock this separately
 	refs = ref_store_init(subrepo, submodule_sb.buf,
 			      REF_STORE_READ | REF_STORE_ODB);
 	register_ref_store_map(&submodule_ref_stores, "submodule",
 			       refs, submodule);
+	pthread_mutex_unlock(&lock);
 
 done:
 	strbuf_release(&submodule_sb);

-- 
2.44.0.rc1.240.g4c46232300-goog