From 7da4c429ea3d21e0fca0755e927f19b93e81a5c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 12:37:41 +0000 Subject: [PATCH 01/13] refdb: adjust the threading tests to what we promise We say it's going to work if you use a different repository in each thread. Let's do precisely that in our code instead of hoping re-using the refdb is going to work. This test does fail currently, surfacing existing bugs. --- tests/threads/refdb.c | 120 +++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 77 deletions(-) diff --git a/tests/threads/refdb.c b/tests/threads/refdb.c index f869bcb44..9b1b37592 100644 --- a/tests/threads/refdb.c +++ b/tests/threads/refdb.c @@ -19,13 +19,21 @@ void test_threads_refdb__cleanup(void) #define REPEAT 20 #define THREADS 20 +struct th_data { + int id; + const char *path; +}; + static void *iterate_refs(void *arg) { + struct th_data *data = (struct th_data *) arg; git_reference_iterator *i; git_reference *ref; int count = 0; + git_repository *repo; - cl_git_pass(git_reference_iterator_new(&i, g_repo)); + cl_git_pass(git_repository_open(&repo, data->path)); + cl_git_pass(git_reference_iterator_new(&i, repo)); for (count = 0; !git_reference_next(&ref, i); ++count) { cl_assert(ref != NULL); @@ -37,77 +45,31 @@ static void *iterate_refs(void *arg) git_reference_iterator_free(i); + git_repository_free(repo); giterr_clear(); return arg; } -void test_threads_refdb__iterator(void) -{ - int r, t; - git_thread th[THREADS]; - int id[THREADS]; - git_oid head; - git_reference *ref; - char name[128]; - git_refdb *refdb; - - g_repo = cl_git_sandbox_init("testrepo2"); - - cl_git_pass(git_reference_name_to_id(&head, g_repo, "HEAD")); - - /* make a bunch of references */ - - for (r = 0; r < 200; ++r) { - p_snprintf(name, sizeof(name), "refs/heads/direct-%03d", r); - cl_git_pass(git_reference_create(&ref, g_repo, name, &head, 0, NULL)); - git_reference_free(ref); - } - - cl_git_pass(git_repository_refdb(&refdb, g_repo)); - cl_git_pass(git_refdb_compress(refdb)); - git_refdb_free(refdb); - - g_expected = 206; - - for (r = 0; r < REPEAT; ++r) { - g_repo = cl_git_sandbox_reopen(); /* reopen to flush caches */ - - for (t = 0; t < THREADS; ++t) { - id[t] = t; -#ifdef GIT_THREADS - cl_git_pass(git_thread_create(&th[t], iterate_refs, &id[t])); -#else - th[t] = t; - iterate_refs(&id[t]); -#endif - } - -#ifdef GIT_THREADS - for (t = 0; t < THREADS; ++t) { - cl_git_pass(git_thread_join(&th[t], NULL)); - } -#endif - - memset(th, 0, sizeof(th)); - } -} - static void *create_refs(void *arg) { - int *id = arg, i; + int i; + struct th_data *data = (struct th_data *) arg; git_oid head; char name[128]; git_reference *ref[10]; + git_repository *repo; - cl_git_pass(git_reference_name_to_id(&head, g_repo, "HEAD")); + cl_git_pass(git_repository_open(&repo, data->path)); + + cl_git_pass(git_reference_name_to_id(&head, repo, "HEAD")); for (i = 0; i < 10; ++i) { - p_snprintf(name, sizeof(name), "refs/heads/thread-%03d-%02d", *id, i); - cl_git_pass(git_reference_create(&ref[i], g_repo, name, &head, 0, NULL)); + p_snprintf(name, sizeof(name), "refs/heads/thread-%03d-%02d", data->id, i); + cl_git_pass(git_reference_create(&ref[i], repo, name, &head, 0, NULL)); if (i == 5) { git_refdb *refdb; - cl_git_pass(git_repository_refdb(&refdb, g_repo)); + cl_git_pass(git_repository_refdb(&refdb, repo)); cl_git_pass(git_refdb_compress(refdb)); git_refdb_free(refdb); } @@ -116,33 +78,40 @@ static void *create_refs(void *arg) for (i = 0; i < 10; ++i) git_reference_free(ref[i]); + git_repository_free(repo); + giterr_clear(); return arg; } static void *delete_refs(void *arg) { - int *id = arg, i; + int i; + struct th_data *data = (struct th_data *) arg; git_reference *ref; char name[128]; + git_repository *repo; + + cl_git_pass(git_repository_open(&repo, data->path)); for (i = 0; i < 10; ++i) { p_snprintf( - name, sizeof(name), "refs/heads/thread-%03d-%02d", (*id) & ~0x3, i); + name, sizeof(name), "refs/heads/thread-%03d-%02d", (data->id) & ~0x3, i); - if (!git_reference_lookup(&ref, g_repo, name)) { + if (!git_reference_lookup(&ref, repo, name)) { cl_git_pass(git_reference_delete(ref)); git_reference_free(ref); } if (i == 5) { git_refdb *refdb; - cl_git_pass(git_repository_refdb(&refdb, g_repo)); + cl_git_pass(git_repository_refdb(&refdb, repo)); cl_git_pass(git_refdb_compress(refdb)); git_refdb_free(refdb); } } + git_repository_free(repo); giterr_clear(); return arg; } @@ -150,7 +119,7 @@ static void *delete_refs(void *arg) void test_threads_refdb__edit_while_iterate(void) { int r, t; - int id[THREADS]; + struct th_data th_data[THREADS]; git_oid head; git_reference *ref; char name[128]; @@ -189,29 +158,26 @@ void test_threads_refdb__edit_while_iterate(void) default: fn = iterate_refs; break; } - id[t] = t; + th_data[t].id = t; + th_data[t].path = git_repository_path(g_repo); - /* It appears with all reflog writing changes, etc., that this - * test has started to fail quite frequently, so let's disable it - * for now by just running on a single thread... - */ -/* #ifdef GIT_THREADS */ -/* cl_git_pass(git_thread_create(&th[t], fn, &id[t])); */ -/* #else */ - fn(&id[t]); -/* #endif */ +#ifdef GIT_THREADS + cl_git_pass(git_thread_create(&th[t], fn, &th_data[t])); +#else + fn(&th_data[t]); +#endif } #ifdef GIT_THREADS -/* for (t = 0; t < THREADS; ++t) { */ -/* cl_git_pass(git_thread_join(th[t], NULL)); */ -/* } */ + for (t = 0; t < THREADS; ++t) { + cl_git_pass(git_thread_join(&th[t], NULL)); + } memset(th, 0, sizeof(th)); for (t = 0; t < THREADS; ++t) { - id[t] = t; - cl_git_pass(git_thread_create(&th[t], iterate_refs, &id[t])); + th_data[t].id = t; + cl_git_pass(git_thread_create(&th[t], iterate_refs, &th_data[t])); } for (t = 0; t < THREADS; ++t) { From 9914efec2a0c32477b25897c98cbf78742eb2f94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 14:00:48 +0000 Subject: [PATCH 02/13] refdb: bubble up errors We can get useful information like GIT_ELOCKED out of this instead of just -1. --- src/refdb_fs.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index f978038e6..6faf6cca7 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -944,41 +944,42 @@ static int packed_write(refdb_fs_backend *backend) { git_sortedcache *refcache = backend->refcache; git_filebuf pack_file = GIT_FILEBUF_INIT; + int error; size_t i; /* lock the cache to updates while we do this */ - if (git_sortedcache_wlock(refcache) < 0) - return -1; + if ((error = git_sortedcache_wlock(refcache)) < 0) + return error; /* Open the file! */ - if (git_filebuf_open(&pack_file, git_sortedcache_path(refcache), 0, GIT_PACKEDREFS_FILE_MODE) < 0) + if ((error = git_filebuf_open(&pack_file, git_sortedcache_path(refcache), 0, GIT_PACKEDREFS_FILE_MODE)) < 0) goto fail; /* Packfiles have a header... apparently * This is in fact not required, but we might as well print it * just for kicks */ - if (git_filebuf_printf(&pack_file, "%s\n", GIT_PACKEDREFS_HEADER) < 0) + if ((error = git_filebuf_printf(&pack_file, "%s\n", GIT_PACKEDREFS_HEADER)) < 0) goto fail; for (i = 0; i < git_sortedcache_entrycount(refcache); ++i) { struct packref *ref = git_sortedcache_entry(refcache, i); assert(ref); - if (packed_find_peel(backend, ref) < 0) + if ((error = packed_find_peel(backend, ref)) < 0) goto fail; - if (packed_write_ref(ref, &pack_file) < 0) + if ((error = packed_write_ref(ref, &pack_file)) < 0) goto fail; } /* if we've written all the references properly, we can commit * the packfile to make the changes effective */ - if (git_filebuf_commit(&pack_file) < 0) + if ((error = git_filebuf_commit(&pack_file)) < 0) goto fail; /* when and only when the packfile has been properly written, * we can go ahead and remove the loose refs */ - if (packed_remove_loose(backend) < 0) + if ((error = packed_remove_loose(backend)) < 0) goto fail; git_sortedcache_updated(refcache); @@ -991,7 +992,7 @@ fail: git_filebuf_cleanup(&pack_file); git_sortedcache_wunlock(refcache); - return -1; + return error; } static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, const git_oid *old, const git_oid *new, const git_signature *author, const char *message); From 2d9aec99fb6a6a456aecbc354443c0c87e8a34e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 14:01:38 +0000 Subject: [PATCH 03/13] refdb: make ref deletion after pack safer In order not to undo concurrent modifications to references, we must make sure that we only delete a loose reference if it still has the same value as when we packed it. This means we need to lock it and then compare the value with the one we put in the packed file. --- src/refdb_fs.c | 51 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 6faf6cca7..2e92911ae 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -901,30 +901,68 @@ static int packed_write_ref(struct packref *ref, git_filebuf *file) static int packed_remove_loose(refdb_fs_backend *backend) { size_t i; - git_buf full_path = GIT_BUF_INIT; - int failed = 0; + git_buf ref_content = GIT_BUF_INIT; + int failed = 0, error = 0; /* backend->refcache is already locked when this is called */ for (i = 0; i < git_sortedcache_entrycount(backend->refcache); ++i) { struct packref *ref = git_sortedcache_entry(backend->refcache, i); + git_filebuf lock = GIT_FILEBUF_INIT; + git_oid current_id; if (!ref || !(ref->flags & PACKREF_WAS_LOOSE)) continue; - if (git_buf_joinpath(&full_path, backend->path, ref->name) < 0) - return -1; /* critical; do not try to recover on oom */ + /* We need to stop anybody from updating the ref while we try to do a safe delete */ + error = loose_lock(&lock, backend, ref->name); + /* If someone else is updating it, let them do it */ + if (error == GIT_EEXISTS) + continue; - if (git_path_exists(full_path.ptr) && p_unlink(full_path.ptr) < 0) { + if (error < 0) { + failed = 1; + continue; + } + + error = git_futils_readbuffer(&ref_content, lock.path_original); + /* Someone else beat us to cleaning up the ref, let's simply continue */ + if (error == GIT_ENOTFOUND) { + git_filebuf_cleanup(&lock); + continue; + } + + /* This became a symref between us packing and trying to delete it, so ignore it */ + if (!git__prefixcmp(ref_content.ptr, GIT_SYMREF)) { + git_filebuf_cleanup(&lock); + continue; + } + + /* Figure out the current id; if we fail record it but don't fail the whole operation */ + if ((error = loose_parse_oid(¤t_id, lock.path_original, &ref_content)) < 0) { + failed = 1; + git_filebuf_cleanup(&lock); + continue; + } + + /* If the ref moved since we packed it, we must not delete it */ + if (!git_oid_equal(¤t_id, &ref->oid)) { + git_filebuf_cleanup(&lock); + continue; + } + + if (p_unlink(lock.path_original) < 0) { if (failed) continue; giterr_set(GITERR_REFERENCE, "Failed to remove loose reference '%s' after packing: %s", - full_path.ptr, strerror(errno)); + lock.path_original, strerror(errno)); failed = 1; } + git_filebuf_cleanup(&lock); + /* * if we fail to remove a single file, this is *not* good, * but we should keep going and remove as many as possible. @@ -933,7 +971,6 @@ static int packed_remove_loose(refdb_fs_backend *backend) */ } - git_buf_free(&full_path); return failed ? -1 : 0; } From f94825c10c1c8f003e80859530cce8ceea1bd314 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 17:21:51 +0000 Subject: [PATCH 04/13] fileops: save errno and report file existence We need to save the errno, lest we clobber it in the giterr_set() call. Also add code for reporting that a path component is missing, which is a distinct failure mode. --- src/fileops.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/fileops.c b/src/fileops.c index fcc0301f9..a82202c98 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -72,8 +72,16 @@ int git_futils_creat_locked(const char *path, const mode_t mode) O_EXCL | O_BINARY | O_CLOEXEC, mode); if (fd < 0) { + int error = errno; giterr_set(GITERR_OS, "Failed to create locked file '%s'", path); - return errno == EEXIST ? GIT_ELOCKED : -1; + switch (error) { + case EEXIST: + return GIT_ELOCKED; + case ENOENT: + return GIT_ENOTFOUND; + default: + return -1; + } } return fd; From 7ea4710ae3529f6c7ddccf70f76bdd8a757b000e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 17:30:24 +0000 Subject: [PATCH 05/13] refdb: don't report failure for expected errors There might be a few threads or processes working with references concurrently, so fortify the code to ignore errors which come from concurrent access which do not stop us from continuing the work. This includes ignoring an unlinking error. Either someone else removed it or we leave the file around. In the former case the job is done, and in the latter case, the ref is still in a valid state. --- src/refdb_fs.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 2e92911ae..17d025edd 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -902,7 +902,7 @@ static int packed_remove_loose(refdb_fs_backend *backend) { size_t i; git_buf ref_content = GIT_BUF_INIT; - int failed = 0, error = 0; + int error = 0; /* backend->refcache is already locked when this is called */ @@ -917,12 +917,12 @@ static int packed_remove_loose(refdb_fs_backend *backend) /* We need to stop anybody from updating the ref while we try to do a safe delete */ error = loose_lock(&lock, backend, ref->name); /* If someone else is updating it, let them do it */ - if (error == GIT_EEXISTS) + if (error == GIT_EEXISTS || error == GIT_ENOTFOUND) continue; if (error < 0) { - failed = 1; - continue; + giterr_set(GITERR_REFERENCE, "failed to lock loose reference '%s'", ref->name); + return error; } error = git_futils_readbuffer(&ref_content, lock.path_original); @@ -940,7 +940,6 @@ static int packed_remove_loose(refdb_fs_backend *backend) /* Figure out the current id; if we fail record it but don't fail the whole operation */ if ((error = loose_parse_oid(¤t_id, lock.path_original, &ref_content)) < 0) { - failed = 1; git_filebuf_cleanup(&lock); continue; } @@ -951,27 +950,17 @@ static int packed_remove_loose(refdb_fs_backend *backend) continue; } - if (p_unlink(lock.path_original) < 0) { - if (failed) - continue; - - giterr_set(GITERR_REFERENCE, - "Failed to remove loose reference '%s' after packing: %s", - lock.path_original, strerror(errno)); - failed = 1; - } - - git_filebuf_cleanup(&lock); - /* * if we fail to remove a single file, this is *not* good, * but we should keep going and remove as many as possible. - * After we've removed as many files as possible, we return - * the error code anyway. + * If we fail to remove, the ref is still in the old state, so + * we haven't lost information. */ + p_unlink(lock.path_original); + git_filebuf_cleanup(&lock); } - return failed ? -1 : 0; + return 0; } /* From dd1ca6f15a85ba1e812be044bc12cdab9b3134dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 17:38:41 +0000 Subject: [PATCH 06/13] refdb: refactor the lockfile cleanup We can reduce the duplication by cleaning up at the beginning of the loop, since it's something we want to do every time we continue. --- src/refdb_fs.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 17d025edd..20cc08fc4 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -901,6 +901,7 @@ static int packed_write_ref(struct packref *ref, git_filebuf *file) static int packed_remove_loose(refdb_fs_backend *backend) { size_t i; + git_filebuf lock = GIT_FILEBUF_INIT; git_buf ref_content = GIT_BUF_INIT; int error = 0; @@ -908,12 +909,13 @@ static int packed_remove_loose(refdb_fs_backend *backend) for (i = 0; i < git_sortedcache_entrycount(backend->refcache); ++i) { struct packref *ref = git_sortedcache_entry(backend->refcache, i); - git_filebuf lock = GIT_FILEBUF_INIT; git_oid current_id; if (!ref || !(ref->flags & PACKREF_WAS_LOOSE)) continue; + git_filebuf_cleanup(&lock); + /* We need to stop anybody from updating the ref while we try to do a safe delete */ error = loose_lock(&lock, backend, ref->name); /* If someone else is updating it, let them do it */ @@ -927,28 +929,20 @@ static int packed_remove_loose(refdb_fs_backend *backend) error = git_futils_readbuffer(&ref_content, lock.path_original); /* Someone else beat us to cleaning up the ref, let's simply continue */ - if (error == GIT_ENOTFOUND) { - git_filebuf_cleanup(&lock); + if (error == GIT_ENOTFOUND) continue; - } /* This became a symref between us packing and trying to delete it, so ignore it */ - if (!git__prefixcmp(ref_content.ptr, GIT_SYMREF)) { - git_filebuf_cleanup(&lock); + if (!git__prefixcmp(ref_content.ptr, GIT_SYMREF)) continue; - } - /* Figure out the current id; if we fail record it but don't fail the whole operation */ - if ((error = loose_parse_oid(¤t_id, lock.path_original, &ref_content)) < 0) { - git_filebuf_cleanup(&lock); + /* Figure out the current id; if we find a bad ref file, skip it so we can do the rest */ + if (loose_parse_oid(¤t_id, lock.path_original, &ref_content) < 0) continue; - } /* If the ref moved since we packed it, we must not delete it */ - if (!git_oid_equal(¤t_id, &ref->oid)) { - git_filebuf_cleanup(&lock); + if (!git_oid_equal(¤t_id, &ref->oid)) continue; - } /* * if we fail to remove a single file, this is *not* good, @@ -957,9 +951,9 @@ static int packed_remove_loose(refdb_fs_backend *backend) * we haven't lost information. */ p_unlink(lock.path_original); - git_filebuf_cleanup(&lock); } + git_filebuf_cleanup(&lock); return 0; } From 2e09106e7a8711e3a4a70ef304643d28f2763c11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 17:49:49 +0000 Subject: [PATCH 07/13] refdb: bubble up the error code when compressing the db This allows the caller to know the errors was e.g. due to the packed-refs file being already locked and they can try again later. --- src/refdb_fs.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 20cc08fc4..6b55960e1 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1383,14 +1383,15 @@ static int refdb_fs_backend__rename( static int refdb_fs_backend__compress(git_refdb_backend *_backend) { + int error; refdb_fs_backend *backend = (refdb_fs_backend *)_backend; assert(backend); - if (packed_reload(backend) < 0 || /* load the existing packfile */ - packed_loadloose(backend) < 0 || /* add all the loose refs */ - packed_write(backend) < 0) /* write back to disk */ - return -1; + if ((error = packed_reload(backend)) < 0 || /* load the existing packfile */ + (error = packed_loadloose(backend)) < 0 || /* add all the loose refs */ + (error = packed_write(backend)) < 0) /* write back to disk */ + return error; return 0; } From 26416f6d20044d693ddfb57d719ee5183c065fc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 24 Dec 2015 17:51:19 +0000 Subject: [PATCH 08/13] refdb: add retry logic to the threaded tests The logic simply consists of retrying for as long as the library says the data is locked, but it eventually gets through. --- tests/threads/refdb.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/threads/refdb.c b/tests/threads/refdb.c index 9b1b37592..0c5cd2be6 100644 --- a/tests/threads/refdb.c +++ b/tests/threads/refdb.c @@ -52,7 +52,7 @@ static void *iterate_refs(void *arg) static void *create_refs(void *arg) { - int i; + int i, error; struct th_data *data = (struct th_data *) arg; git_oid head; char name[128]; @@ -70,7 +70,9 @@ static void *create_refs(void *arg) if (i == 5) { git_refdb *refdb; cl_git_pass(git_repository_refdb(&refdb, repo)); - cl_git_pass(git_refdb_compress(refdb)); + do { + error = git_refdb_compress(refdb); + } while (error == GIT_ELOCKED); git_refdb_free(refdb); } } @@ -86,7 +88,7 @@ static void *create_refs(void *arg) static void *delete_refs(void *arg) { - int i; + int i, error; struct th_data *data = (struct th_data *) arg; git_reference *ref; char name[128]; @@ -99,14 +101,20 @@ static void *delete_refs(void *arg) name, sizeof(name), "refs/heads/thread-%03d-%02d", (data->id) & ~0x3, i); if (!git_reference_lookup(&ref, repo, name)) { - cl_git_pass(git_reference_delete(ref)); + do { + error = git_reference_delete(ref); + } while (error == GIT_ELOCKED); + cl_git_pass(error); git_reference_free(ref); } if (i == 5) { git_refdb *refdb; cl_git_pass(git_repository_refdb(&refdb, repo)); - cl_git_pass(git_refdb_compress(refdb)); + do { + error = git_refdb_compress(refdb); + } while (error == GIT_ELOCKED); + cl_git_pass(error); git_refdb_free(refdb); } } From 40ffa07f4ff3616acb814ff3ef93df7206f5b5c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 31 Dec 2015 14:51:42 +0000 Subject: [PATCH 09/13] sortedcache: check file size after opening the file Checking the size before we open the file descriptor can lead to the file being replaced from under us when renames aren't quite atomic, so we can end up reading too little of the file, leading to us thinking the file is corrupted. --- src/sortedcache.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/sortedcache.c b/src/sortedcache.c index 5c2a167a7..ed4199b71 100644 --- a/src/sortedcache.c +++ b/src/sortedcache.c @@ -200,6 +200,7 @@ void git_sortedcache_runlock(git_sortedcache *sc) int git_sortedcache_lockandload(git_sortedcache *sc, git_buf *buf) { int error, fd; + struct stat st; if ((error = git_sortedcache_wlock(sc)) < 0) return error; @@ -207,19 +208,26 @@ int git_sortedcache_lockandload(git_sortedcache *sc, git_buf *buf) if ((error = git_futils_filestamp_check(&sc->stamp, sc->path)) <= 0) goto unlock; - if (!git__is_sizet(sc->stamp.size)) { - giterr_set(GITERR_INVALID, "Unable to load file larger than size_t"); - error = -1; - goto unlock; - } - if ((fd = git_futils_open_ro(sc->path)) < 0) { error = fd; goto unlock; } + if (p_fstat(fd, &st) < 0) { + giterr_set(GITERR_OS, "failed to stat file"); + error = -1; + goto unlock; + } + + if (!git__is_sizet(st.st_size)) { + giterr_set(GITERR_INVALID, "Unable to load file larger than size_t"); + error = -1; + (void)p_close(fd); + goto unlock; + } + if (buf) - error = git_futils_readbuffer_fd(buf, fd, (size_t)sc->stamp.size); + error = git_futils_readbuffer_fd(buf, fd, (size_t)st.st_size); (void)p_close(fd); From 33248b9edb74a6bac4223873402efe68a76a30a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 10 Mar 2016 12:22:34 +0100 Subject: [PATCH 10/13] refdb: remove a check-delete race when removing a loose ref It does not help us to check whether the file exists before trying to unlink it since it might be gone by the time unlink is called. Instead try to remove it and handle the resulting error if it did not exist. --- src/refdb_fs.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 6b55960e1..ab309c841 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1281,15 +1281,14 @@ static int refdb_fs_backend__delete_tail( if (git_buf_joinpath(&loose_path, backend->path, ref_name) < 0) return -1; - if (git_path_isfile(loose_path.ptr)) { - error = p_unlink(loose_path.ptr); - loose_deleted = 1; - } - git_buf_free(&loose_path); - - if (error != 0) + error = p_unlink(loose_path.ptr); + if (error < 0 && errno == ENOENT) + error = 0; + else if (error < 0) goto cleanup; + else if (error == 0) + loose_deleted = 1; if ((error = packed_reload(backend)) < 0) goto cleanup; @@ -1312,6 +1311,7 @@ static int refdb_fs_backend__delete_tail( error = packed_write(backend); cleanup: + git_buf_free(&loose_path); git_filebuf_cleanup(file); return error; From 7c32d874503ee43206cb5d2a8e65dbe23f18eaca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 10 Mar 2016 12:27:07 +0100 Subject: [PATCH 11/13] refdb: expect threaded test deletes to race At times we may try to delete a reference which a different thread has already taken care of. --- tests/threads/refdb.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/threads/refdb.c b/tests/threads/refdb.c index 0c5cd2be6..d8dc77ba5 100644 --- a/tests/threads/refdb.c +++ b/tests/threads/refdb.c @@ -104,6 +104,10 @@ static void *delete_refs(void *arg) do { error = git_reference_delete(ref); } while (error == GIT_ELOCKED); + /* Sometimes we race with other deleter threads */ + if (error == GIT_ENOTFOUND) + error = 0; + cl_git_pass(error); git_reference_free(ref); } From ce5553d48b8ba3510dd5032dfbfd161fb801cd77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 10 Mar 2016 22:01:09 +0100 Subject: [PATCH 12/13] refdb: bubble up locked files on the read side On Windows we can find locked files even when reading a reference or the packed-refs file. Bubble up the error in this case as well to allow callers on Windows to retry more intelligently. --- src/path.c | 4 ++++ src/refdb.c | 6 ++++-- src/refdb_fs.c | 38 +++++++++++++++++++++----------------- tests/threads/refdb.c | 17 +++++++++++++---- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/path.c b/src/path.c index f91e42242..2b1a9622e 100644 --- a/src/path.c +++ b/src/path.c @@ -644,6 +644,10 @@ int git_path_set_error(int errno_value, const char *path, const char *action) giterr_set(GITERR_OS, "Failed %s - '%s' already exists", action, path); return GIT_EEXISTS; + case EACCES: + giterr_set(GITERR_OS, "Failed %s - '%s' is locked", action, path); + return GIT_ELOCKED; + default: giterr_set(GITERR_OS, "Could not %s '%s'", action, path); return -1; diff --git a/src/refdb.c b/src/refdb.c index debba1276..85c84892b 100644 --- a/src/refdb.c +++ b/src/refdb.c @@ -125,13 +125,15 @@ int git_refdb_lookup(git_reference **out, git_refdb *db, const char *ref_name) int git_refdb_iterator(git_reference_iterator **out, git_refdb *db, const char *glob) { + int error; + if (!db->backend || !db->backend->iterator) { giterr_set(GITERR_REFERENCE, "This backend doesn't support iterators"); return -1; } - if (db->backend->iterator(out, db->backend, glob) < 0) - return -1; + if ((error = db->backend->iterator(out, db->backend, glob)) < 0) + return error; GIT_REFCOUNT_INC(db); (*out)->db = db; diff --git a/src/refdb_fs.c b/src/refdb_fs.c index ab309c841..7601aa0ac 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -326,12 +326,13 @@ static int refdb_fs_backend__exists( { refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_buf ref_path = GIT_BUF_INIT; + int error; assert(backend); - if (packed_reload(backend) < 0 || - git_buf_joinpath(&ref_path, backend->path, ref_name) < 0) - return -1; + if ((error = packed_reload(backend)) < 0 || + (error = git_buf_joinpath(&ref_path, backend->path, ref_name)) < 0) + return error; *exists = git_path_isfile(ref_path.ptr) || (git_sortedcache_lookup(backend->refcache, ref_name) != NULL); @@ -409,8 +410,8 @@ static int packed_lookup( int error = 0; struct packref *entry; - if (packed_reload(backend) < 0) - return -1; + if ((error = packed_reload(backend)) < 0) + return error; if (git_sortedcache_rlock(backend->refcache) < 0) return -1; @@ -615,13 +616,14 @@ static int refdb_fs_backend__iterator_next_name( static int refdb_fs_backend__iterator( git_reference_iterator **out, git_refdb_backend *_backend, const char *glob) { + int error; refdb_fs_iter *iter; refdb_fs_backend *backend = (refdb_fs_backend *)_backend; assert(backend); - if (packed_reload(backend) < 0) - return -1; + if ((error = packed_reload(backend)) < 0) + return error; iter = git__calloc(1, sizeof(refdb_fs_iter)); GITERR_CHECK_ALLOC(iter); @@ -674,16 +676,18 @@ static int reference_path_available( int force) { size_t i; + int error; - if (packed_reload(backend) < 0) - return -1; + if ((error = packed_reload(backend)) < 0) + return error; if (!force) { int exists; - if (refdb_fs_backend__exists( - &exists, (git_refdb_backend *)backend, new_ref) < 0) - return -1; + if ((error = refdb_fs_backend__exists( + &exists, (git_refdb_backend *)backend, new_ref)) < 0) { + return error; + } if (exists) { giterr_set(GITERR_REFERENCE, @@ -1164,8 +1168,7 @@ static int refdb_fs_backend__write( assert(backend); - error = reference_path_available(backend, ref->name, NULL, force); - if (error < 0) + if ((error = reference_path_available(backend, ref->name, NULL, force)) < 0) return error; /* We need to perform the reflog append and old value check under the ref's lock */ @@ -1811,9 +1814,10 @@ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, co * there maybe an obsolete/unused directory (or directory hierarchy) in the way. */ if (git_path_isdir(git_buf_cstr(&path))) { - if ((git_futils_rmdir_r(git_buf_cstr(&path), NULL, GIT_RMDIR_SKIP_NONEMPTY) < 0)) - error = -1; - else if (git_path_isdir(git_buf_cstr(&path))) { + if ((error = git_futils_rmdir_r(git_buf_cstr(&path), NULL, GIT_RMDIR_SKIP_NONEMPTY)) < 0) { + if (error == GIT_ENOTFOUND) + error = 0; + } else if (git_path_isdir(git_buf_cstr(&path))) { giterr_set(GITERR_REFERENCE, "cannot create reflog at '%s', there are reflogs beneath that folder", ref->name); error = GIT_EDIRECTORY; diff --git a/tests/threads/refdb.c b/tests/threads/refdb.c index d8dc77ba5..ae1a935de 100644 --- a/tests/threads/refdb.c +++ b/tests/threads/refdb.c @@ -29,11 +29,14 @@ static void *iterate_refs(void *arg) struct th_data *data = (struct th_data *) arg; git_reference_iterator *i; git_reference *ref; - int count = 0; + int count = 0, error; git_repository *repo; cl_git_pass(git_repository_open(&repo, data->path)); - cl_git_pass(git_reference_iterator_new(&i, repo)); + do { + error = git_reference_iterator_new(&i, repo); + } while (error == GIT_ELOCKED); + cl_git_pass(error); for (count = 0; !git_reference_next(&ref, i); ++count) { cl_assert(ref != NULL); @@ -61,11 +64,17 @@ static void *create_refs(void *arg) cl_git_pass(git_repository_open(&repo, data->path)); - cl_git_pass(git_reference_name_to_id(&head, repo, "HEAD")); + do { + error = git_reference_name_to_id(&head, repo, "HEAD"); + } while (error == GIT_ELOCKED); + cl_git_pass(error); for (i = 0; i < 10; ++i) { p_snprintf(name, sizeof(name), "refs/heads/thread-%03d-%02d", data->id, i); - cl_git_pass(git_reference_create(&ref[i], repo, name, &head, 0, NULL)); + do { + error = git_reference_create(&ref[i], repo, name, &head, 0, NULL); + } while (error == GIT_ELOCKED); + cl_git_pass(error); if (i == 5) { git_refdb *refdb; From aef54a466ac5403690e7ff6c72fc1252dabf5899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 14 Nov 2016 11:29:40 +0100 Subject: [PATCH 13/13] refdb: use a constant for the number of per-thread creations/deletes --- tests/threads/refdb.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/threads/refdb.c b/tests/threads/refdb.c index ae1a935de..5484b71d6 100644 --- a/tests/threads/refdb.c +++ b/tests/threads/refdb.c @@ -18,6 +18,8 @@ void test_threads_refdb__cleanup(void) #define REPEAT 20 #define THREADS 20 +/* Number of references to create or delete in each thread */ +#define NREFS 10 struct th_data { int id; @@ -59,7 +61,7 @@ static void *create_refs(void *arg) struct th_data *data = (struct th_data *) arg; git_oid head; char name[128]; - git_reference *ref[10]; + git_reference *ref[NREFS]; git_repository *repo; cl_git_pass(git_repository_open(&repo, data->path)); @@ -69,14 +71,14 @@ static void *create_refs(void *arg) } while (error == GIT_ELOCKED); cl_git_pass(error); - for (i = 0; i < 10; ++i) { + for (i = 0; i < NREFS; ++i) { p_snprintf(name, sizeof(name), "refs/heads/thread-%03d-%02d", data->id, i); do { error = git_reference_create(&ref[i], repo, name, &head, 0, NULL); } while (error == GIT_ELOCKED); cl_git_pass(error); - if (i == 5) { + if (i == NREFS/2) { git_refdb *refdb; cl_git_pass(git_repository_refdb(&refdb, repo)); do { @@ -86,7 +88,7 @@ static void *create_refs(void *arg) } } - for (i = 0; i < 10; ++i) + for (i = 0; i < NREFS; ++i) git_reference_free(ref[i]); git_repository_free(repo); @@ -105,7 +107,7 @@ static void *delete_refs(void *arg) cl_git_pass(git_repository_open(&repo, data->path)); - for (i = 0; i < 10; ++i) { + for (i = 0; i < NREFS; ++i) { p_snprintf( name, sizeof(name), "refs/heads/thread-%03d-%02d", (data->id) & ~0x3, i); @@ -121,7 +123,7 @@ static void *delete_refs(void *arg) git_reference_free(ref); } - if (i == 5) { + if (i == NREFS/2) { git_refdb *refdb; cl_git_pass(git_repository_refdb(&refdb, repo)); do {