{"thread":{"id":"39204","subject":"[PATCH 0/2] Retry attempts to acquire the packed-refs lock","startedAt":"2015-05-01T14:52:55Z","lastAt":"2015-05-11T18:04:54Z","messageCount":18,"participants":["Michael Haggerty","Johannes Sixt","Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"260335","messageId":"1430491977-25817-1-git-send-email-mhagger@alum.mit.edu","threadId":"39204","inReplyTo":null,"subject":"[PATCH 0/2] Retry attempts to acquire the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T14:52:55Z","receivedAt":"2015-05-01T14:52:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"At GitHub we were seeing occasional lock contention over packed-refs.\nIt wasn't very common, but when you have as much git traffic as we\nhave, anything that *can* happen *will* happen.\n\nThe problem is that Git only tries to acquire locks a single time. If\nthat attempt fails, the whole process fails. So, for example, if two\nprocesses are trying to delete two different references, one of them\ncan fail due to inability to acquire the packed-refs lock, even though\nit could have succeeded if it had just waited a moment.\n\nThis patch series adds a new function,\nhold_lock_file_for_update_timeout(), which retries the lock\nacquisition for a specified length of time. The retries necessarily\nhave to use polling, which it does using a quadratic backoff with a\nrandom component. It might be a bit overengineered for this single\npurpose, but I wanted it to be usable in other contexts without second\nthoughts.\n\nThis patch series also changes lock_packed_refs() to call the new\nfunction with a timeout of 1 second (by default; the timeout is\nconfigurable) and adds some tests that the retry and timeout are\nworking.\n\nIt might be that there are other call sites that would benefit from\nretrying lock acquisition (the index file?), but this patch series\nonly applies the new functionality to packed-refs.lock.\n\nWe have a patch similar to this one running on our servers at GitHub,\nwith no problems observed so far.\n\nThis series applies to master. It is also available from my GitHub\nrepository:\n\n https://github.com/mhagger/git branch lockfile-retry\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":"260337","messageId":"1430491977-25817-2-git-send-email-mhagger@alum.mit.edu","threadId":"39204","inReplyTo":"1430491977-25817-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/2] lockfile: allow file locking to be retried with a timeout","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T14:52:56Z","receivedAt":"2015-05-01T14:52:56Z","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 \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":"260336","messageId":"1430491977-25817-3-git-send-email-mhagger@alum.mit.edu","threadId":"39204","inReplyTo":"1430491977-25817-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T14:52:57Z","receivedAt":"2015-05-01T14:52:57Z","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 \n---\n\nNotes (discussion):\n At first I wasn't going to make this setting configurable. After all,\n who could object to one second?\n \n But then I realized that making the length of the timeout configurable\n would make it easier to test. Since sleep(1) is only guaranteed to\n have a 1-second resolution, it is helpful to set the timeout longer\n than 1 second in the test. So I made it configurable, and documented\n the setting.\n \n I don't have a strong opinion either way.\n \n By the way, if anybody has a better idea for how to test this, please\n chime up. As written, the two new tests each take about one second to\n run (though they are mostly idle during that time, so they parallelize\n well with other tests).\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..d18b726 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 \"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"},{"id":"260351","messageId":"5543A621.7010208@kdbg.org","threadId":"39204","inReplyTo":"1430491977-25817-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-05-01T16:13:21Z","receivedAt":"2015-05-01T16:13:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.05.2015 um 16:52 schrieb Michael Haggerty:\n> +test_expect_success 'retry acquiring packed-refs.lock' '\n> +\tLOCK=.git/packed-refs.lock &&\n> +\t>$LOCK &&\n> +\ttest_when_finished \"rm -f $LOCK\" &&\n> +\t{\n> +\t\t( sleep 1 ; rm -f $LOCK ) &\n> +\t} &&\n\nI haven't tested yet, but I think that this will be problematic on \nWindows: a directory cannot be removed if it is the current directory of \na process. Here, the sub-shell process is alive for a second. If the \nremainder of the test script completes before the process dies, the test \ndirectory cannot be removed.\n\nHow about this:\n\n\ttest_when_finished \"wait; rm -f $LOCK\" &&\n\t{ sleep 1 & } &&\n\t...\n\n> +\tgit -c core.packedrefstimeout=3000 pack-refs --all --prune\n> +'\n> +\n> test_done\n>\n\n-- Hannes\n"},{"id":"260364","messageId":"CAGZ79kZnhv+aW_GW8mBDyhfv_k54ScAFsHQz=8zfHBUJ8WrVUw@mail.gmail.com","threadId":"39204","inReplyTo":"1430491977-25817-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-01T17:51:47Z","receivedAt":"2015-05-01T17:51:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 1, 2015 at 7:52 AM, Michael Haggerty wrote:\n> Currently, there is only one attempt to acquire any lockfile, and if\n> the lock is held by another process, the locking attempt fails\n> immediately.\n>\n> This is not such a limitation for loose reference files. First, they\n> don't take long to rewrite. Second, most reference updates have a\n> known \"old\" value, so if another process is updating a reference at\n> the same moment that we are trying to lock it, then probably the\n> expected \"old\" value will not longer be valid, and the update will\n> fail anyway.\n>\n> But 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>\n> Therefore, it is much more likely for there to be spurious lock\n> conflicts in connection to the packed-refs file, resulting in\n> unnecessary command failures.\n>\n> So, if the first attempt to lock the packed-refs file fails, continue\n> retrying for a configurable length of time before giving up. The\n> default timeout is 1 second.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n>\n> Notes (discussion):\n> At first I wasn't going to make this setting configurable. After all,\n> who could object to one second?\n>\n> But then I realized that making the length of the timeout configurable\n> would make it easier to test. Since sleep(1) is only guaranteed to\n> have a 1-second resolution, it is helpful to set the timeout longer\n> than 1 second in the test. So I made it configurable, and documented\n> the setting.\n>\n> I don't have a strong opinion either way.\n>\n> By the way, if anybody has a better idea for how to test this, please\n> chime up. As written, the two new tests each take about one second to\n> run (though they are mostly idle during that time, so they parallelize\n> well with other tests).\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>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 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> + The length of time, in milliseconds, to retry when trying to\n> + lock the `packed-refs` file. Value 0 means not to retry at\n> + all; -1 means to try indefinitely. Default is 1000 (i.e.,\n> + retry for 1 second).\n> +\n> sequence.editor::\n> Text editor used by `git rebase -i` for editing the rebase instruction file.\n> The value is meant to be interpreted by the shell when it is used.\n> diff --git a/refs.c b/refs.c\n> index 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> + static int timeout_configured = 0;\n> + static int timeout_value = 1000;\n\nI'd personally be more happier with a default value of 100 ms or less\nThe reason is found in the human nature, as humans tend to perceive\nanything faster than 100ms as \"instant\"[1], while a 100ms is a long time\nfor computers.\n\nNow a small default time may lead to to little retries, so maybe it's worth\nchecking at the very end of the time again (ignoring the computed backoff\ntimes). As pushes to $server usually take a while (connecting, packing packs,\nwriting objects etc), this may be overcautios bikeshedding from my side.\n\n[1] http://stackoverflow.com/questions/536300/what-is-the-shortest-perceivable-application-response-delay\n\n\n> +\n> struct packed_ref_cache *packed_ref_cache;\n>\n> - if (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), flags) < 0)\n> + if (!timeout_configured) {\n> + git_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n> + timeout_configured = 1;\n> + }\n> +\n> + if (hold_lock_file_for_update_timeout(\n> + &packlock, git_path(\"packed-refs\"),\n> + flags, timeout_value) < 0)\n> return -1;\n> /*\n> * Get the current packed-refs while holding the lock. If the\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index aa9eb3a..d18b726 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> test_must_fail git branch foo/bar/baz/lots/of/extra/components\n> '\n>\n> +test_expect_success 'timeout if packed-refs.lock exists' '\n> + LOCK=.git/packed-refs.lock &&\n> + >$LOCK &&\n> + test_when_finished \"rm -f $LOCK\" &&\n> + test_must_fail git pack-refs --all --prune\n> +'\n> +\n> +test_expect_success 'retry acquiring packed-refs.lock' '\n> + LOCK=.git/packed-refs.lock &&\n> + >$LOCK &&\n> + test_when_finished \"rm -f $LOCK\" &&\n> + {\n> + ( sleep 1 ; rm -f $LOCK ) &\n> + } &&\n> + git -c core.packedrefstimeout=3000 pack-refs --all --prune\n> +'\n> +\n> test_done\n> --\n> 2.1.4\n>\n"},{"id":"260368","messageId":"20150501182257.GA27728@peff.net","threadId":"39204","inReplyTo":"CAGZ79kZnhv+aW_GW8mBDyhfv_k54ScAFsHQz=8zfHBUJ8WrVUw@mail.gmail.com","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-01T18:22:58Z","receivedAt":"2015-05-01T18:22:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 01, 2015 at 10:51:47AM -0700, Stefan Beller wrote:\n\n> > diff --git a/refs.c b/refs.c\n> > index 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> > + static int timeout_configured = 0;\n> > + static int timeout_value = 1000;\n> \n> I'd personally be more happier with a default value of 100 ms or less\n> The reason is found in the human nature, as humans tend to perceive\n> anything faster than 100ms as \"instant\"[1], while a 100ms is a long time\n> for computers.\n> \n> Now a small default time may lead to to little retries, so maybe it's worth\n> checking at the very end of the time again (ignoring the computed backoff\n> times). As pushes to $server usually take a while (connecting, packing packs,\n> writing objects etc), this may be overcautios bikeshedding from my side.\n\nKeep in mind that this 1s is the maximum time to wait. The\nlock_file_timeout() code from patch 1 starts off at 1ms, grows\nquadratically, and quits as soon as it succeeds. So in most cases, the\nuser will wait a much smaller amount of time.\n\nThe factors that go into this timeout length are really:\n\n 1. If there's a stale lockfile, the user will have to wait the whole\n period. How long do we keep retrying before giving up?\n\n 2. How long do we typically hold the lock for? Aside from absurd\n cases, writing out the packed-refs file isn't that expensive. But\n while holding the packed-refs lock, we may actually be iterating\n the loose refs, which can be rather slow on a cold-cache.\n\nIf we want to improve _responsiveness_ in the normal case, I think it's\nnot the max timeout we want to tweak but the resolution of retries.\nThat's set in patch 1 by the maximum backoff multiplier, which can put\nus up to 1s between retries. It might make sense to drop that to 500ms\nor even less.\n\n-Peff\n"},{"id":"260415","messageId":"554448B6.6080605@alum.mit.edu","threadId":"39204","inReplyTo":"5543A621.7010208@kdbg.org","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-02T03:47:02Z","receivedAt":"2015-05-02T03:47:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/01/2015 06:13 PM, Johannes Sixt wrote:\n> Am 01.05.2015 um 16:52 schrieb Michael Haggerty:\n>> +test_expect_success 'retry acquiring packed-refs.lock' '\n>> + LOCK=.git/packed-refs.lock &&\n>> + >$LOCK &&\n>> + test_when_finished \"rm -f $LOCK\" &&\n>> + {\n>> + ( sleep 1 ; rm -f $LOCK ) &\n>> + } &&\n> \n> I haven't tested yet, but I think that this will be problematic on\n> Windows: a directory cannot be removed if it is the current directory of\n> a process. Here, the sub-shell process is alive for a second. If the\n> remainder of the test script completes before the process dies, the test\n> directory cannot be removed.\n> \n> How about this:\n> \n> test_when_finished \"wait; rm -f $LOCK\" &&\n> { sleep 1 & } &&\n> ...\n> \n>> + git -c core.packedrefstimeout=3000 pack-refs --all --prune\n>> +'\n>> +\n>> test_done\n\nThanks for pointing out this problem. Your suggestion seems good. I\nassume that you didn't intend to omit the \"rm -f $LOCK\" from the\nsubprocess, because the whole point is for that to happen while \"git\npack-refs\" is running.\n\nI will include your change in v2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"260416","messageId":"55445E60.6010205@alum.mit.edu","threadId":"39204","inReplyTo":"20150501182257.GA27728@peff.net","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-02T05:19:28Z","receivedAt":"2015-05-02T05:19:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/01/2015 08:22 PM, Jeff King wrote:\n> On Fri, May 01, 2015 at 10:51:47AM -0700, Stefan Beller wrote:\n> \n>>> diff --git a/refs.c b/refs.c\n>>> index 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>>> + static int timeout_configured = 0;\n>>> + static int timeout_value = 1000;\n>>\n>> I'd personally be more happier with a default value of 100 ms or less\n>> The reason is found in the human nature, as humans tend to perceive\n>> anything faster than 100ms as \"instant\"[1], while a 100ms is a long time\n>> for computers.\n>>\n>> Now a small default time may lead to to little retries, so maybe it's worth\n>> checking at the very end of the time again (ignoring the computed backoff\n>> times). As pushes to $server usually take a while (connecting, packing packs,\n>> writing objects etc), this may be overcautios bikeshedding from my side.\n> \n> Keep in mind that this 1s is the maximum time to wait. The\n> lock_file_timeout() code from patch 1 starts off at 1ms, grows\n> quadratically, and quits as soon as it succeeds. So in most cases, the\n> user will wait a much smaller amount of time.\n> \n> The factors that go into this timeout length are really:\n> \n> 1. If there's a stale lockfile, the user will have to wait the whole\n> period. How long do we keep retrying before giving up?\n> \n> 2. How long do we typically hold the lock for? Aside from absurd\n> cases, writing out the packed-refs file isn't that expensive. But\n> while holding the packed-refs lock, we may actually be iterating\n> the loose refs, which can be rather slow on a cold-cache.\n> \n> If we want to improve _responsiveness_ in the normal case, I think it's\n> not the max timeout we want to tweak but the resolution of retries.\n> That's set in patch 1 by the maximum backoff multiplier, which can put\n> us up to 1s between retries. It might make sense to drop that to 500ms\n> or even less.\n\nThanks for the discussion.\n\n100 ms seems to be considered an acceptable delay between the time that\na user, say, clicks a button and the time that the button reacts. What\nwe are talking about is the time between the release of a lock by one\nprocess and the resumption of another process that was blocked waiting\nfor the lock. The former is probably not under the control of the user\nanyway, and perhaps not even observable by the user. Thus I don't think\nthat a perceivable delay between that event and the resumption of the\nblocked process would be annoying. The more salient delay is between the\ntime that the user started the blocked command and when that command\ncompleted. Let's look in more detail.\n\nThe current code would poll at the following times (in ms), ignoring the\ntime taken for the actual polling attempt and ignoring the random component:\n\n time backoff percent\n ---- ------- -------\n 0 1 N/A\n 1 4 400%\n 5 9 180%\n 14 16 114%\n 30 25 83%\n 55 36 65%\n 91 49 54%\n 140 64 46%\n 204 81 40%\n 285 100 35%\n 385 121 31%\n 506 144 28%\n 650 169 26%\n 819 196 24%\n 1015 225 22% <- Stop here with the default 1 s timeout\n 1240 256 21%\n 1496 289 19%\n 1785 324 18%\n 2109 361 17%\n 2470 400 16%\n 2870 441 15%\n 3311 484 15%\n 3795 529 14%\n 4324 576 13%\n 4900 625 13%\n 5525 676 12%\n 6201 729 12%\n 6930 784 11%\n 7714 841 11%\n 8555 900 11%\n 9455 961 10%\n 10416 1000 10%\n 11416 1000 9%\n 12416 1000 8%\n\n>From the table, the first backoff that is longer than 100 ms doesn't\nstart until 385 ms, and in the worst case, that 121 ms delay would\nincrease the *total* wait by only 31%. And the longest backoff within\nthe first second is only 196 ms. The backoff doesn't reach its maximum\nvalue, 1 s, until the process has already been blocked for 10.4 seconds.\n\nRemember, these backoffs are the *maximum* time that the user might wait\nbetween the time that one process is finished and the time that the\nsecond process resumes. The *average* wait time will be half of that.\n\nAnd finally, remember that lock contention should be very rare in the\nfirst place, and will mostly occur on servers (because normal users are\nunlikely to have lots of parallel processes accessing a single git\nrepository). What are the most likely scenarios for lock contention in\nthe real world?\n\n* Occasionally, by chance, under normal operations. In most cases, the\nblocking process will finish up within a few milliseconds, the blocked\nprocess will resume almost immediately, and nobody will notice a thing.\n\n* In a pathological repository that has, say, a million packed\nreferences, and writing the packed-refs file takes unusually long. Here,\ntypical lock contention delays will be long anyway, and adding (worst\ncase) 121 ms == 31% to the delay is not unreasonable.\n\n* When the server is struggling with enormous load, or a\ndenial-of-service attack, or some other system-wide problem that is\nmaking processes super slow. In this case it would be counterproductive\nto have *additional* processes waking up every 100 ms.\n\n* When a packed-refs.lock file fails to get cleaned up for some reason.\nIn this case the \"contention\" will never go away on its own, so the\npolling is wasted effort. (Happily, I've almost never seen this happen\non our servers.)\n\nIt would be trivial to increase or decrease the maximum backoff. But I\nthink the current value is a reasonable compromise.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"260437","messageId":"55454505.9060305@kdbg.org","threadId":"39204","inReplyTo":"554448B6.6080605@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-05-02T21:43:33Z","receivedAt":"2015-05-02T21:43:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.05.2015 um 05:47 schrieb Michael Haggerty:\n> On 05/01/2015 06:13 PM, Johannes Sixt wrote:\n>> How about this:\n>>\n>> test_when_finished \"wait; rm -f $LOCK\" &&\n>> { sleep 1 & } &&\n>> ...\n>\n> Thanks for pointing out this problem. Your suggestion seems good. I\n> assume that you didn't intend to omit the \"rm -f $LOCK\" from the\n> subprocess, because the whole point is for that to happen while \"git\n> pack-refs\" is running.\n\nI see. So, if git pack-refs works correctly, it waits for the \nsub-process, and the 'wait' in test_when_finished does not buy a lot. If \nthere is breakage, the trash directory is not attempted to be removed, \nand it does not matter that a process potentially occupies it. I think \nyour version is good then.\n\n-- Hannes\n"},{"id":"260447","messageId":"xmqqtwvuwgsp.fsf@gitster.dls.corp.google.com","threadId":"39204","inReplyTo":"1430491977-25817-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Retry attempts to acquire the packed-refs lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-03T02:12:06Z","receivedAt":"2015-05-03T02:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> At GitHub we were seeing occasional lock contention over packed-refs.\n> It wasn't very common, but when you have as much git traffic as we\n> have, anything that *can* happen *will* happen.\n>\n> The problem is that Git only tries to acquire locks a single time. If\n> that attempt fails, the whole process fails. So, for example, if two\n> processes are trying to delete two different references, one of them\n> can fail due to inability to acquire the packed-refs lock, even though\n> it could have succeeded if it had just waited a moment.\n\nYeah, try and abort may be perfectly fine for Git used interactively\nby humans, but is totally unsuitable for the server side. Thanks\nfor looking into this.\n"},{"id":"260530","messageId":"CAGZ79kbohcVBnx2euCtJP+VndBA7DzO7EJDEvPNC4m0arSVHdA@mail.gmail.com","threadId":"39204","inReplyTo":"55445E60.6010205@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-04T17:31:24Z","receivedAt":"2015-05-04T17:31:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 1, 2015 at 10:19 PM, Michael Haggerty wrote:\n> On 05/01/2015 08:22 PM, Jeff King wrote:\n>> On Fri, May 01, 2015 at 10:51:47AM -0700, Stefan Beller wrote:\n>>\n>>>> diff --git a/refs.c b/refs.c\n>>>> index 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>>>> + static int timeout_configured = 0;\n>>>> + static int timeout_value = 1000;\n>>>\n>>> I'd personally be more happier with a default value of 100 ms or less\n>>> The reason is found in the human nature, as humans tend to perceive\n>>> anything faster than 100ms as \"instant\"[1], while a 100ms is a long time\n>>> for computers.\n>>>\n>>> Now a small default time may lead to to little retries, so maybe it's worth\n>>> checking at the very end of the time again (ignoring the computed backoff\n>>> times). As pushes to $server usually take a while (connecting, packing packs,\n>>> writing objects etc), this may be overcautios bikeshedding from my side.\n>>\n>> Keep in mind that this 1s is the maximum time to wait. The\n>> lock_file_timeout() code from patch 1 starts off at 1ms, grows\n>> quadratically, and quits as soon as it succeeds. So in most cases, the\n>> user will wait a much smaller amount of time.\n\nYes, I forgot about that when having an opinion. I agree a one second\ntime out is reasonable.\n\n> The current code would poll at the following times (in ms), ignoring the\n> time taken for the actual polling attempt and ignoring the random component:\n>\n> time backoff percent\n> ---- ------- -------\n> 0 1 N/A\n> 1 4 400%\n> 5 9 180%\n> 14 16 114%\n> 30 25 83%\n> 55 36 65%\n> 91 49 54%\n> 140 64 46%\n> 204 81 40%\n> 285 100 35%\n> 385 121 31%\n> 506 144 28%\n> 650 169 26%\n> 819 196 24%\n> 1015 225 22% <- Stop here with the default 1 s timeout\n\nSo a configuration of one second only has about twice the attempts\nthan a 100ms configuration in the worst case, which is nice for the\nmachine load, and as I said above, not too long for the user.\n\nThanks for laying out the numbers here, it's more\nunderstandable now.\n\n> 1240 256 21%\n> 1496 289 19%\n> 1785 324 18%\n> 2109 361 17%\n> 2470 400 16%\n> 2870 441 15%\n> 3311 484 15%\n> 3795 529 14%\n> 4324 576 13%\n> 4900 625 13%\n> 5525 676 12%\n> 6201 729 12%\n> 6930 784 11%\n> 7714 841 11%\n> 8555 900 11%\n> 9455 961 10%\n> 10416 1000 10%\n> 11416 1000 9%\n> 12416 1000 8%\n>\n> From the table, the first backoff that is longer than 100 ms doesn't\n> start until 385 ms, and in the worst case, that 121 ms delay would\n> increase the *total* wait by only 31%. And the longest backoff within\n> the first second is only 196 ms. The backoff doesn't reach its maximum\n> value, 1 s, until the process has already been blocked for 10.4 seconds.\n>\n> Remember, these backoffs are the *maximum* time that the user might wait\n> between the time that one process is finished and the time that the\n> second process resumes. The *average* wait time will be half of that.\n>\n> And finally, remember that lock contention should be very rare in the\n> first place, and will mostly occur on servers (because normal users are\n> unlikely to have lots of parallel processes accessing a single git\n> repository). What are the most likely scenarios for lock contention in\n> the real world?\n>\n> * Occasionally, by chance, under normal operations. In most cases, the\n> blocking process will finish up within a few milliseconds, the blocked\n> process will resume almost immediately, and nobody will notice a thing.\n>\n> * In a pathological repository that has, say, a million packed\n> references, and writing the packed-refs file takes unusually long. Here,\n> typical lock contention delays will be long anyway, and adding (worst\n> case) 121 ms == 31% to the delay is not unreasonable.\n>\n> * When the server is struggling with enormous load, or a\n> denial-of-service attack, or some other system-wide problem that is\n> making processes super slow. In this case it would be counterproductive\n> to have *additional* processes waking up every 100 ms.\n>\n> * When a packed-refs.lock file fails to get cleaned up for some reason.\n> In this case the \"contention\" will never go away on its own, so the\n> polling is wasted effort. (Happily, I've almost never seen this happen\n> on our servers.)\n>\n> It would be trivial to increase or decrease the maximum backoff. But I\n> think the current value is a reasonable compromise.\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n>\n"},{"id":"260613","messageId":"20150505192110.GD10463@peff.net","threadId":"39204","inReplyTo":"55445E60.6010205@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-05T19:21:10Z","receivedAt":"2015-05-05T19:21:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 02, 2015 at 07:19:28AM +0200, Michael Haggerty wrote:\n\n> 100 ms seems to be considered an acceptable delay between the time that\n> a user, say, clicks a button and the time that the button reacts. What\n> we are talking about is the time between the release of a lock by one\n> process and the resumption of another process that was blocked waiting\n> for the lock. The former is probably not under the control of the user\n> anyway, and perhaps not even observable by the user. Thus I don't think\n> that a perceivable delay between that event and the resumption of the\n> blocked process would be annoying. The more salient delay is between the\n> time that the user started the blocked command and when that command\n> completed. Let's look in more detail.\n\nYeah, you can't impact when the other process will drop the lock, but if\nwe assume that it takes on the order of 100ms for the other process to\ndo its whole operation, then on average we experience half that. And\nthen tack on to that whatever time we waste in sleep() after the other\nguy drops the lock. And that's on average half of our backoff time.\n\nSo something like 100ms max backoff makes sense to me, in that it keeps\nus in the same order of magnitude as the expected time that the lock is\nheld.\n\nOf course these numbers are all grossly hand-wavy, and as you point out,\nthe current formula never even hits 100ms with the current 1s timeout,\nanyway. So for the record, I'm fine leaving your patch as-is.\n\nI think for our disgusting 1GB packed-refs files at GitHub, we will end\nup bumping the maximum timeout, but by my own argument above, it will be\nfine for the backoff to increase at the same time (i.e., they will\nremain in the same rough order of magnitude).\n\n-Peff\n"},{"id":"260960","messageId":"555076D4.9020601@alum.mit.edu","threadId":"39204","inReplyTo":"55454505.9060305@kdbg.org","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T09:31:00Z","receivedAt":"2015-05-11T09:31:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/02/2015 11:43 PM, Johannes Sixt wrote:\n> Am 02.05.2015 um 05:47 schrieb Michael Haggerty:\n>> On 05/01/2015 06:13 PM, Johannes Sixt wrote:\n>>> How about this:\n>>>\n>>> test_when_finished \"wait; rm -f $LOCK\" &&\n>>> { sleep 1 & } &&\n>>> ...\n>>\n>> Thanks for pointing out this problem. Your suggestion seems good. I\n>> assume that you didn't intend to omit the \"rm -f $LOCK\" from the\n>> subprocess, because the whole point is for that to happen while \"git\n>> pack-refs\" is running.\n> \n> I see. So, if git pack-refs works correctly, it waits for the\n> sub-process, and the 'wait' in test_when_finished does not buy a lot. If\n> there is breakage, the trash directory is not attempted to be removed,\n> and it does not matter that a process potentially occupies it. I think\n> your version is good then.\n\nWell, even if my version is working correctly, there is still a gap\nbetween when the subprocess removes the lockfile and when the subprocess\nends, during which the outer process could theoretically finish and try\nto delete the test directory. It may not cause problems in practice, but\nlet's add the wait anyway to make everything kosher.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"260961","messageId":"555083CF.8010205@alum.mit.edu","threadId":"39204","inReplyTo":"20150505192110.GD10463@peff.net","subject":"Re: [PATCH 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:26:23Z","receivedAt":"2015-05-11T10:26:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/05/2015 09:21 PM, Jeff King wrote:\n> On Sat, May 02, 2015 at 07:19:28AM +0200, Michael Haggerty wrote:\n> \n>> 100 ms seems to be considered an acceptable delay between the time that\n>> a user, say, clicks a button and the time that the button reacts. What\n>> we are talking about is the time between the release of a lock by one\n>> process and the resumption of another process that was blocked waiting\n>> for the lock. The former is probably not under the control of the user\n>> anyway, and perhaps not even observable by the user. Thus I don't think\n>> that a perceivable delay between that event and the resumption of the\n>> blocked process would be annoying. The more salient delay is between the\n>> time that the user started the blocked command and when that command\n>> completed. Let's look in more detail.\n> \n> Yeah, you can't impact when the other process will drop the lock, but if\n> we assume that it takes on the order of 100ms for the other process to\n> do its whole operation, then on average we experience half that. And\n> then tack on to that whatever time we waste in sleep() after the other\n> guy drops the lock. And that's on average half of our backoff time.\n> \n> So something like 100ms max backoff makes sense to me, in that it keeps\n> us in the same order of magnitude as the expected time that the lock is\n> held. [...]\n\nI don't understand your argument. If another process blocks us for on\nthe order of 100 ms, the backoff time (reading from my table) is less\nthan half of that. It is only if another process blocks us for longer\nthat our backoff times grow larger than 100 ms. I don't see the point of\ncomparing those larger backoff numbers to hypothetical 100 ms expected\nblocking times when the larger backoffs *can only happen* for larger\nblocking times [1].\n\nBut even aside from bikeshedding about which backoff algorithm might be\na tiny bit better than another, let's remember that these locking\nconflicts are INCREDIBLY RARE in real life. Current git doesn't have any\nretry at all, but users don't seem to be noticeably upset.\n\nIn a moment I will submit a re-roll, changing the test case to add the\n\"wait\" that Johannes suggested but leaving the maximum backoff time\nunchanged. If anybody feels strongly about changing it, go ahead and do\nso (or make it configurable). I like the current setting because I think\nit makes more sense for servers, which is the only environment where\nlock contention is likely to occur with any measurable frequency.\n\nMichael\n\n[1] For completeness, let's also consider a difference scenario: Suppose\nthe blocking is not being caused by a single long-lived process but\nrather by many short-lived processes running one after the other. In\nthat case the time we spend blocking depends more on the duty cycle of\nother blocking processes, so our backoff time could grow to be longer\nthan the mean time that any single process holds the lock. But in this\nscenario we are throughput-limited rather than latency limited, so our\nsuccess in acquiring the lock sooner only deprives another process of\nthe lock, not significantly improving the throughput of the system as a\nwhole. (And given that the other processes are probably following the\nsame rules as we are, the shorter backoff times are just as often\nhelping them snatch the lock from us as us from them.)\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"260991","messageId":"20150511164941.GA30541@peff.net","threadId":"39204","inReplyTo":"555083CF.8010205@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-11T16:49:41Z","receivedAt":"2015-05-11T16:49:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 11, 2015 at 12:26:23PM +0200, Michael Haggerty wrote:\n\n> > So something like 100ms max backoff makes sense to me, in that it keeps\n> > us in the same order of magnitude as the expected time that the lock is\n> > held. [...]\n> \n> I don't understand your argument. If another process blocks us for on\n> the order of 100 ms, the backoff time (reading from my table) is less\n> than half of that.\n\nI think it is just that I was agreeing with you, but communicated it\nbadly. I think your series is fine as-is.\n\n-Peff\n"},{"id":"260998","messageId":"CAGZ79kYc91z5S9j_hpA3b9-TvMBxr9jkdxhqEgHhOCT3mGzH5w@mail.gmail.com","threadId":"39204","inReplyTo":"20150511164941.GA30541@peff.net","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-11T17:49:09Z","receivedAt":"2015-05-11T17:49:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 11, 2015 at 9:49 AM, Jeff King wrote:\n> On Mon, May 11, 2015 at 12:26:23PM +0200, Michael Haggerty wrote:\n>\n>> > So something like 100ms max backoff makes sense to me, in that it keeps\n>> > us in the same order of magnitude as the expected time that the lock is\n>> > held. [...]\n>>\n>> I don't understand your argument. If another process blocks us for on\n>> the order of 100 ms, the backoff time (reading from my table) is less\n>> than half of that.\n>\n> I think it is just that I was agreeing with you, but communicated it\n> badly. I think your series is fine as-is.\n\nBy now I also think your series is fine as is.\nI am currently implementing something similar for Gerrit and I realize\ntesting time based things is a royal pain in the butt. The tests you propose\njust take wall clock time and all should is good, though slowing down\nthe test suite\nby another second or three in the worst case.\n\nSo for tests in Gerrit I use a dedicated java class which is\nspecialized to pretend different\ntimes, such that you can write:\n\n doThings();\n pretendTimePassing(1 second);\n checkResultsFromThreads();\n\nbut running the tests takes less than 1 second as it's no real wall\nclock time passing.\n\nOn my machine there is\n/bin/usleep - sleep some number of microseconds\nwhich may be better for running the test suite. I don't know if it's\nworth the hassle\nof checking if that program is there and selectively running a faster\nversion of said\ntest though.\n\nThat said, I'm personally fine with the waste of 1 second as I view\nrunning the test suite\nas a long waiting game anyway. Maybe Junio as the maintainer (I assume\nhe runs more tests\nthan all of us?) dislikes waiting for the second all the time?\n\n\n>\n> -Peff\n"},{"id":"260999","messageId":"CAGZ79kaR70QeX26jwR7AXSig5F_FjuT-bXSNBBaW_3Bk1vWJQQ@mail.gmail.com","threadId":"39204","inReplyTo":"CAGZ79kYc91z5S9j_hpA3b9-TvMBxr9jkdxhqEgHhOCT3mGzH5w@mail.gmail.com","subject":"Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-11T17:50:57Z","receivedAt":"2015-05-11T17:50:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 11, 2015 at 10:49 AM, Stefan Beller wrote:\n> On Mon, May 11, 2015 at 9:49 AM, Jeff King wrote:\n>> On Mon, May 11, 2015 at 12:26:23PM +0200, Michael Haggerty wrote:\n>>\n>>> > So something like 100ms max backoff makes sense to me, in that it keeps\n>>> > us in the same order of magnitude as the expected time that the lock is\n>>> > held. [...]\n>>>\n>>> I don't understand your argument. If another process blocks us for on\n>>> the order of 100 ms, the backoff time (reading from my table) is less\n>>> than half of that.\n>>\n>> I think it is just that I was agreeing with you, but communicated it\n>> badly. I think your series is fine as-is.\n>\n> By now I also think your series is fine as is.\n> I am currently implementing something similar for Gerrit and I realize\n> testing time based things is a royal pain in the butt. The tests you propose\n> just take wall clock time and all should is good, though slowing down\n> the test suite\n> by another second or three in the worst case.\n>\n> So for tests in Gerrit I use a dedicated java class which is\n> specialized to pretend different\n> times, such that you can write:\n>\n> doThings();\n> pretendTimePassing(1 second);\n> checkResultsFromThreads();\n>\n> but running the tests takes less than 1 second as it's no real wall\n> clock time passing.\n>\n> On my machine there is\n> /bin/usleep - sleep some number of microseconds\n\nAs we're using perl anyway we may even want to just do\n\n perl -e \"select(undef,undef,undef,0.1);\"\n\nas found at\nhttp://serverfault.com/questions/469247/how-do-i-sleep-for-a-millisecond-in-bash-or-ksh\n"},{"id":"261005","messageId":"xmqq38337zy1.fsf@gitster.dls.corp.google.com","threadId":"39204","inReplyTo":"1430491977-25817-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] lockfile: allow file locking to be retried with a timeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T18:04:54Z","receivedAt":"2015-05-11T18:04:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n>\n> Notes (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>\n> diff --git a/lockfile.c b/lockfile.c\n> index 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\nThis is familiar for an old timer like me. It seems help.c uses\n\"poll(NULL, 0, timeout)\" for \"do nothing but wait\" (and we have\ncompat/poll to help those who lack poll() by using select()).\n\nAs we do not need microseconds resolution here, either would be fine\n;-)\n"}]}