From e6e8530aa6c8773dd523fd6fe07629b9481a8fee Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 14 Apr 2014 12:31:17 -0700 Subject: [PATCH] Lock attribute file while reparsing data I don't love this approach, but achieving thread-safety for attribute and ignore data while reloading files would require a larger rewrite in order to avoid this. If an attribute or ignore file is out of date, this holds a lock on the file while we are reloading the data so that another thread won't try to reload the data at the same time. --- src/attr_file.c | 31 +++++++++++++++++++++++++++++-- src/attr_file.h | 4 +++- src/ignore.c | 31 +++++++++++++++++-------------- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/attr_file.c b/src/attr_file.c index 66f71ad19..574de25bb 100644 --- a/src/attr_file.c +++ b/src/attr_file.c @@ -9,8 +9,13 @@ static void attr_file_free(git_attr_file *file) { - git_attr_file__clear_rules(file); + bool unlock = !git_mutex_lock(&file->lock); + git_attr_file__clear_rules(file, false); git_pool_clear(&file->pool); + if (unlock) + git_mutex_unlock(&file->lock); + git_mutex_free(&file->lock); + git__memzero(file, sizeof(*file)); git__free(file); } @@ -23,6 +28,12 @@ int git_attr_file__new( git_attr_file *attrs = git__calloc(1, sizeof(git_attr_file)); GITERR_CHECK_ALLOC(attrs); + if (git_mutex_init(&attrs->lock) < 0) { + giterr_set(GITERR_OS, "Failed to initialize lock"); + git__free(attrs); + return -1; + } + if (git_pool_init(&attrs->pool, 1, 0) < 0) { attr_file_free(attrs); return -1; @@ -35,14 +46,24 @@ int git_attr_file__new( return 0; } -void git_attr_file__clear_rules(git_attr_file *file) +int git_attr_file__clear_rules(git_attr_file *file, bool need_lock) { unsigned int i; git_attr_rule *rule; + if (need_lock && git_mutex_lock(&file->lock) < 0) { + giterr_set(GITERR_OS, "Failed to lock attribute file"); + return -1; + } + git_vector_foreach(&file->rules, i, rule) git_attr_rule__free(rule); git_vector_free(&file->rules); + + if (need_lock) + git_mutex_unlock(&file->lock); + + return 0; } void git_attr_file__free(git_attr_file *file) @@ -162,6 +183,11 @@ int git_attr_file__parse_buffer( !git__suffixcmp(attrs->ce->path, "/" GIT_ATTR_FILE)) context = attrs->ce->path; + if (git_mutex_lock(&attrs->lock) < 0) { + giterr_set(GITERR_OS, "Failed to lock attribute file"); + return -1; + } + while (!error && *scan) { /* allocate rule if needed */ if (!rule) { @@ -198,6 +224,7 @@ int git_attr_file__parse_buffer( } } + git_mutex_unlock(&attrs->lock); git_attr_rule__free(rule); return error; diff --git a/src/attr_file.h b/src/attr_file.h index f92ce3c96..9b4b8724b 100644 --- a/src/attr_file.h +++ b/src/attr_file.h @@ -66,6 +66,7 @@ typedef struct { struct git_attr_file { git_refcount rc; + git_mutex lock; git_attr_cache_entry *ce; git_attr_cache_source source; git_vector rules; /* vector of or */ @@ -115,7 +116,8 @@ int git_attr_file__parse_buffer( const char *data, void *payload); -void git_attr_file__clear_rules(git_attr_file *file); +int git_attr_file__clear_rules( + git_attr_file *file, bool need_lock); int git_attr_file__lookup_one( git_attr_file *file, diff --git a/src/ignore.c b/src/ignore.c index 1f9df8ab4..fbebd9ad3 100644 --- a/src/ignore.c +++ b/src/ignore.c @@ -32,6 +32,11 @@ static int parse_ignore_file( !git__suffixcmp(attrs->ce->path, "/" GIT_IGNORE_FILE)) context = attrs->ce->path; + if (git_mutex_lock(&attrs->lock) < 0) { + giterr_set(GITERR_OS, "Failed to lock attribute file"); + return -1; + } + while (!error && *scan) { if (!match) { match = git__calloc(1, sizeof(*match)); @@ -63,6 +68,7 @@ static int parse_ignore_file( } } + git_mutex_unlock(&attrs->lock); git__free(match); return error; @@ -247,12 +253,12 @@ void git_ignore__free(git_ignores *ignores) } static bool ignore_lookup_in_rules( - git_vector *rules, git_attr_path *path, int *ignored) + git_attr_file *file, git_attr_path *path, int *ignored) { size_t j; git_attr_fnmatch *match; - git_vector_rforeach(rules, j, match) { + git_vector_rforeach(&file->rules, j, match) { if (git_attr_fnmatch__match(match, path)) { *ignored = ((match->flags & GIT_ATTR_FNMATCH_NEGATIVE) == 0); return true; @@ -274,19 +280,18 @@ int git_ignore__lookup( return -1; /* first process builtins - success means path was found */ - if (ignore_lookup_in_rules( - &ignores->ign_internal->rules, &path, ignored)) + if (ignore_lookup_in_rules(ignores->ign_internal, &path, ignored)) goto cleanup; /* next process files in the path */ git_vector_foreach(&ignores->ign_path, i, file) { - if (ignore_lookup_in_rules(&file->rules, &path, ignored)) + if (ignore_lookup_in_rules(file, &path, ignored)) goto cleanup; } /* last process global ignores */ git_vector_foreach(&ignores->ign_global, i, file) { - if (ignore_lookup_in_rules(&file->rules, &path, ignored)) + if (ignore_lookup_in_rules(file, &path, ignored)) goto cleanup; } @@ -319,10 +324,9 @@ int git_ignore_clear_internal_rules( git_attr_file *ign_internal; if (!(error = get_internal_ignores(&ign_internal, repo))) { - git_attr_file__clear_rules(ign_internal); - - error = parse_ignore_file( - repo, ign_internal, GIT_IGNORE_DEFAULT_RULES, NULL); + if (!(error = git_attr_file__clear_rules(ign_internal, true))) + error = parse_ignore_file( + repo, ign_internal, GIT_IGNORE_DEFAULT_RULES, NULL); git_attr_file__free(ign_internal); } @@ -371,19 +375,18 @@ int git_ignore_path_is_ignored( break; /* first process builtins - success means path was found */ - if (ignore_lookup_in_rules( - &ignores.ign_internal->rules, &path, ignored)) + if (ignore_lookup_in_rules(ignores.ign_internal, &path, ignored)) goto cleanup; /* next process files in the path */ git_vector_foreach(&ignores.ign_path, i, file) { - if (ignore_lookup_in_rules(&file->rules, &path, ignored)) + if (ignore_lookup_in_rules(file, &path, ignored)) goto cleanup; } /* last process global ignores */ git_vector_foreach(&ignores.ign_global, i, file) { - if (ignore_lookup_in_rules(&file->rules, &path, ignored)) + if (ignore_lookup_in_rules(file, &path, ignored)) goto cleanup; }