Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock

From: Stefan Beller <sbeller@google.com>
Date: May 1, 2015, 17:51 UTC
Message-ID: <CAGZ79kZnhv+aW_GW8mBDyhfv_k54ScAFsHQz=8zfHBUJ8WrVUw@mail.gmail.com>
In-Reply-To: <1430491977-25817-3-git-send-email-mhagger@alum.mit.edu>

On Fri, May 1, 2015 at 7:52 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 80 quoted lines

> Currently, there is only one attempt to acquire any lockfile, and if
> the lock is held by another process, the locking attempt fails
> immediately.
>
> This is not such a limitation for loose reference files. First, they
> don't take long to rewrite. Second, most reference updates have a
> known "old" value, so if another process is updating a reference at
> the same moment that we are trying to lock it, then probably the
> expected "old" value will not longer be valid, and the update will
> fail anyway.
>
> But these arguments do not hold for packed-refs:
>
> * The packed-refs file can be large and take significant time to
>   rewrite.
>
> * Many references are stored in a single packed-refs file, so it could
>   be that the other process was changing a different reference than
>   the one that we are interested in.
>
> Therefore, it is much more likely for there to be spurious lock
> conflicts in connection to the packed-refs file, resulting in
> unnecessary command failures.
>
> So, if the first attempt to lock the packed-refs file fails, continue
> retrying for a configurable length of time before giving up. The
> default timeout is 1 second.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>
> Notes (discussion):
>     At first I wasn't going to make this setting configurable. After all,
>     who could object to one second?
>
>     But then I realized that making the length of the timeout configurable
>     would make it easier to test. Since sleep(1) is only guaranteed to
>     have a 1-second resolution, it is helpful to set the timeout longer
>     than 1 second in the test. So I made it configurable, and documented
>     the setting.
>
>     I don't have a strong opinion either way.
>
>     By the way, if anybody has a better idea for how to test this, please
>     chime up. As written, the two new tests each take about one second to
>     run (though they are mostly idle during that time, so they parallelize
>     well with other tests).
>
>  Documentation/config.txt |  6 ++++++
>  refs.c                   | 12 +++++++++++-
>  t/t3210-pack-refs.sh     | 17 +++++++++++++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..3c24e10 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -622,6 +622,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.
>
> +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
> +       all; -1 means to try indefinitely. Default is 1000 (i.e.,
> +       retry for 1 second).
> +
>  sequence.editor::
>         Text editor used by `git rebase -i` for editing the rebase instruction file.
>         The value is meant to be interpreted by the shell when it is used.
> diff --git a/refs.c b/refs.c
> index 47e4e53..3f8ac63 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2413,9 +2413,19 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
>  /* This should return a meaningful errno on failure */
>  int lock_packed_refs(int flags)
>  {
> +       static int timeout_configured = 0;
> +       static int timeout_value = 1000;

I'd personally be more happier with a default value of 100 ms or less The reason is found in the human nature, as humans tend to perceive anything faster than 100ms as "instant"[1], while a 100ms is a long time for computers.

Now a small default time may lead to to little retries, so maybe it's worth checking at the very end of the time again (ignoring the computed backoff times). As pushes to $server usually take a while (connecting, packing packs, writing objects etc), this may be overcautios bikeshedding from my side.

[1] http://stackoverflow.com/questions/536300/what-is-the-shortest-perceivable-application-response-delay

Show 44 quoted lines

> +
>         struct packed_ref_cache *packed_ref_cache;
>
> -       if (hold_lock_file_for_update(&packlock, git_path("packed-refs"), flags) < 0)
> +       if (!timeout_configured) {
> +               git_config_get_int("core.packedrefstimeout", &timeout_value);
> +               timeout_configured = 1;
> +       }
> +
> +       if (hold_lock_file_for_update_timeout(
> +                           &packlock, git_path("packed-refs"),
> +                           flags, timeout_value) < 0)
>                 return -1;
>         /*
>          * Get the current packed-refs while holding the lock.  If the
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index aa9eb3a..d18b726 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -187,4 +187,21 @@ test_expect_success 'notice d/f conflict with existing ref' '
>         test_must_fail git branch foo/bar/baz/lots/of/extra/components
>  '
>
> +test_expect_success 'timeout if packed-refs.lock exists' '
> +       LOCK=.git/packed-refs.lock &&
> +       >$LOCK &&
> +       test_when_finished "rm -f $LOCK" &&
> +       test_must_fail git pack-refs --all --prune
> +'
> +
> +test_expect_success 'retry acquiring packed-refs.lock' '
> +       LOCK=.git/packed-refs.lock &&
> +       >$LOCK &&
> +       test_when_finished "rm -f $LOCK" &&
> +       {
> +               ( sleep 1 ; rm -f $LOCK ) &
> +       } &&
> +       git -c core.packedrefstimeout=3000 pack-refs --all --prune
> +'
> +
>  test_done
> --
> 2.1.4
>