git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Credential Store: Don't acquire lock when only reading the file
@ 2020-10-29 19:20 Simão Afonso
  2020-10-30  5:55 ` Jeff King
  0 siblings, 1 reply; 7+ messages in thread
From: Simão Afonso @ 2020-10-29 19:20 UTC (permalink / raw)
  To: git

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Credential Store: Don't acquire lock when only reading the file
  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
  2020-10-30 11:22   ` Simão Afonso
  2020-10-30 12:05   ` Simão Afonso
  0 siblings, 2 replies; 7+ messages in thread
From: Jeff King @ 2020-10-30  5:55 UTC (permalink / raw)
  To: Simão Afonso; +Cc: git

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Credential Store: Don't acquire lock when only reading the file
  2020-10-30  5:55 ` Jeff King
@ 2020-10-30 11:22   ` Simão Afonso
  2020-10-30 11:25     ` Jeff King
  2020-10-30 12:05   ` Simão Afonso
  1 sibling, 1 reply; 7+ messages in thread
From: Simão Afonso @ 2020-10-30 11:22 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Hi Jeff, thanks for the fast reply.

On 2020-10-30 01:55:41, Jeff King wrote:
> 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.

Ah, the plot thickens. That sounds more like it.

On 2020-10-30 01:55:41, Jeff King wrote:
> 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.

Right, I did not imagine you could chain credential helpers, but that
makes sense.

On 2020-10-30 01:55:41, Jeff King wrote:
> Interested in trying a patch for any of those?

Sounds good to me, I should be able to handle it if you CC/BCC me
(subscribing to the firehose is a bit too much).

For reference, I'm using:

> $ git --version
> git version 2.29.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Credential Store: Don't acquire lock when only reading the file
  2020-10-30 11:22   ` Simão Afonso
@ 2020-10-30 11:25     ` Jeff King
  2020-10-30 11:39       ` Simão Afonso
  0 siblings, 1 reply; 7+ messages in thread
From: Jeff King @ 2020-10-30 11:25 UTC (permalink / raw)
  To: Simão Afonso; +Cc: git

On Fri, Oct 30, 2020 at 11:22:29AM +0000, Simão Afonso wrote:

> On 2020-10-30 01:55:41, Jeff King wrote:
> > Interested in trying a patch for any of those?
> 
> Sounds good to me, I should be able to handle it if you CC/BCC me
> (subscribing to the firehose is a bit too much).

I meant: would you like to try writing a patch. :)

The offer applies to anybody else, too, of course. I may work on it
eventually, but I've successfully left that series at the bottom of my
todo list for 8 years, so...

-Peff

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Credential Store: Don't acquire lock when only reading the file
  2020-10-30 11:25     ` Jeff King
@ 2020-10-30 11:39       ` Simão Afonso
  0 siblings, 0 replies; 7+ messages in thread
From: Simão Afonso @ 2020-10-30 11:39 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On 2020-10-30 07:25:24, Jeff King wrote:
> I meant: would you like to try writing a patch. :)
> 
> The offer applies to anybody else, too, of course. I may work on it
> eventually, but I've successfully left that series at the bottom of my
> todo list for 8 years, so...

Ah, that's another kettle of fish. I can try to take a look at the
simpler fixes you mentioned.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Credential Store: Don't acquire lock when only reading the file
  2020-10-30  5:55 ` Jeff King
  2020-10-30 11:22   ` Simão Afonso
@ 2020-10-30 12:05   ` Simão Afonso
  2020-10-30 15:01     ` Jeff King
  1 sibling, 1 reply; 7+ messages in thread
From: Simão Afonso @ 2020-10-30 12:05 UTC (permalink / raw)
  To: Jeff King; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On 2020-10-30 01:55:41, Jeff King wrote:
>       - 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).

So I tried to patch the credential store with that timeout function and
it seems to solve it! Thanks.

https://github.com/git/git/blob/v2.29.2/builtin/credential-store.c#L61

This is easily reproduced if you do a `fetch --all` in parallel on the
same repository (seen on the attached image):

> $ for n in $(seq 100); do git fetch --all; sleep 0.5; done

About the timeout, I put it at infinite retries, but should this be
configurable? Or should it be a different default?

There aren't many timeout configurations. I found
"core.filesRefLockTimeout" and "core.packedRefsTimeout", doesn't sound
like something that should be re-used.
Should it be something like this with a different name?

[-- Attachment #2: 2020-10-30T11:50:31+00:00.png --]
[-- Type: image/png, Size: 68623 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Credential Store: Don't acquire lock when only reading the file
  2020-10-30 12:05   ` Simão Afonso
@ 2020-10-30 15:01     ` Jeff King
  0 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2020-10-30 15:01 UTC (permalink / raw)
  To: Simão Afonso; +Cc: git

On Fri, Oct 30, 2020 at 12:05:40PM +0000, Simão Afonso wrote:

> On 2020-10-30 01:55:41, Jeff King wrote:
> >       - 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).
> 
> So I tried to patch the credential store with that timeout function and
> it seems to solve it! Thanks.
> 
> https://github.com/git/git/blob/v2.29.2/builtin/credential-store.c#L61
> 
> This is easily reproduced if you do a `fetch --all` in parallel on the
> same repository (seen on the attached image):

Great!

> > $ for n in $(seq 100); do git fetch --all; sleep 0.5; done
> 
> About the timeout, I put it at infinite retries, but should this be
> configurable? Or should it be a different default?
> 
> There aren't many timeout configurations. I found
> "core.filesRefLockTimeout" and "core.packedRefsTimeout", doesn't sound
> like something that should be re-used.
> Should it be something like this with a different name?

Yeah, I don't think it makes sense to reuse those. I kind of doubt that
it is worth making it configurable, as credential-store does not
otherwise read any config at all.

The defaults for those other two values are 100ms and 1s respectively.
So probably just setting a 1s timeout would be plenty, and saves us from
looping forever if we see a stale lockfile.

-Peff

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-30 15:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git