refs: retry acquiring reference locks for 100ms

The philosophy of reference locking has been, "if another process is
changing a reference, then whatever I'm trying to do to it will
probably fail anyway because my old-SHA-1 value is probably no longer
current". But this argument falls down if the other process has locked
the reference to do something that doesn't actually change the value
of the reference, such as `pack-refs` or `reflog expire`. There
actually *is* a decent chance that a planned reference update will
still be able to go through after the other process has released the
lock.

So when trying to lock an individual reference (e.g., when creating
"refs/heads/master.lock"), if it is already locked, then retry the
lock acquisition for approximately 100 ms before giving up. This
should eliminate some unnecessary lock conflicts without wasting a lot
of time.

Add a configuration setting, `core.filesRefLockTimeout`, to allow this
setting to be tweaked.

Note: the function `get_files_ref_lock_timeout_ms()` cannot be private
to the files backend because it is also used by `write_pseudoref()`
and `delete_pseudoref()`, which are defined in `refs.c` so that they
can be used by other reference backends.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5c9c4cab60531d178d5c11d623a5c3f1036a0d0..2c04b9dfb4d43e6ade316d05d93a284b02bfa65f 100644 (file)
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -776,6 +776,12 @@ core.commentChar::
 If set to "auto", `git-commit` would select a character that is not
 the beginning character of any line in existing commit messages.
 
 If set to "auto", `git-commit` would select a character that is not
 the beginning character of any line in existing commit messages.
 

+core.filesRefLockTimeout::
+       The length of time, in milliseconds, to retry when trying to
+       lock an individual reference. Value 0 means not to retry at
+       all; -1 means to try indefinitely. Default is 100 (i.e.,
+       retry for 100ms).
+

 core.packedRefsTimeout::
        The length of time, in milliseconds, to retry when trying to
        lock the `packed-refs` file. Value 0 means not to retry at
 core.packedRefsTimeout::
        The length of time, in milliseconds, to retry when trying to
        lock the `packed-refs` file. Value 0 means not to retry at

diff --git a/refs.c b/refs.c
index ba22f4acefa262cc1029bcf8360edb21c0635953..748e34d7c2d07cc420a50292d145e08823c21ed2 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -561,6 +561,21 @@ enum ref_type ref_type(const char *refname)
        return REF_TYPE_NORMAL;
 }
 
        return REF_TYPE_NORMAL;
 }
 

+long get_files_ref_lock_timeout_ms(void)
+{
+       static int configured = 0;
+
+       /* The default timeout is 100 ms: */
+       static int timeout_ms = 100;
+
+       if (!configured) {
+               git_config_get_int("core.filesreflocktimeout", &timeout_ms);
+               configured = 1;
+       }
+
+       return timeout_ms;
+}
+

 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
                           const unsigned char *old_sha1, struct strbuf *err)
 {
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
                           const unsigned char *old_sha1, struct strbuf *err)
 {


@@ -573,7 +588,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
        strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
 
        filename = git_path("%s", pseudoref);
        strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
 
        filename = git_path("%s", pseudoref);

-       fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
+       fd = hold_lock_file_for_update_timeout(&lock, filename,
+                                              LOCK_DIE_ON_ERROR,
+                                              get_files_ref_lock_timeout_ms());

        if (fd < 0) {
                strbuf_addf(err, "could not open '%s' for writing: %s",
                            filename, strerror(errno));
        if (fd < 0) {
                strbuf_addf(err, "could not open '%s' for writing: %s",
                            filename, strerror(errno));


@@ -616,8 +633,9 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
                int fd;
                unsigned char actual_old_sha1[20];
 
                int fd;
                unsigned char actual_old_sha1[20];
 

-               fd = hold_lock_file_for_update(&lock, filename,
-                                              LOCK_DIE_ON_ERROR);
+               fd = hold_lock_file_for_update_timeout(
+                               &lock, filename, LOCK_DIE_ON_ERROR,
+                               get_files_ref_lock_timeout_ms());

                if (fd < 0)
                        die_errno(_("Could not open '%s' for writing"), filename);
                if (read_ref(pseudoref, actual_old_sha1))
                if (fd < 0)
                        die_errno(_("Could not open '%s' for writing"), filename);
                if (read_ref(pseudoref, actual_old_sha1))

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0404f2c2333c0fadeb57d225deedc6f0d4afadad..d611e0f7d7760b4e85fc6d3c9fd26cb2646eeea4 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -855,7 +855,9 @@ static int lock_raw_ref(struct files_ref_store *refs,
        if (!lock->lk)
                lock->lk = xcalloc(1, sizeof(struct lock_file));
 
        if (!lock->lk)
                lock->lk = xcalloc(1, sizeof(struct lock_file));
 

-       if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {
+       if (hold_lock_file_for_update_timeout(
+                           lock->lk, ref_file.buf, LOCK_NO_DEREF,
+                           get_files_ref_lock_timeout_ms()) < 0) {

                if (errno == ENOENT && --attempts_remaining > 0) {
                        /*
                         * Maybe somebody just deleted one of the
                if (errno == ENOENT && --attempts_remaining > 0) {
                        /*
                         * Maybe somebody just deleted one of the


@@ -1181,7 +1183,9 @@ static int create_reflock(const char *path, void *cb)
 {
        struct lock_file *lk = cb;
 
 {
        struct lock_file *lk = cb;
 

-       return hold_lock_file_for_update(lk, path, LOCK_NO_DEREF) < 0 ? -1 : 0;
+       return hold_lock_file_for_update_timeout(
+                       lk, path, LOCK_NO_DEREF,
+                       get_files_ref_lock_timeout_ms()) < 0 ? -1 : 0;

 }
 
 /*
 }
 
 /*

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 192f9f85c97c0d7ca4e9c933e27ba0686da31dbe..9977fea98b666bae75caf019ad43545ab953e262 100644 (file)
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -61,6 +61,12 @@
  */
 #define REF_DELETED_LOOSE 0x200
 
  */
 #define REF_DELETED_LOOSE 0x200
 

+/*
+ * Return the length of time to retry acquiring a loose reference lock
+ * before giving up, in milliseconds:
+ */
+long get_files_ref_lock_timeout_ms(void);
+

 /*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
 /*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for