{"thread":{"id":"54007","subject":"[PATCH] refs: fix interleaving hook calls with reference-transaction hook","startedAt":"2020-08-07T07:06:06Z","lastAt":"2020-08-07T19:26:23Z","messageCount":9,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403119","messageId":"63fb363375b515b903ed1269d10124b727c1d1cc.1596783732.git.ps@pks.im","threadId":"54007","inReplyTo":null,"subject":"[PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-07T07:05:58Z","receivedAt":"2020-08-07T07:06:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to not repeatedly search for the reference-transaction hook in\ncase it's getting called multiple times, we use a caching mechanism to\nonly call `find_hook()` once. What was missed though is that the return\nvalue of `find_hook()` actually comes from a static strbuf, which means\nit will get overwritten when calling `find_hook()` again. As a result,\nwe may call the wrong hook with parameters of the reference-transaction\nhook.\n\nThis scenario was spotted in the wild when executing a git-push(1) with\nmultiple references, where there are interleaving calls to both the\nupdate and the reference-transaction hook. While initial calls to the\nreference-transaction hook work as expected, it will stop working after\nthe next invocation of the update hook. The result is that we now start\ncalling the update hook with parameters and stdin of the\nreference-transaction hook.\n\nThis commit fixes the issue by storing a copy of `find_hook()`'s return\nvalue in the cache.\n---\n refs.c                           |  2 +-\n t/t1416-ref-transaction-hooks.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2dd851fe81..17e515b288 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2044,7 +2044,7 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \tif (hook == &hook_not_found)\n \t\treturn ret;\n \tif (!hook)\n-\t\thook = find_hook(\"reference-transaction\");\n+\t\thook = xstrdup_or_null(find_hook(\"reference-transaction\"));\n \tif (!hook) {\n \t\thook = &hook_not_found;\n \t\treturn ret;\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex da58d867a5..d4d19194bf 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -106,4 +106,30 @@ test_expect_success 'hook gets all queued updates in aborted state' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'interleaving hook calls succeed' '\n+\ttest_when_finished \"rm -r target-repo.git\" &&\n+\n+\tgit init --bare target-repo.git &&\n+\n+\twrite_script target-repo.git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo $0 \"$@\" >>actual\n+\tEOF\n+\n+\twrite_script target-repo.git/hooks/update <<-\\EOF &&\n+\t\techo $0 \"$@\" >>actual\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t\thooks/update refs/tags/PRE 0000000000000000000000000000000000000000 63ac8e7bcdb882293465435909f54a96de17d4f7\n+\t\thooks/reference-transaction prepared\n+\t\thooks/reference-transaction committed\n+\t\thooks/update refs/tags/POST 0000000000000000000000000000000000000000 99d53161c3a0a903b6561b9f6c0c665b3a476401\n+\t\thooks/reference-transaction prepared\n+\t\thooks/reference-transaction committed\n+\tEOF\n+\n+\tgit push ./target-repo.git PRE POST &&\n+\ttest_cmp expect target-repo.git/actual\n+'\n+\n test_done\n-- \n2.28.0\n\n"},{"id":"403121","messageId":"20200807075837.GA32344@coredump.intra.peff.net","threadId":"54007","inReplyTo":"63fb363375b515b903ed1269d10124b727c1d1cc.1596783732.git.ps@pks.im","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T07:58:37Z","receivedAt":"2020-08-07T07:58:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 09:05:58AM +0200, Patrick Steinhardt wrote:\n\n> In order to not repeatedly search for the reference-transaction hook in\n> case it's getting called multiple times, we use a caching mechanism to\n> only call `find_hook()` once. What was missed though is that the return\n> value of `find_hook()` actually comes from a static strbuf, which means\n> it will get overwritten when calling `find_hook()` again. As a result,\n> we may call the wrong hook with parameters of the reference-transaction\n> hook.\n> \n> This scenario was spotted in the wild when executing a git-push(1) with\n> multiple references, where there are interleaving calls to both the\n> update and the reference-transaction hook. While initial calls to the\n> reference-transaction hook work as expected, it will stop working after\n> the next invocation of the update hook. The result is that we now start\n> calling the update hook with parameters and stdin of the\n> reference-transaction hook.\n\nThat makes sense. I think of push as a single transaction, but that's\nonly if the caller sends the \"atomic\" capability. Otherwise get one per\nref.\n\n> diff --git a/refs.c b/refs.c\n> index 2dd851fe81..17e515b288 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2044,7 +2044,7 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n>  \tif (hook == &hook_not_found)\n>  \t\treturn ret;\n>  \tif (!hook)\n> -\t\thook = find_hook(\"reference-transaction\");\n> +\t\thook = xstrdup_or_null(find_hook(\"reference-transaction\"));\n>  \tif (!hook) {\n>  \t\thook = &hook_not_found;\n>  \t\treturn ret;\n\nThe fix here looks obviously correct, though I have to wonder if the\ncaching is even worth it. It's saving us an access() call, but we're\nabout to exec a whole sub-process.\n\nIt's perhaps more justifiable when there isn't a hook (we're still just\npaying that one access(), but it's proportionally bigger). I kind of\ndoubt it's measurable, though, since a ref write requires a bunch of\nsyscalls anyway.\n\n-Peff\n"},{"id":"403126","messageId":"20200807090412.GA1407904@tanuki.pks.im","threadId":"54007","inReplyTo":"20200807075837.GA32344@coredump.intra.peff.net","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-07T09:04:12Z","receivedAt":"2020-08-07T09:04:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 07, 2020 at 03:58:37AM -0400, Jeff King wrote:\n> On Fri, Aug 07, 2020 at 09:05:58AM +0200, Patrick Steinhardt wrote:\n> \n> > In order to not repeatedly search for the reference-transaction hook in\n> > case it's getting called multiple times, we use a caching mechanism to\n> > only call `find_hook()` once. What was missed though is that the return\n> > value of `find_hook()` actually comes from a static strbuf, which means\n> > it will get overwritten when calling `find_hook()` again. As a result,\n> > we may call the wrong hook with parameters of the reference-transaction\n> > hook.\n> > \n> > This scenario was spotted in the wild when executing a git-push(1) with\n> > multiple references, where there are interleaving calls to both the\n> > update and the reference-transaction hook. While initial calls to the\n> > reference-transaction hook work as expected, it will stop working after\n> > the next invocation of the update hook. The result is that we now start\n> > calling the update hook with parameters and stdin of the\n> > reference-transaction hook.\n> \n> That makes sense. I think of push as a single transaction, but that's\n> only if the caller sends the \"atomic\" capability. Otherwise get one per\n> ref.\n> \n> > diff --git a/refs.c b/refs.c\n> > index 2dd851fe81..17e515b288 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -2044,7 +2044,7 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n> >  \tif (hook == &hook_not_found)\n> >  \t\treturn ret;\n> >  \tif (!hook)\n> > -\t\thook = find_hook(\"reference-transaction\");\n> > +\t\thook = xstrdup_or_null(find_hook(\"reference-transaction\"));\n> >  \tif (!hook) {\n> >  \t\thook = &hook_not_found;\n> >  \t\treturn ret;\n> \n> The fix here looks obviously correct, though I have to wonder if the\n> caching is even worth it. It's saving us an access() call, but we're\n> about to exec a whole sub-process.\n> \n> It's perhaps more justifiable when there isn't a hook (we're still just\n> paying that one access(), but it's proportionally bigger). I kind of\n> doubt it's measurable, though, since a ref write requires a bunch of\n> syscalls anyway.\n\nYeah, this really was done to not have to pay a performance penalty if\nupdating thousands of refs if no reference-transaction hook exists. E.g.\nif doing a non-atomic push of n reference, we'd have n calls to\naccess(3P). See [1] for reference.\n\nI've just did another quick benchmark without the cache, and it still\nconsistently shows a non-zero performance hit without it:\n\nTest                         pks-reftx-hook-interleaving   no-cache\n--------------------------------------------------------------------------------\n1400.2: update-ref           2.82(2.13+0.81)               2.86(2.19+0.78) +1.4%\n1400.3: update-ref --stdin   0.22(0.07+0.15)               0.22(0.07+0.15) +0.0%\n\nPatrick\n\n[1]: https://public-inbox.org/git/20200603165142.GA24049@syl.local/\n"},{"id":"403128","messageId":"20200807093239.GA1228157@coredump.intra.peff.net","threadId":"54007","inReplyTo":"20200807090412.GA1407904@tanuki.pks.im","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T09:32:39Z","receivedAt":"2020-08-07T09:32:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 11:04:12AM +0200, Patrick Steinhardt wrote:\n\n> > It's perhaps more justifiable when there isn't a hook (we're still just\n> > paying that one access(), but it's proportionally bigger). I kind of\n> > doubt it's measurable, though, since a ref write requires a bunch of\n> > syscalls anyway.\n> \n> Yeah, this really was done to not have to pay a performance penalty if\n> updating thousands of refs if no reference-transaction hook exists. E.g.\n> if doing a non-atomic push of n reference, we'd have n calls to\n> access(3P). See [1] for reference.\n> \n> I've just did another quick benchmark without the cache, and it still\n> consistently shows a non-zero performance hit without it:\n> \n> Test                         pks-reftx-hook-interleaving   no-cache\n> --------------------------------------------------------------------------------\n> 1400.2: update-ref           2.82(2.13+0.81)               2.86(2.19+0.78) +1.4%\n> 1400.3: update-ref --stdin   0.22(0.07+0.15)               0.22(0.07+0.15) +0.0%\n\nI'm skeptical that those results are useful. In the first test, we're\nrunning update-ref 1000 times, so:\n\n  - the cache shouldn't be helping at all, since we only have one ref to\n    update (well, I guess once for \"prepare\" and once for \"commit\", so\n    really it's saving one syscall total per process).\n\n  - I'd expect a lot of noise because we're spending most of our time in\n    starting up the process\n\nIn the second test, we run 1000 ref operations per update-ref process.\nSo we should be cutting down on our hook-lookup overhead by a factor of\n1000. Yet it shows no improvement.\n\nThat implies you're just seeing noise. And indeed, with the patch below\nI get:\n\nTest                         HEAD^             HEAD\n--------------------------------------------------------------------\n1400.2: update-ref           1.93(1.57+0.42)   1.91(1.55+0.42) -1.0%\n1400.3: update-ref --stdin   0.07(0.02+0.05)   0.07(0.02+0.05) +0.0%\n\nRunning it a second time gets me +0.5%. :)\n\n---\n refs.c | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2dd851fe81..427bf5843c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2031,24 +2031,16 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n-static const char hook_not_found;\n-static const char *hook;\n-\n static int run_transaction_hook(struct ref_transaction *transaction,\n \t\t\t\tconst char *state)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *hook = find_hook(\"reference-transaction\");\n \tint ret = 0, i;\n \n-\tif (hook == &hook_not_found)\n-\t\treturn ret;\n \tif (!hook)\n-\t\thook = find_hook(\"reference-transaction\");\n-\tif (!hook) {\n-\t\thook = &hook_not_found;\n \t\treturn ret;\n-\t}\n \n \targv_array_pushl(&proc.args, hook, state, NULL);\n \tproc.in = -1;\n-- \n2.28.0.520.g10e2ce7e11\n\n"},{"id":"403129","messageId":"20200807094946.GA1758126@tanuki.pks.im","threadId":"54007","inReplyTo":"20200807093239.GA1228157@coredump.intra.peff.net","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-07T09:49:46Z","receivedAt":"2020-08-07T09:49:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 07, 2020 at 05:32:39AM -0400, Jeff King wrote:\n> On Fri, Aug 07, 2020 at 11:04:12AM +0200, Patrick Steinhardt wrote:\n> \n> > > It's perhaps more justifiable when there isn't a hook (we're still just\n> > > paying that one access(), but it's proportionally bigger). I kind of\n> > > doubt it's measurable, though, since a ref write requires a bunch of\n> > > syscalls anyway.\n> > \n> > Yeah, this really was done to not have to pay a performance penalty if\n> > updating thousands of refs if no reference-transaction hook exists. E.g.\n> > if doing a non-atomic push of n reference, we'd have n calls to\n> > access(3P). See [1] for reference.\n> > \n> > I've just did another quick benchmark without the cache, and it still\n> > consistently shows a non-zero performance hit without it:\n> > \n> > Test                         pks-reftx-hook-interleaving   no-cache\n> > --------------------------------------------------------------------------------\n> > 1400.2: update-ref           2.82(2.13+0.81)               2.86(2.19+0.78) +1.4%\n> > 1400.3: update-ref --stdin   0.22(0.07+0.15)               0.22(0.07+0.15) +0.0%\n> \n> I'm skeptical that those results are useful. In the first test, we're\n> running update-ref 1000 times, so:\n> \n>   - the cache shouldn't be helping at all, since we only have one ref to\n>     update (well, I guess once for \"prepare\" and once for \"commit\", so\n>     really it's saving one syscall total per process).\n> \n>   - I'd expect a lot of noise because we're spending most of our time in\n>     starting up the process\n> \n> In the second test, we run 1000 ref operations per update-ref process.\n> So we should be cutting down on our hook-lookup overhead by a factor of\n> 1000. Yet it shows no improvement.\n> \n> That implies you're just seeing noise. And indeed, with the patch below\n> I get:\n> \n> Test                         HEAD^             HEAD\n> --------------------------------------------------------------------\n> 1400.2: update-ref           1.93(1.57+0.42)   1.91(1.55+0.42) -1.0%\n> 1400.3: update-ref --stdin   0.07(0.02+0.05)   0.07(0.02+0.05) +0.0%\n> \n> Running it a second time gets me +0.5%. :)\n\nYeah, it's also been my take that OS-level overhead is probably going to\nmatter more than those access calls, and I argued such back when I\nproposed the hook. So I'm perfectly happy to see this caching mechanism\ngo.\n\nShould I re-post a v2 with your patch and my test?\n\nPatrick\n"},{"id":"403136","messageId":"xmqqpn82gy51.fsf@gitster.c.googlers.com","threadId":"54007","inReplyTo":"20200807094946.GA1758126@tanuki.pks.im","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T17:32:26Z","receivedAt":"2020-08-07T17:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Aug 07, 2020 at 05:32:39AM -0400, Jeff King wrote:\n>\n>> That implies you're just seeing noise. And indeed, with the patch below\n>> I get:\n>> \n>> Test                         HEAD^             HEAD\n>> --------------------------------------------------------------------\n>> 1400.2: update-ref           1.93(1.57+0.42)   1.91(1.55+0.42) -1.0%\n>> 1400.3: update-ref --stdin   0.07(0.02+0.05)   0.07(0.02+0.05) +0.0%\n>> \n>> Running it a second time gets me +0.5%. :)\n>\n> Yeah, it's also been my take that OS-level overhead is probably going to\n> matter more than those access calls, and I argued such back when I\n> proposed the hook. So I'm perfectly happy to see this caching mechanism\n> go.\n\nIs the above about negative cache?  IOW, does the above demonstrate\nthat one extra access() to make sure there is no hook does not hurt\nus anything?  \n\nIf so, yes, I am 100% for removing the cache mechanism.\n\nThanks for driving design decision with numbers.  That's always\npleasant to see.\n\n\n"},{"id":"403139","messageId":"20200807182141.GA1307024@coredump.intra.peff.net","threadId":"54007","inReplyTo":"20200807094946.GA1758126@tanuki.pks.im","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T18:21:41Z","receivedAt":"2020-08-07T18:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 11:49:46AM +0200, Patrick Steinhardt wrote:\n\n> Yeah, it's also been my take that OS-level overhead is probably going to\n> matter more than those access calls, and I argued such back when I\n> proposed the hook. So I'm perfectly happy to see this caching mechanism\n> go.\n> \n> Should I re-post a v2 with your patch and my test?\n\nSure, that would be fine (to be clear, I'd also be OK with your original\npatch, too; it was mostly just a curiosity to me).\n\n-Peff\n"},{"id":"403140","messageId":"20200807190024.GA1307861@coredump.intra.peff.net","threadId":"54007","inReplyTo":"xmqqpn82gy51.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T19:00:24Z","receivedAt":"2020-08-07T19:00:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 10:32:26AM -0700, Junio C Hamano wrote:\n\n> >> Test                         HEAD^             HEAD\n> >> --------------------------------------------------------------------\n> >> 1400.2: update-ref           1.93(1.57+0.42)   1.91(1.55+0.42) -1.0%\n> >> 1400.3: update-ref --stdin   0.07(0.02+0.05)   0.07(0.02+0.05) +0.0%\n> >> \n> >> Running it a second time gets me +0.5%. :)\n> >\n> > Yeah, it's also been my take that OS-level overhead is probably going to\n> > matter more than those access calls, and I argued such back when I\n> > proposed the hook. So I'm perfectly happy to see this caching mechanism\n> > go.\n> \n> Is the above about negative cache?  IOW, does the above demonstrate\n> that one extra access() to make sure there is no hook does not hurt\n> us anything?\n\nYes, those numbers are with no cache at all, and without a hook. So they\nare measuring the cost of access() only.\n\n-Peff\n"},{"id":"403144","messageId":"xmqqd042gsve.fsf@gitster.c.googlers.com","threadId":"54007","inReplyTo":"20200807182141.GA1307024@coredump.intra.peff.net","subject":"Re: [PATCH] refs: fix interleaving hook calls with reference-transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T19:26:13Z","receivedAt":"2020-08-07T19:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Aug 07, 2020 at 11:49:46AM +0200, Patrick Steinhardt wrote:\n>\n>> Yeah, it's also been my take that OS-level overhead is probably going to\n>> matter more than those access calls, and I argued such back when I\n>> proposed the hook. So I'm perfectly happy to see this caching mechanism\n>> go.\n>> \n>> Should I re-post a v2 with your patch and my test?\n>\n> Sure, that would be fine (to be clear, I'd also be OK with your original\n> patch, too; it was mostly just a curiosity to me).\n\nLet's queue the correctness fix patch as-is. \n\nWe can and should make the simplification as a separate step,\njustified with the performance numbers.\n\nThanks.\n"}]}