Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout

Joerg Thalheim <[email protected]> writes:

> From: Jörg Thalheim <[email protected]>
>
> Concurrent config writers race for the ".lock" file, which is taken
> with open(O_EXCL) and no retry, so the losers fail right away with
> "could not lock config file".
>
> This shows up with parallel "git worktree add -b" against the same
> repository: each one writes a couple of branch.* keys and the losers
> fail at random. Worse, "git worktree add" doesn't propagate that
> failure to its exit code, so the tracking config is silently dropped.
> (The swallowed error is a separate bug.)
>
> Retry instead of giving up on the first EEXIST. The lock is only held
> while rewriting a small file, so the loser only has to wait out the
> other writers. Same approach as 4ff0f01cb7 (refs: retry acquiring
> reference locks for 100ms, 2017-08-21).
>
> On the semantics: the on-disk config is read only after the lock is
> taken, so writers touching different keys can't lose each other's
> change. Writers touching the same key still get last-writer-wins, but
> that is already the case today and would need a compare-and-swap config
> API to fix. The retry only turns hard failures into successes.
>
> Default to 1000ms, like core.packedRefsTimeout: same shape of problem,
> one shared file everyone serializes through. A larger timeout only
> costs anything when a stale lock is left behind by a crash, which is
> rare; a smaller one fails spuriously on slow filesystems (NTFS has
> been seen needing more than 100ms). Make it configurable as
> core.configLockTimeout. There is no chicken-and-egg problem: we read
> the config before we lock it.
>
> microsoft/git carries a similar patch (core.configWriteLockTimeoutMS,
> default off) for Scalar's tests. Defaulting to non-zero here because
> the worktree case fails silently.
>
> Helped-by: Patrick Steinhardt <[email protected]>
> Helped-by: Johannes Schindelin <[email protected]>
> Signed-off-by: Jörg Thalheim <[email protected]>
> ---
> Thanks for the review and for poking me, this had fallen off my radar.
>
> v1 -> v2:
>
> - added core.configLockTimeout. Johannes is right that there is no
>   chicken-and-egg problem (config is read before the lock), so no env
>   var needed.
> - default bumped to 1000ms; packed-refs is the closer precedent and it
>   keeps NTFS out of trouble.
> - commit message now covers the read-after-lock / last-writer-wins
>   semantics Patrick asked about.
> - added tests; existing stale-lock tests in t3200/t5505 now pass
>   -c core.configLockTimeout=0 so they still fail fast.
>
> I matched the core.filesRefLockTimeout naming rather than reusing
> microsoft/git's core.configWriteLockTimeoutMS, but can switch if the
> downstream compat matters more.

I was reviewing the whats-cooking and noticed there are a handful of
stalled topics that are not going anywhere, and this is one of them.

This time it had fallen off my radar, sorry about that.    All the
outstanding issues seem to have been resolved, so let's merge it
down to 'next'.

Thanks.