git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Simão Afonso" <simao.afonso@powertools-tech.com>
Cc: git@vger.kernel.org
Subject: Re: Credential Store: Don't acquire lock when only reading the file
Date: Fri, 30 Oct 2020 01:55:41 -0400	[thread overview]
Message-ID: <20201030055541.GA3264588@coredump.intra.peff.net> (raw)
In-Reply-To: <20201029192020.mcri76ylbdure2o7@safonso-t430>

On Thu, Oct 29, 2020 at 07:20:20PM +0000, Simão Afonso wrote:

> When git is configured for parallel fetches `fetch.parallel = 0` and
> uses the `store` credential helper, fetching all remotes might lead to a
> spurious lock fail. It's a race condition when several remotes require
> the credentials at the same time.
> 
> This shouldn't happen, using the `get` operation should not lock the
> file.

I agree that "get" should not be taking a lock. And looking at the code,
I don't think that it is.

However, after successfully using a password, Git will then trigger each
helper with a "store" command. So likely what you are seeing is each
fetch telling credential-store to store the successful password.

There are a few options here:

  - we could have Git not do that. And indeed, I wrote a patch for that
    long ago:

      https://lore.kernel.org/git/20120407033417.GA13914@sigill.intra.peff.net/

    but it turns out that some people rely on it. There were some
    options discussed there for moving it forward, but nothing ever
    happened.

    Another option that we didn't discuss there: we could remember which
    helper gave us the credential and not feed it back to itself. That
    would let simple cases work without the extra request, but would let
    more complex ones (feeding the result of one helper to another)
    continue to work the same.

  - the problem with `store` is that it has unfriendly concurrency
    semantics. Only one instance can hold the lock at a time, and
    clients which see that the lock is held just fail immediately, even
    though they could probably succeed by waiting a few milliseconds.
    Whereas other helpers are likely a bit smarter about this. So if we
    just wanted to improve credential-store, some other options are:

      - teach it to try the lock until hitting a timeout. I think just
	swapping out hold_lock_file_for_update() for
	hold_lock_file_for_update_timeout() would do it (I probably
	would have used it back then, but it didn't exist yet).

      - teach it to check whether the requested update is a noop
	(because we already have the identical entry in the file)
	without holding the lock. This implies an extra pass over the
	file when we _do_ write something, but the noop case is clearly
	going to be the more common one (and will also be faster,
	because we won't do any writing at all).

	I think I do prefer handling this inside Git, though (i.e.,
	having it realize it would be a noop and avoid calling the
	helper at all).

Interested in trying a patch for any of those?

-Peff

  reply	other threads:[~2020-10-30  5:55 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29 19:20 Credential Store: Don't acquire lock when only reading the file Simão Afonso
2020-10-30  5:55 ` Jeff King [this message]
2020-10-30 11:22   ` Simão Afonso
2020-10-30 11:25     ` Jeff King
2020-10-30 11:39       ` Simão Afonso
2020-10-30 12:05   ` Simão Afonso
2020-10-30 15:01     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201030055541.GA3264588@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=simao.afonso@powertools-tech.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).