git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] credential-store: use timeout when locking file
@ 2020-10-30 18:07 Simão Afonso
  2020-10-30 19:49 ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Simão Afonso @ 2020-10-30 18:07 UTC (permalink / raw)
  To: git; +Cc: Jeff King

When holding the lock for rewriting the credential file, use a timeout
to avoid race conditions when the credentials file needs to be updated
in parallel.

An example would be doing `fetch --all` on a repository with several
remotes that need credentials, using parallel fetching.

The timeout is hardcoded to 1 second, since this is just to solve a race
condition.

This was reported here:
https://lore.kernel.org/git/20201029192020.mcri76ylbdure2o7@safonso-t430/
---
 builtin/credential-store.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index 5331ab151..acff4abae 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -58,8 +58,9 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
-	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
-		die_errno("unable to get credential storage lock");
+	static long timeout = 1000;
+	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout) < 0)
+		die_errno("unable to get credential storage lock in %ld ms", timeout);
 	if (extra)
 		print_line(extra);
 	parse_credential_file(fn, c, NULL, print_line);
-- 
2.29.1


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

* Re: [PATCH] credential-store: use timeout when locking file
  2020-10-30 18:07 [PATCH] credential-store: use timeout when locking file Simão Afonso
@ 2020-10-30 19:49 ` Junio C Hamano
  2020-11-24 19:32   ` [PATCH v2] crendential-store: " Simão Afonso
  0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2020-10-30 19:49 UTC (permalink / raw)
  To: Simão Afonso; +Cc: git, Jeff King

Simão Afonso <simao.afonso@powertools-tech.com> writes:

> When holding the lock for rewriting the credential file, use a timeout
> to avoid race conditions when the credentials file needs to be updated
> in parallel.
>
> An example would be doing `fetch --all` on a repository with several
> remotes that need credentials, using parallel fetching.

OK.

> The timeout is hardcoded to 1 second, since this is just to solve a race
> condition.

It is unclear what this sentence wants to explain.  How does "this
is to solve a race" justifies the choice of "1 second" (as opposed
to say 10 seconds or 0.5 second)?  Or is this trying to justify why
there is no configurability?  If so, why is it OK to hardcode it if
it is done to solve a race?  Are we assuming certain maximum rate
of operation that is "reasonable"?

> This was reported here:
> https://lore.kernel.org/git/20201029192020.mcri76ylbdure2o7@safonso-t430/
> ---

Missing sign-off.

cf. https://git-scm.com/docs/SubmittingPatches.html#sign-off

>  builtin/credential-store.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index 5331ab151..acff4abae 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -58,8 +58,9 @@ static void print_line(struct strbuf *buf)
>  static void rewrite_credential_file(const char *fn, struct credential *c,
>  				    struct strbuf *extra)
>  {
> -	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
> -		die_errno("unable to get credential storage lock");
> +	static long timeout = 1000;

Why "static"?  It would make your code easier to follow if you limit
use of "static" to only cases where you want to take advantage of
the fact that the value previously left by the earlier call is seen
by the next call, and/or the address of the variable must be valid
even after the control returns to the caller.

I would understand if this were "const long timeout = 1000".

If this were an identifier with longer lifespan, I would have
suggested to include some scale in the variable name (e.g.
timeout_ms to clarify that it is counted in milliseconds), but it is
just for this short function, so let's say "timeout" is just fine.

> +	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout) < 0)
> +		die_errno("unable to get credential storage lock in %ld ms", timeout);
>  	if (extra)
>  		print_line(extra);
>  	parse_credential_file(fn, c, NULL, print_line);

Thanks.

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

* [PATCH v2] crendential-store: use timeout when locking file
  2020-10-30 19:49 ` Junio C Hamano
@ 2020-11-24 19:32   ` Simão Afonso
  2020-11-24 22:08     ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Simão Afonso @ 2020-11-24 19:32 UTC (permalink / raw)
  To: git; +Cc: gitster, Jeff King

When holding the lock for rewriting the credential file, use a timeout
to avoid race conditions when the credentials file needs to be updated
in parallel.

An example would be doing `fetch --all` on a repository with several
remotes that need credentials, using parallel fetching.

The timeout can be configured using "credentialStore.fileTimeout",
defaulting to 1 second.

Signed-off-by: Simão Afonso <simao.afonso@powertools-tech.com>
---

Thanks for the review.
I got stuck with work and only got around to tweak this now.

I added a configurable timeout with the old value as default. I think
that txt file is the only documentation that requires update for a new
configuration value.

 Documentation/config/credential.txt | 6 ++++++
 builtin/credential-store.c          | 8 ++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt
index 9d01641c2..132e04b47 100644
--- a/Documentation/config/credential.txt
+++ b/Documentation/config/credential.txt
@@ -28,3 +28,9 @@ credential.<url>.*::
 
 credentialCache.ignoreSIGHUP::
 	Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
+
+credentialStore.fileTimeout::
+	The length of time, in milliseconds, for git-credential-store to retry
+	when trying to lock the credentials file. Value 0 means not to retry at
+	all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
+	1s).
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index 5331ab151..82284176e 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "config.h"
 #include "lockfile.h"
 #include "credential.h"
 #include "string-list.h"
@@ -58,8 +59,11 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
-	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
-		die_errno("unable to get credential storage lock");
+	int timeout_ms = 1000;
+	git_config_get_int("credentialstore.filetimeout", &timeout_ms);
+
+	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
+		die_errno("unable to get credential storage lock in %d ms", timeout_ms);
 	if (extra)
 		print_line(extra);
 	parse_credential_file(fn, c, NULL, print_line);
-- 
2.29.2


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

* Re: [PATCH v2] crendential-store: use timeout when locking file
  2020-11-24 19:32   ` [PATCH v2] crendential-store: " Simão Afonso
@ 2020-11-24 22:08     ` Junio C Hamano
  2020-11-25  9:37       ` Jeff King
  2020-11-25 18:31       ` [PATCH v3] " Simão Afonso
  0 siblings, 2 replies; 7+ messages in thread
From: Junio C Hamano @ 2020-11-24 22:08 UTC (permalink / raw)
  To: Simão Afonso; +Cc: git, Jeff King

Simão Afonso <simao.afonso@powertools-tech.com> writes:

> +credentialStore.fileTimeout::
> +	The length of time, in milliseconds, for git-credential-store to retry
> +	when trying to lock the credentials file. Value 0 means not to retry at
> +	all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
> +	1s).

I do not remember what was said in the first round of the review,
but I wonder if this is the best name for users.  I think it is good
enough, but do ".lockTimeout" or ".lockTimeoutMS" make it even
easier to grok, perhaps?

> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index 5331ab151..82284176e 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -1,4 +1,5 @@
>  #include "builtin.h"
> +#include "config.h"
>  #include "lockfile.h"
>  #include "credential.h"
>  #include "string-list.h"
> @@ -58,8 +59,11 @@ static void print_line(struct strbuf *buf)
>  static void rewrite_credential_file(const char *fn, struct credential *c,
>  				    struct strbuf *extra)
>  {
> -	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
> -		die_errno("unable to get credential storage lock");
> +	int timeout_ms = 1000;
> +	git_config_get_int("credentialstore.filetimeout", &timeout_ms);

Please have a blank line before the first statement.

> +
> +	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
> +		die_errno("unable to get credential storage lock in %d ms", timeout_ms);

Should this be die_errno()?  Looking at lock_file_timeout(), I am
not sure if the value of errno is valid in all codepaths that return
failure.

In any case, the message should be markd with _() for translation.

Other than that, it looks good.

Thanks.

>  	if (extra)
>  		print_line(extra);
>  	parse_credential_file(fn, c, NULL, print_line);


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

* Re: [PATCH v2] crendential-store: use timeout when locking file
  2020-11-24 22:08     ` Junio C Hamano
@ 2020-11-25  9:37       ` Jeff King
  2020-11-25 18:31       ` [PATCH v3] " Simão Afonso
  1 sibling, 0 replies; 7+ messages in thread
From: Jeff King @ 2020-11-25  9:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Simão Afonso, git

On Tue, Nov 24, 2020 at 02:08:01PM -0800, Junio C Hamano wrote:

> Simão Afonso <simao.afonso@powertools-tech.com> writes:
> 
> > +credentialStore.fileTimeout::
> > +	The length of time, in milliseconds, for git-credential-store to retry
> > +	when trying to lock the credentials file. Value 0 means not to retry at
> > +	all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
> > +	1s).
> 
> I do not remember what was said in the first round of the review,
> but I wonder if this is the best name for users.  I think it is good
> enough, but do ".lockTimeout" or ".lockTimeoutMS" make it even
> easier to grok, perhaps?

Yeah, I think those are a bit more obvious.

> > +
> > +	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
> > +		die_errno("unable to get credential storage lock in %d ms", timeout_ms);
> 
> Should this be die_errno()?  Looking at lock_file_timeout(), I am
> not sure if the value of errno is valid in all codepaths that return
> failure.

I think it's the right thing here. Inside hold_lock_file_for_update_timeout(),
we'd pass errno to unable_to_lock_die(), etc. So if there is a code path
in lock_file_timeout() that isn't setting errno properly, we should
probably be fixing that.

Another option would be to just pass LOCK_DIE_ON_ERROR here, but I think
for this use I prefer the smaller "unable to lock" to the big "another
git process may have crashed" advice message we'd give in that case.

-Peff

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

* [PATCH v3] crendential-store: use timeout when locking file
  2020-11-24 22:08     ` Junio C Hamano
  2020-11-25  9:37       ` Jeff King
@ 2020-11-25 18:31       ` Simão Afonso
  2020-11-26  7:37         ` Jeff King
  1 sibling, 1 reply; 7+ messages in thread
From: Simão Afonso @ 2020-11-25 18:31 UTC (permalink / raw)
  To: git; +Cc: gitster, Jeff King

When holding the lock for rewriting the credential file, use a timeout
to avoid race conditions when the credentials file needs to be updated
in parallel.

An example would be doing `fetch --all` on a repository with several
remotes that need credentials, using parallel fetching.

The timeout can be configured using "credentialStore.lockTimeoutMS",
defaulting to 1 second.

Signed-off-by: Simão Afonso <simao.afonso@powertools-tech.com>
---

lockTimeoutMS sounds like the consensual name, implemented.

 Documentation/config/credential.txt | 6 ++++++
 builtin/credential-store.c          | 8 ++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt
index 9d01641c2..512f31876 100644
--- a/Documentation/config/credential.txt
+++ b/Documentation/config/credential.txt
@@ -28,3 +28,9 @@ credential.<url>.*::
 
 credentialCache.ignoreSIGHUP::
 	Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
+
+credentialStore.lockTimeoutMS::
+	The length of time, in milliseconds, for git-credential-store to retry
+	when trying to lock the credentials file. Value 0 means not to retry at
+	all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
+	1s).
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index 5331ab151..ae3c1ba75 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "config.h"
 #include "lockfile.h"
 #include "credential.h"
 #include "string-list.h"
@@ -58,8 +59,11 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
-	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
-		die_errno("unable to get credential storage lock");
+	int timeout_ms = 1000;
+
+	git_config_get_int("credentialstore.locktimeoutms", &timeout_ms);
+	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
+		die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
 	if (extra)
 		print_line(extra);
 	parse_credential_file(fn, c, NULL, print_line);
-- 
2.29.2


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

* Re: [PATCH v3] crendential-store: use timeout when locking file
  2020-11-25 18:31       ` [PATCH v3] " Simão Afonso
@ 2020-11-26  7:37         ` Jeff King
  0 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2020-11-26  7:37 UTC (permalink / raw)
  To: Simão Afonso; +Cc: git, gitster

On Wed, Nov 25, 2020 at 06:31:23PM +0000, Simão Afonso wrote:

> When holding the lock for rewriting the credential file, use a timeout
> to avoid race conditions when the credentials file needs to be updated
> in parallel.
> 
> An example would be doing `fetch --all` on a repository with several
> remotes that need credentials, using parallel fetching.
> 
> The timeout can be configured using "credentialStore.lockTimeoutMS",
> defaulting to 1 second.
> 
> Signed-off-by: Simão Afonso <simao.afonso@powertools-tech.com>
> ---
> 
> lockTimeoutMS sounds like the consensual name, implemented.

Thanks, this version looks good to me.

-Peff

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

end of thread, other threads:[~2020-11-26  7:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-30 18:07 [PATCH] credential-store: use timeout when locking file Simão Afonso
2020-10-30 19:49 ` Junio C Hamano
2020-11-24 19:32   ` [PATCH v2] crendential-store: " Simão Afonso
2020-11-24 22:08     ` Junio C Hamano
2020-11-25  9:37       ` Jeff King
2020-11-25 18:31       ` [PATCH v3] " Simão Afonso
2020-11-26  7:37         ` 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