{"thread":{"id":"54539","subject":"[PATCH] credential-store: use timeout when locking file","startedAt":"2020-10-30T18:07:27Z","lastAt":"2020-11-26T07:38:12Z","messageCount":7,"participants":["Simão Afonso","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408766","messageId":"20201030180718.4i7txqkgye7r6pkb@safonso-t430","threadId":"54539","inReplyTo":null,"subject":"[PATCH] credential-store: use timeout when locking file","fromName":"Simão Afonso","fromEmail":"simao.afonso@powertools-tech.com","sentAt":"2020-10-30T18:07:18Z","receivedAt":"2020-10-30T18:07:27Z","isPatch":true,"sender":{"key":"simao.afonso@powertools-tech.com","avatar":"https://avatars.githubusercontent.com/u/49308688?v=4"},"body":"When holding the lock for rewriting the credential file, use a timeout\nto avoid race conditions when the credentials file needs to be updated\nin parallel.\n\nAn example would be doing `fetch --all` on a repository with several\nremotes that need credentials, using parallel fetching.\n\nThe timeout is hardcoded to 1 second, since this is just to solve a race\ncondition.\n\nThis was reported here:\nhttps://lore.kernel.org/git/20201029192020.mcri76ylbdure2o7@safonso-t430/\n---\n builtin/credential-store.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 5331ab151..acff4abae 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -58,8 +58,9 @@ static void print_line(struct strbuf *buf)\n static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\t\t\t    struct strbuf *extra)\n {\n-\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n-\t\tdie_errno(\"unable to get credential storage lock\");\n+\tstatic long timeout = 1000;\n+\tif (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout) < 0)\n+\t\tdie_errno(\"unable to get credential storage lock in %ld ms\", timeout);\n \tif (extra)\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n-- \n2.29.1\n\n"},{"id":"408769","messageId":"xmqq361v334r.fsf@gitster.c.googlers.com","threadId":"54539","inReplyTo":"20201030180718.4i7txqkgye7r6pkb@safonso-t430","subject":"Re: [PATCH] credential-store: use timeout when locking file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-30T19:49:56Z","receivedAt":"2020-10-30T19:50:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simão Afonso <simao.afonso@powertools-tech.com> writes:\n\n> When holding the lock for rewriting the credential file, use a timeout\n> to avoid race conditions when the credentials file needs to be updated\n> in parallel.\n>\n> An example would be doing `fetch --all` on a repository with several\n> remotes that need credentials, using parallel fetching.\n\nOK.\n\n> The timeout is hardcoded to 1 second, since this is just to solve a race\n> condition.\n\nIt is unclear what this sentence wants to explain.  How does \"this\nis to solve a race\" justifies the choice of \"1 second\" (as opposed\nto say 10 seconds or 0.5 second)?  Or is this trying to justify why\nthere is no configurability?  If so, why is it OK to hardcode it if\nit is done to solve a race?  Are we assuming certain maximum rate\nof operation that is \"reasonable\"?\n\n> This was reported here:\n> https://lore.kernel.org/git/20201029192020.mcri76ylbdure2o7@safonso-t430/\n> ---\n\nMissing sign-off.\n\ncf. https://git-scm.com/docs/SubmittingPatches.html#sign-off\n\n>  builtin/credential-store.c | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index 5331ab151..acff4abae 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -58,8 +58,9 @@ static void print_line(struct strbuf *buf)\n>  static void rewrite_credential_file(const char *fn, struct credential *c,\n>  \t\t\t\t    struct strbuf *extra)\n>  {\n> -\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n> -\t\tdie_errno(\"unable to get credential storage lock\");\n> +\tstatic long timeout = 1000;\n\nWhy \"static\"?  It would make your code easier to follow if you limit\nuse of \"static\" to only cases where you want to take advantage of\nthe fact that the value previously left by the earlier call is seen\nby the next call, and/or the address of the variable must be valid\neven after the control returns to the caller.\n\nI would understand if this were \"const long timeout = 1000\".\n\nIf this were an identifier with longer lifespan, I would have\nsuggested to include some scale in the variable name (e.g.\ntimeout_ms to clarify that it is counted in milliseconds), but it is\njust for this short function, so let's say \"timeout\" is just fine.\n\n> +\tif (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout) < 0)\n> +\t\tdie_errno(\"unable to get credential storage lock in %ld ms\", timeout);\n>  \tif (extra)\n>  \t\tprint_line(extra);\n>  \tparse_credential_file(fn, c, NULL, print_line);\n\nThanks.\n"},{"id":"410694","messageId":"20201124193208.74fe4mticgkbxou3@safonso-t430","threadId":"54539","inReplyTo":"xmqq361v334r.fsf@gitster.c.googlers.com","subject":"[PATCH v2] crendential-store: use timeout when locking file","fromName":"Simão Afonso","fromEmail":"simao.afonso@powertools-tech.com","sentAt":"2020-11-24T19:32:08Z","receivedAt":"2020-11-24T19:32:15Z","isPatch":true,"sender":{"key":"simao.afonso@powertools-tech.com","avatar":"https://avatars.githubusercontent.com/u/49308688?v=4"},"body":"When holding the lock for rewriting the credential file, use a timeout\nto avoid race conditions when the credentials file needs to be updated\nin parallel.\n\nAn example would be doing `fetch --all` on a repository with several\nremotes that need credentials, using parallel fetching.\n\nThe timeout can be configured using \"credentialStore.fileTimeout\",\ndefaulting to 1 second.\n\nSigned-off-by: Simão Afonso <simao.afonso@powertools-tech.com>\n---\n\nThanks for the review.\nI got stuck with work and only got around to tweak this now.\n\nI added a configurable timeout with the old value as default. I think\nthat txt file is the only documentation that requires update for a new\nconfiguration value.\n\n Documentation/config/credential.txt | 6 ++++++\n builtin/credential-store.c          | 8 ++++++--\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt\nindex 9d01641c2..132e04b47 100644\n--- a/Documentation/config/credential.txt\n+++ b/Documentation/config/credential.txt\n@@ -28,3 +28,9 @@ credential.<url>.*::\n \n credentialCache.ignoreSIGHUP::\n \tTell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.\n+\n+credentialStore.fileTimeout::\n+\tThe length of time, in milliseconds, for git-credential-store to retry\n+\twhen trying to lock the credentials file. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 1000 (i.e., retry for\n+\t1s).\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 5331ab151..82284176e 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -1,4 +1,5 @@\n #include \"builtin.h\"\n+#include \"config.h\"\n #include \"lockfile.h\"\n #include \"credential.h\"\n #include \"string-list.h\"\n@@ -58,8 +59,11 @@ static void print_line(struct strbuf *buf)\n static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\t\t\t    struct strbuf *extra)\n {\n-\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n-\t\tdie_errno(\"unable to get credential storage lock\");\n+\tint timeout_ms = 1000;\n+\tgit_config_get_int(\"credentialstore.filetimeout\", &timeout_ms);\n+\n+\tif (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)\n+\t\tdie_errno(\"unable to get credential storage lock in %d ms\", timeout_ms);\n \tif (extra)\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n-- \n2.29.2\n\n"},{"id":"410722","messageId":"xmqq4kleif26.fsf@gitster.c.googlers.com","threadId":"54539","inReplyTo":"20201124193208.74fe4mticgkbxou3@safonso-t430","subject":"Re: [PATCH v2] crendential-store: use timeout when locking file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-24T22:08:01Z","receivedAt":"2020-11-24T22:08:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simão Afonso <simao.afonso@powertools-tech.com> writes:\n\n> +credentialStore.fileTimeout::\n> +\tThe length of time, in milliseconds, for git-credential-store to retry\n> +\twhen trying to lock the credentials file. Value 0 means not to retry at\n> +\tall; -1 means to try indefinitely. Default is 1000 (i.e., retry for\n> +\t1s).\n\nI do not remember what was said in the first round of the review,\nbut I wonder if this is the best name for users.  I think it is good\nenough, but do \".lockTimeout\" or \".lockTimeoutMS\" make it even\neasier to grok, perhaps?\n\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index 5331ab151..82284176e 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -1,4 +1,5 @@\n>  #include \"builtin.h\"\n> +#include \"config.h\"\n>  #include \"lockfile.h\"\n>  #include \"credential.h\"\n>  #include \"string-list.h\"\n> @@ -58,8 +59,11 @@ static void print_line(struct strbuf *buf)\n>  static void rewrite_credential_file(const char *fn, struct credential *c,\n>  \t\t\t\t    struct strbuf *extra)\n>  {\n> -\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n> -\t\tdie_errno(\"unable to get credential storage lock\");\n> +\tint timeout_ms = 1000;\n> +\tgit_config_get_int(\"credentialstore.filetimeout\", &timeout_ms);\n\nPlease have a blank line before the first statement.\n\n> +\n> +\tif (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)\n> +\t\tdie_errno(\"unable to get credential storage lock in %d ms\", timeout_ms);\n\nShould this be die_errno()?  Looking at lock_file_timeout(), I am\nnot sure if the value of errno is valid in all codepaths that return\nfailure.\n\nIn any case, the message should be markd with _() for translation.\n\nOther than that, it looks good.\n\nThanks.\n\n>  \tif (extra)\n>  \t\tprint_line(extra);\n>  \tparse_credential_file(fn, c, NULL, print_line);\n\n"},{"id":"410777","messageId":"X74lw+fyurzqVbg1@coredump.intra.peff.net","threadId":"54539","inReplyTo":"xmqq4kleif26.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] crendential-store: use timeout when locking file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-25T09:37:07Z","receivedAt":"2020-11-25T09:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 24, 2020 at 02:08:01PM -0800, Junio C Hamano wrote:\n\n> Simão Afonso <simao.afonso@powertools-tech.com> writes:\n> \n> > +credentialStore.fileTimeout::\n> > +\tThe length of time, in milliseconds, for git-credential-store to retry\n> > +\twhen trying to lock the credentials file. Value 0 means not to retry at\n> > +\tall; -1 means to try indefinitely. Default is 1000 (i.e., retry for\n> > +\t1s).\n> \n> I do not remember what was said in the first round of the review,\n> but I wonder if this is the best name for users.  I think it is good\n> enough, but do \".lockTimeout\" or \".lockTimeoutMS\" make it even\n> easier to grok, perhaps?\n\nYeah, I think those are a bit more obvious.\n\n> > +\n> > +\tif (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)\n> > +\t\tdie_errno(\"unable to get credential storage lock in %d ms\", timeout_ms);\n> \n> Should this be die_errno()?  Looking at lock_file_timeout(), I am\n> not sure if the value of errno is valid in all codepaths that return\n> failure.\n\nI think it's the right thing here. Inside hold_lock_file_for_update_timeout(),\nwe'd pass errno to unable_to_lock_die(), etc. So if there is a code path\nin lock_file_timeout() that isn't setting errno properly, we should\nprobably be fixing that.\n\nAnother option would be to just pass LOCK_DIE_ON_ERROR here, but I think\nfor this use I prefer the smaller \"unable to lock\" to the big \"another\ngit process may have crashed\" advice message we'd give in that case.\n\n-Peff\n"},{"id":"410791","messageId":"20201125183123.47praa6hcgrgrib4@safonso-t430","threadId":"54539","inReplyTo":"xmqq4kleif26.fsf@gitster.c.googlers.com","subject":"[PATCH v3] crendential-store: use timeout when locking file","fromName":"Simão Afonso","fromEmail":"simao.afonso@powertools-tech.com","sentAt":"2020-11-25T18:31:23Z","receivedAt":"2020-11-25T18:31:43Z","isPatch":true,"sender":{"key":"simao.afonso@powertools-tech.com","avatar":"https://avatars.githubusercontent.com/u/49308688?v=4"},"body":"When holding the lock for rewriting the credential file, use a timeout\nto avoid race conditions when the credentials file needs to be updated\nin parallel.\n\nAn example would be doing `fetch --all` on a repository with several\nremotes that need credentials, using parallel fetching.\n\nThe timeout can be configured using \"credentialStore.lockTimeoutMS\",\ndefaulting to 1 second.\n\nSigned-off-by: Simão Afonso <simao.afonso@powertools-tech.com>\n---\n\nlockTimeoutMS sounds like the consensual name, implemented.\n\n Documentation/config/credential.txt | 6 ++++++\n builtin/credential-store.c          | 8 ++++++--\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt\nindex 9d01641c2..512f31876 100644\n--- a/Documentation/config/credential.txt\n+++ b/Documentation/config/credential.txt\n@@ -28,3 +28,9 @@ credential.<url>.*::\n \n credentialCache.ignoreSIGHUP::\n \tTell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.\n+\n+credentialStore.lockTimeoutMS::\n+\tThe length of time, in milliseconds, for git-credential-store to retry\n+\twhen trying to lock the credentials file. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 1000 (i.e., retry for\n+\t1s).\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 5331ab151..ae3c1ba75 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -1,4 +1,5 @@\n #include \"builtin.h\"\n+#include \"config.h\"\n #include \"lockfile.h\"\n #include \"credential.h\"\n #include \"string-list.h\"\n@@ -58,8 +59,11 @@ static void print_line(struct strbuf *buf)\n static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\t\t\t    struct strbuf *extra)\n {\n-\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n-\t\tdie_errno(\"unable to get credential storage lock\");\n+\tint timeout_ms = 1000;\n+\n+\tgit_config_get_int(\"credentialstore.locktimeoutms\", &timeout_ms);\n+\tif (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)\n+\t\tdie_errno(_(\"unable to get credential storage lock in %d ms\"), timeout_ms);\n \tif (extra)\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n-- \n2.29.2\n\n"},{"id":"410868","messageId":"X79bUrreRkpiiorU@coredump.intra.peff.net","threadId":"54539","inReplyTo":"20201125183123.47praa6hcgrgrib4@safonso-t430","subject":"Re: [PATCH v3] crendential-store: use timeout when locking file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-26T07:37:54Z","receivedAt":"2020-11-26T07:38:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2020 at 06:31:23PM +0000, Simão Afonso wrote:\n\n> When holding the lock for rewriting the credential file, use a timeout\n> to avoid race conditions when the credentials file needs to be updated\n> in parallel.\n> \n> An example would be doing `fetch --all` on a repository with several\n> remotes that need credentials, using parallel fetching.\n> \n> The timeout can be configured using \"credentialStore.lockTimeoutMS\",\n> defaulting to 1 second.\n> \n> Signed-off-by: Simão Afonso <simao.afonso@powertools-tech.com>\n> ---\n> \n> lockTimeoutMS sounds like the consensual name, implemented.\n\nThanks, this version looks good to me.\n\n-Peff\n"}]}