{"thread":{"id":"39293","subject":"[PATCH v2 1/2] lockfile: allow file locking to be retried with a timeout","startedAt":"2015-05-11T10:35:24Z","lastAt":"2015-05-11T10:35:26Z","messageCount":3,"participants":["Michael Haggerty"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"260963","messageId":"1431340526-24288-1-git-send-email-mhagger@alum.mit.edu","threadId":"39293","inReplyTo":null,"subject":"[PATCH v2 0/2] Retry attempts to acquire the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T10:35:24Z","receivedAt":"2015-05-11T10:35:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a re-roll of [1]. The only change is the addition of a \"wait\"\nin the test to avoid an unlikely race between the lockfile-removing\nsubprocess and the attempt by the outer layer of the test to delete\nthe test directory.\n\nThanks to Hannes, Stefan, and Peff for their comments about v1.\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/268137\n\nMichael Haggerty (2):\n  lockfile: allow file locking to be retried with a timeout\n  lock_packed_refs(): allow retries when acquiring the packed-refs lock\n\n Documentation/config.txt |  6 ++++\n lockfile.c               | 79 ++++++++++++++++++++++++++++++++++++++++++++++--\n lockfile.h               | 16 ++++++++--\n refs.c                   | 12 +++++++-\n t/t3210-pack-refs.sh     | 17 +++++++++++\n 5 files changed, 125 insertions(+), 5 deletions(-)\n\n-- \n2.1.4\n"},{"id":"260962","messageId":"1431340526-24288-2-git-send-email-mhagger@alum.mit.edu","threadId":"39293","inReplyTo":"1431340526-24288-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/2] lockfile: allow file locking to be retried with a timeout","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T10:35:25Z","receivedAt":"2015-05-11T10:35:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Currently, there is only one attempt to lock a file. If it fails, the\nwhole operation fails.\n\nBut it might sometimes be advantageous to try acquiring a file lock a\nfew times before giving up. So add a new function,\nhold_lock_file_for_update_timeout(), that allows a timeout to be\nspecified. Make hold_lock_file_for_update() a thin wrapper around the\nnew function.\n\nIf timeout_ms is positive, then retry for at least that many\nmilliseconds to acquire the lock. On each failed attempt, use select()\nto wait for a backoff time that increases quadratically (capped at 1\nsecond) and has a random component to prevent two processes from\ngetting synchronized. If timeout_ms is negative, retry indefinitely.\n\nIn a moment we will switch to using the new function when locking\npacked-refs.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nNotes (discussion):\n    It would be easy to also add a hold_lock_file_for_append_timeout(),\n    but I can't yet think of an application for it.\n\n lockfile.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n lockfile.h | 16 +++++++++++--\n 2 files changed, 91 insertions(+), 4 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9889277..30e65e9 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -157,6 +157,80 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \treturn lk->fd;\n }\n \n+static int sleep_microseconds(long us)\n+{\n+\tstruct timeval tv;\n+\ttv.tv_sec = 0;\n+\ttv.tv_usec = us;\n+\treturn select(0, NULL, NULL, NULL, &tv);\n+}\n+\n+/*\n+ * Constants defining the gaps between attempts to lock a file. The\n+ * first backoff period is approximately INITIAL_BACKOFF_MS\n+ * milliseconds. The longest backoff period is approximately\n+ * (BACKOFF_MAX_MULTIPLIER * INITIAL_BACKOFF_MS) milliseconds.\n+ */\n+#define INITIAL_BACKOFF_MS 1L\n+#define BACKOFF_MAX_MULTIPLIER 1000\n+\n+/*\n+ * Try locking path, retrying with quadratic backoff for at least\n+ * timeout_ms milliseconds. If timeout_ms is 0, try locking the file\n+ * exactly once. If timeout_ms is -1, try indefinitely.\n+ */\n+static int lock_file_timeout(struct lock_file *lk, const char *path,\n+\t\t\t     int flags, long timeout_ms)\n+{\n+\tint n = 1;\n+\tint multiplier = 1;\n+\tlong remaining_us = 0;\n+\tstatic int random_initialized = 0;\n+\n+\tif (timeout_ms == 0)\n+\t\treturn lock_file(lk, path, flags);\n+\n+\tif (!random_initialized) {\n+\t\tsrandom((unsigned int)getpid());\n+\t\trandom_initialized = 1;\n+\t}\n+\n+\tif (timeout_ms > 0) {\n+\t\t/* avoid overflow */\n+\t\tif (timeout_ms <= LONG_MAX / 1000)\n+\t\t\tremaining_us = timeout_ms * 1000;\n+\t\telse\n+\t\t\tremaining_us = LONG_MAX;\n+\t}\n+\n+\twhile (1) {\n+\t\tlong backoff_ms, wait_us;\n+\t\tint fd;\n+\n+\t\tfd = lock_file(lk, path, flags);\n+\n+\t\tif (fd >= 0)\n+\t\t\treturn fd; /* success */\n+\t\telse if (errno != EEXIST)\n+\t\t\treturn -1; /* failure other than lock held */\n+\t\telse if (timeout_ms > 0 && remaining_us <= 0)\n+\t\t\treturn -1; /* failure due to timeout */\n+\n+\t\tbackoff_ms = multiplier * INITIAL_BACKOFF_MS;\n+\t\t/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */\n+\t\twait_us = (750 + random() % 500) * backoff_ms;\n+\t\tsleep_microseconds(wait_us);\n+\t\tremaining_us -= wait_us;\n+\n+\t\t/* Recursion: (n+1)^2 = n^2 + 2n + 1 */\n+\t\tmultiplier += 2*n + 1;\n+\t\tif (multiplier > BACKOFF_MAX_MULTIPLIER)\n+\t\t\tmultiplier = BACKOFF_MAX_MULTIPLIER;\n+\t\telse\n+\t\t\tn++;\n+\t}\n+}\n+\n void unable_to_lock_message(const char *path, int err, struct strbuf *buf)\n {\n \tif (err == EEXIST) {\n@@ -179,9 +253,10 @@ NORETURN void unable_to_lock_die(const char *path, int err)\n }\n \n /* This should return a meaningful errno on failure */\n-int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n+int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n+\t\t\t\t      int flags, long timeout_ms)\n {\n-\tint fd = lock_file(lk, path, flags);\n+\tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n \tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n \t\tunable_to_lock_die(path, errno);\n \treturn fd;\ndiff --git a/lockfile.h b/lockfile.h\nindex cd2ec95..b4abc61 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -74,8 +74,20 @@ struct lock_file {\n extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t   struct strbuf *buf);\n extern NORETURN void unable_to_lock_die(const char *path, int err);\n-extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n-extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_update_timeout(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, long timeout_ms);\n+\n+static inline int hold_lock_file_for_update(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags)\n+{\n+\treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n+}\n+\n+extern int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n+\t\t\t\t     int flags);\n+\n extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\n extern char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\n-- \n2.1.4\n"},{"id":"260964","messageId":"1431340526-24288-3-git-send-email-mhagger@alum.mit.edu","threadId":"39293","inReplyTo":"1431340526-24288-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T10:35:26Z","receivedAt":"2015-05-11T10:35:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Currently, there is only one attempt to acquire any lockfile, and if\nthe lock is held by another process, the locking attempt fails\nimmediately.\n\nThis is not such a limitation for loose reference files. First, they\ndon't take long to rewrite. Second, most reference updates have a\nknown \"old\" value, so if another process is updating a reference at\nthe same moment that we are trying to lock it, then probably the\nexpected \"old\" value will not longer be valid, and the update will\nfail anyway.\n\nBut these arguments do not hold for packed-refs:\n\n* The packed-refs file can be large and take significant time to\n  rewrite.\n\n* Many references are stored in a single packed-refs file, so it could\n  be that the other process was changing a different reference than\n  the one that we are interested in.\n\nTherefore, it is much more likely for there to be spurious lock\nconflicts in connection to the packed-refs file, resulting in\nunnecessary command failures.\n\nSo, if the first attempt to lock the packed-refs file fails, continue\nretrying for a configurable length of time before giving up. The\ndefault timeout is 1 second.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n Documentation/config.txt |  6 ++++++\n refs.c                   | 12 +++++++++++-\n t/t3210-pack-refs.sh     | 17 +++++++++++++++++\n 3 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2e5ceaf..3c24e10 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -622,6 +622,12 @@ core.commentChar::\n If set to \"auto\", `git-commit` would select a character that is not\n the beginning character of any line in existing commit messages.\n \n+core.packedRefsTimeout::\n+\tThe length of time, in milliseconds, to retry when trying to\n+\tlock the `packed-refs` file. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 1000 (i.e.,\n+\tretry for 1 second).\n+\n sequence.editor::\n \tText editor used by `git rebase -i` for editing the rebase instruction file.\n \tThe value is meant to be interpreted by the shell when it is used.\ndiff --git a/refs.c b/refs.c\nindex 47e4e53..3f8ac63 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2413,9 +2413,19 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n /* This should return a meaningful errno on failure */\n int lock_packed_refs(int flags)\n {\n+\tstatic int timeout_configured = 0;\n+\tstatic int timeout_value = 1000;\n+\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), flags) < 0)\n+\tif (!timeout_configured) {\n+\t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n+\t\ttimeout_configured = 1;\n+\t}\n+\n+\tif (hold_lock_file_for_update_timeout(\n+\t\t\t    &packlock, git_path(\"packed-refs\"),\n+\t\t\t    flags, timeout_value) < 0)\n \t\treturn -1;\n \t/*\n \t * Get the current packed-refs while holding the lock.  If the\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex aa9eb3a..e5e3204 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -187,4 +187,21 @@ test_expect_success 'notice d/f conflict with existing ref' '\n \ttest_must_fail git branch foo/bar/baz/lots/of/extra/components\n '\n \n+test_expect_success 'timeout if packed-refs.lock exists' '\n+\tLOCK=.git/packed-refs.lock &&\n+\t>$LOCK &&\n+\ttest_when_finished \"rm -f $LOCK\" &&\n+\ttest_must_fail git pack-refs --all --prune\n+'\n+\n+test_expect_success 'retry acquiring packed-refs.lock' '\n+\tLOCK=.git/packed-refs.lock &&\n+\t>$LOCK &&\n+\ttest_when_finished \"wait; rm -f $LOCK\" &&\n+\t{\n+\t\t( sleep 1 ; rm -f $LOCK ) &\n+\t} &&\n+\tgit -c core.packedrefstimeout=3000 pack-refs --all --prune\n+'\n+\n test_done\n-- \n2.1.4\n"}]}