Re: [PATCH] config: retry acquiring config.lock for 100ms

[Junio C Hamano <gitster@pobox.com>]

Joerg Thalheim <joerg@thalheim.io> writes:

> From: Jörg Thalheim <joerg@thalheim.io>
>
> When multiple processes write to a config file concurrently, they
> contend on its ".lock" file, which is acquired via open(O_EXCL) with
> no retry. The losers fail immediately with "could not lock config
> file". Two processes writing unrelated keys (say, "branch.a.remote"
> and "branch.b.remote") have no semantic conflict, yet one of them
> fails for a purely mechanical reason.
>
> This bites in practice when running `git worktree add -b` concurrently
> against the same repository. Each invocation makes several writes to
> ".git/config" to set up branch tracking, and tooling that creates
> worktrees in parallel sees intermittent failures. Worse, `git worktree
> add` does not propagate the failed config write to its exit code: the
> worktree is created and the command exits 0, but tracking
> configuration is silently dropped.
>
> The lock is held only for the duration of rewriting a small file, so
> retrying for 100 ms papers over any realistic contention while still
> failing fast if a stale lock has been left behind by a crashed
> process. This mirrors what we already do for individual reference
> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> 2017-08-21)).
>
> Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
> ---
>  config.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

OK.  This is consistent with the default used for a ref update with
files backend.  Various subsystems use their own value randomly
chosen out of thin air, it seems.  credential-store uses 1000ms, gc
uses 150ms to interact with launchctl, refs have their own per
backend, etc.

> diff --git a/config.c b/config.c
> index 156f2a24fa..f7aff8725d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)
>  	return prepared;
>  }
>  
> +/*
> + * How long to retry acquiring config.lock when another process holds it.
> + * The lock is held only for the duration of rewriting a small file, so
> + * 100 ms covers any realistic contention while still failing fast if
> + * a stale lock has been left behind by a crashed process.
> + */
> +#define CONFIG_LOCK_TIMEOUT_MS 100
> +

Making this configurable would make a cute chicken-and-egg problem?

;-)  No, no need for that---just kidding.

Will queue.  Thanks.

> @@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
>  	 * The lock serves a purpose in addition to locking: the new
>  	 * contents of .git/config will be written into it.
>  	 */
> -	fd = hold_lock_file_for_update(&lock, config_filename, 0);
> +	fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
> +					       CONFIG_LOCK_TIMEOUT_MS);
>  	if (fd < 0) {
>  		error_errno(_("could not lock config file %s"), config_filename);
>  		ret = CONFIG_NO_LOCK;
> @@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(
>  	if (!config_filename)
>  		config_filename = filename_buf = repo_git_path(r, "config");
>  
> -	out_fd = hold_lock_file_for_update(&lock, config_filename, 0);
> +	out_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
> +						   CONFIG_LOCK_TIMEOUT_MS);
>  	if (out_fd < 0) {
>  		ret = error(_("could not lock config file %s"), config_filename);
>  		goto out;