{"thread":{"id":"54675","subject":"","startedAt":"2020-11-19T08:18:53Z","lastAt":"2020-12-02T22:15:24Z","messageCount":16,"participants":["唐宇奕","René Scharfe","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"410356","messageId":"CAFWsj_UwkQX9y0xPQJE3xguo0z3TMkvKAwei5iryCWXvVP8CjA@mail.gmail.com","threadId":"54675","inReplyTo":null,"subject":"","fromName":"唐宇奕","fromEmail":"winglovet@gmail.com","sentAt":"2020-11-19T08:18:36Z","receivedAt":"2020-11-19T08:18:53Z","isPatch":false,"sender":{"key":"winglovet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16458679?v=4"},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n 1. linux environment, prepare a large repo to give you enough time to debug\n 2. clone that repo\n 3. during clone, send SIGTERM to the git-upload-pack process\n\nWhat did you expect to happen? (Expected behavior)\n there is no git process left\n\nWhat happened instead? (Actual behavior)\n there is a git pack-objects process which is forked by the\ngit-upload-pack process, it becomes a zombie\n\nWhat's different between what you expected and what actually happened?\n there shouldn't exist zombie processes\n\nAnything else you want to add:\n you may need a docker container environment to reproduce this since\nmostly init process will cleanup zombie processes for you\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.26.2\ncpu: x86_64\n"},{"id":"410438","messageId":"badf3777-3970-b714-3ad9-67d2f77f94a5@web.de","threadId":"54675","inReplyTo":"CAFWsj_UwkQX9y0xPQJE3xguo0z3TMkvKAwei5iryCWXvVP8CjA@mail.gmail.com","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [was: (no subject)]","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-20T18:52:45Z","receivedAt":"2020-11-20T18:53:22Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.11.20 um 09:18 schrieb 唐宇奕:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> 1. linux environment, prepare a large repo to give you enough time to debug\n> 2. clone that repo\n> 3. during clone, send SIGTERM to the git-upload-pack process\n>\n> What did you expect to happen? (Expected behavior)\n> there is no git process left\n>\n> What happened instead? (Actual behavior)\n> there is a git pack-objects process which is forked by the\n> git-upload-pack process, it becomes a zombie\n>\n> What's different between what you expected and what actually happened?\n> there shouldn't exist zombie processes\n>\n> Anything else you want to add:\n> you may need a docker container environment to reproduce this since\n> mostly init process will cleanup zombie processes for you\n>\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n>\n>\n> [System Info]\n> git version:\n> git version 2.26.2\n> cpu: x86_64\n\nHave you seen this working as you expect in an earlier version?\n\nI suspect it's a matter of turning on some flags (patch below), but I\nhave to admit I don't really know what I'm doing here -- and why this\nhasn't been done already.\n\nRené\n\n---\n upload-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5dc8e1f844..e42dea26fa 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -321,6 +321,8 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n+\tpack_objects.clean_on_exit = 1;\n+\tpack_objects.wait_after_clean = 1;\n\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n--\n2.29.2\n"},{"id":"410449","messageId":"20201121002921.GC353076@coredump.intra.peff.net","threadId":"54675","inReplyTo":"badf3777-3970-b714-3ad9-67d2f77f94a5@web.de","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [was: (no subject)]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-21T00:29:21Z","receivedAt":"2020-11-21T00:29:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 20, 2020 at 07:52:45PM +0100, René Scharfe wrote:\n\n> Have you seen this working as you expect in an earlier version?\n> \n> I suspect it's a matter of turning on some flags (patch below), but I\n> have to admit I don't really know what I'm doing here -- and why this\n> hasn't been done already.\n> \n> René\n> \n> ---\n> upload-pack.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index 5dc8e1f844..e42dea26fa 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -321,6 +321,8 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> \tpack_objects.in = -1;\n> \tpack_objects.out = -1;\n> \tpack_objects.err = -1;\n> +\tpack_objects.clean_on_exit = 1;\n> +\tpack_objects.wait_after_clean = 1;\n\nYeah, clean_on_exit seems quite reasonable to me. I suspect nobody ever\nreally noticed, because as soon as pack-objects starts to write out the\npack, it will get SIGPIPE or EPIPE and die. But there is no point in\nletting it chug on expensive object enumeration or delta compression if\nupload-pack has already exited.\n\nI don't know that wait_after_clean is necessary. We don't need to wait\nfor pack-objects to fail.\n\nOn the flip side, one of the reasons I added clean_on_exit long ago was\nfor the similar issue on the push side, which is even worse. Here we\nmight just waste some CPU, but on the push side we connect pack-objects\ndirectly to the remote socket, so it could actually complete the push\n(from the server's perspective) after the local git-push has died. Or at\nleast I think that was possible at one point; it might not be the case\nany more.\n\nI wrote this patch ages ago, and it is still sitting close to the bottom\n(if not the bottom) of my todo stack, waiting to be investigated. ;)\n\n-- >8 --\nSubject: [PATCH] send-pack: kill pack-objects helper on signal or exit\n\nWe spawn an external pack-objects process to actually send\nobjects to the remote side. If we are killed by a signal\nduring this process, the pack-objects will keep running and\ncomplete the push, which may surprise the user. We should\ntake it down when we go down.\n\nSigned-off-by: Jeff King \n---\n send-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex eb4a44270b..d2701bf35c 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -85,6 +85,7 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n \tpo.in = -1;\n \tpo.out = args->stateless_rpc ? -1 : fd;\n \tpo.git_cmd = 1;\n+\tpo.clean_on_exit = 1;\n \tif (start_command(&po))\n \t\tdie_errno(\"git pack-objects failed\");\n \n-- \n2.29.2.730.g3e418f96ba\n\n"},{"id":"410474","messageId":"xmqqd006s7ee.fsf@gitster.c.googlers.com","threadId":"54675","inReplyTo":"20201121002921.GC353076@coredump.intra.peff.net","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-21T21:54:17Z","receivedAt":"2020-11-21T21:54:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, clean_on_exit seems quite reasonable to me. I suspect nobody ever\n> really noticed, because as soon as pack-objects starts to write out the\n> pack, it will get SIGPIPE or EPIPE and die. But there is no point in\n> letting it chug on expensive object enumeration or delta compression if\n> upload-pack has already exited.\n>\n> I don't know that wait_after_clean is necessary. We don't need to wait\n> for pack-objects to fail.\n>\n> On the flip side, one of the reasons I added clean_on_exit long ago was\n> for the similar issue on the push side, which is even worse. Here we\n> might just waste some CPU, but on the push side we connect pack-objects\n> directly to the remote socket, so it could actually complete the push\n> (from the server's perspective) after the local git-push has died. Or at\n> least I think that was possible at one point; it might not be the case\n> any more.\n>\n> I wrote this patch ages ago, and it is still sitting close to the bottom\n> (if not the bottom) of my todo stack, waiting to be investigated. ;)\n\nSounds sensible.\n\n> -- >8 --\n> Subject: [PATCH] send-pack: kill pack-objects helper on signal or exit\n>\n> We spawn an external pack-objects process to actually send\n> objects to the remote side. If we are killed by a signal\n> during this process, the pack-objects will keep running and\n> complete the push, which may surprise the user. We should\n> take it down when we go down.\n>\n> Signed-off-by: Jeff King \n> ---\n> send-pack.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index eb4a44270b..d2701bf35c 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -85,6 +85,7 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n> \tpo.in = -1;\n> \tpo.out = args->stateless_rpc ? -1 : fd;\n> \tpo.git_cmd = 1;\n> +\tpo.clean_on_exit = 1;\n> \tif (start_command(&po))\n> \t\tdie_errno(\"git pack-objects failed\");\n"},{"id":"410628","messageId":"CAFWsj_WsexXgFe-jMYOk8_m9N7GLrhRjqFjRWRePZmxUDEYkSg@mail.gmail.com","threadId":"54675","inReplyTo":"xmqqd006s7ee.fsf@gitster.c.googlers.com","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"唐宇奕","fromEmail":"winglovet@gmail.com","sentAt":"2020-11-24T03:21:09Z","receivedAt":"2020-11-24T03:21:44Z","isPatch":false,"sender":{"key":"winglovet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16458679?v=4"},"body":"when will this bug be fixed?\n\nJunio C Hamano 于2020年11月22日周日 上午5:54写道：\n>\n> Jeff King writes:\n>\n> > Yeah, clean_on_exit seems quite reasonable to me. I suspect nobody ever\n> > really noticed, because as soon as pack-objects starts to write out the\n> > pack, it will get SIGPIPE or EPIPE and die. But there is no point in\n> > letting it chug on expensive object enumeration or delta compression if\n> > upload-pack has already exited.\n> >\n> > I don't know that wait_after_clean is necessary. We don't need to wait\n> > for pack-objects to fail.\n> >\n> > On the flip side, one of the reasons I added clean_on_exit long ago was\n> > for the similar issue on the push side, which is even worse. Here we\n> > might just waste some CPU, but on the push side we connect pack-objects\n> > directly to the remote socket, so it could actually complete the push\n> > (from the server's perspective) after the local git-push has died. Or at\n> > least I think that was possible at one point; it might not be the case\n> > any more.\n> >\n> > I wrote this patch ages ago, and it is still sitting close to the bottom\n> > (if not the bottom) of my todo stack, waiting to be investigated. ;)\n>\n> Sounds sensible.\n>\n> > -- >8 --\n> > Subject: [PATCH] send-pack: kill pack-objects helper on signal or exit\n> >\n> > We spawn an external pack-objects process to actually send\n> > objects to the remote side. If we are killed by a signal\n> > during this process, the pack-objects will keep running and\n> > complete the push, which may surprise the user. We should\n> > take it down when we go down.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > send-pack.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/send-pack.c b/send-pack.c\n> > index eb4a44270b..d2701bf35c 100644\n> > --- a/send-pack.c\n> > +++ b/send-pack.c\n> > @@ -85,6 +85,7 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n> > po.in = -1;\n> > po.out = args->stateless_rpc ? -1 : fd;\n> > po.git_cmd = 1;\n> > + po.clean_on_exit = 1;\n> > if (start_command(&po))\n> > die_errno(\"git pack-objects failed\");\n"},{"id":"410647","messageId":"X7zOKbzR9gwJHMbJ@coredump.intra.peff.net","threadId":"54675","inReplyTo":"xmqqd006s7ee.fsf@gitster.c.googlers.com","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-24T09:11:05Z","receivedAt":"2020-11-24T09:11:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 21, 2020 at 01:54:17PM -0800, Junio C Hamano wrote:\n\n> > I wrote this patch ages ago, and it is still sitting close to the bottom\n> > (if not the bottom) of my todo stack, waiting to be investigated. ;)\n> \n> Sounds sensible.\n> \n> > -- >8 --\n> > Subject: [PATCH] send-pack: kill pack-objects helper on signal or exit\n\nLooks like you picked up this patch on a topic branch\n\nI was thinking of polishing it further with a test, but I can't think of\na good way to do one that would not be horribly racy. I even tried to\nthink of a racy test I could run manually (e.g., pushing and killing in\na loop) but even that is tough (even with the patch, it's racy whether\nthe push will succeed or not, depending on exactly when you kill the\nparent, so there's no automatic way to distinguish the improved cases).\n\nSo I think it is still a good idea to do, and what I wrote earlier is\nas good a we can do.\n\nRené, do you want to wrap up your similar patch for the fetch side?\n\n-Peff\n"},{"id":"410806","messageId":"xmqqy2ipcdvj.fsf@gitster.c.googlers.com","threadId":"54675","inReplyTo":"X7zOKbzR9gwJHMbJ@coredump.intra.peff.net","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T21:42:24Z","receivedAt":"2020-11-25T21:42:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think it is still a good idea to do, and what I wrote earlier is\n> as good a we can do.\n>\n> René, do you want to wrap up your similar patch for the fetch side?\n\nIn the meantime, I may queue this on 'seen' but it cannot move\nforward without a signoff.\n\nThanks.\n\n--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ---\nFrom: René Scharfe \nDate: Fri, 20 Nov 2020 19:52:45 +0100\nSubject: [PATCH] upload-pack: kill pack-objects helper on signal or exit\n\nWe spawn an external pack-objects process to actually send objects\nto the remote side. If we are killed by a signal during this\nprocess, the pack-objects will hang around as a zombie. We should\ntake it down when we go down.\n---\n upload-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb457..d4f7192d04 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -321,6 +321,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n+\tpack_objects.clean_on_exit = 1;\n \n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n-- \n2.29.2-538-g65d51b1459\n"},{"id":"410844","messageId":"X778eIAr3uzdh0H0@coredump.intra.peff.net","threadId":"54675","inReplyTo":"xmqqy2ipcdvj.fsf@gitster.c.googlers.com","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-26T00:53:12Z","receivedAt":"2020-11-26T00:53:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2020 at 01:42:24PM -0800, Junio C Hamano wrote:\n\n> In the meantime, I may queue this on 'seen' but it cannot move\n> forward without a signoff.\n> \n> Thanks.\n> \n> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ---\n> From: René Scharfe \n> Date: Fri, 20 Nov 2020 19:52:45 +0100\n> Subject: [PATCH] upload-pack: kill pack-objects helper on signal or exit\n> \n> We spawn an external pack-objects process to actually send objects\n> to the remote side. If we are killed by a signal during this\n> process, the pack-objects will hang around as a zombie. We should\n> take it down when we go down.\n\nI think this is a good thing to do. I'd probably avoid the word \"zombie\"\nhere, though. The orphaned pack-objects does not become a zombie process\nin the traditional Unix sense of the word, waiting to be reaped by a\nparent which is not paying attention. Instead it is still running but\ndoing work for a caller that will never read the result.\n\nSo maybe:\n\n We spawn an external pack-objects process to actually send objects to\n the remote side. If we are killed by a signal during this process,\n then pack-objects may continue to run. As soon as it starts producing\n output for the pack, it will see a failure writing to upload-pack and\n exit itself. But before then, it may do significant work traversing\n the object graph, compressing deltas, etc, which will all be\n pointless. So let's make sure to kill as soon as we know that the\n caller will not read the result.\n\n-Peff\n"},{"id":"410848","messageId":"xmqqo8jllyhc.fsf@gitster.c.googlers.com","threadId":"54675","inReplyTo":"X778eIAr3uzdh0H0@coredump.intra.peff.net","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-26T01:04:47Z","receivedAt":"2020-11-26T01:04:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Nov 25, 2020 at 01:42:24PM -0800, Junio C Hamano wrote:\n>\n>> In the meantime, I may queue this on 'seen' but it cannot move\n>> forward without a signoff.\n>> ...\n> We spawn an external pack-objects process to actually send objects to\n> the remote side. If we are killed by a signal during this process,\n> then pack-objects may continue to run. As soon as it starts producing\n> output for the pack, it will see a failure writing to upload-pack and\n> exit itself. But before then, it may do significant work traversing\n> the object graph, compressing deltas, etc, which will all be\n> pointless. So let's make sure to kill as soon as we know that the\n> caller will not read the result.\n\nThanks, that reads well.\n"},{"id":"410900","messageId":"bd2c4577-4c8c-851c-6045-ba4b306ca612@web.de","threadId":"54675","inReplyTo":"xmqqo8jllyhc.fsf@gitster.c.googlers.com","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-26T20:04:35Z","receivedAt":"2020-11-26T20:05:33Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.11.20 um 02:04 schrieb Junio C Hamano:\n> Jeff King writes:\n>\n>> On Wed, Nov 25, 2020 at 01:42:24PM -0800, Junio C Hamano wrote:\n>>\n>>> In the meantime, I may queue this on 'seen' but it cannot move\n>>> forward without a signoff.\n>>> ...\n>> We spawn an external pack-objects process to actually send objects to\n>> the remote side. If we are killed by a signal during this process,\n>> then pack-objects may continue to run. As soon as it starts producing\n>> output for the pack, it will see a failure writing to upload-pack and\n>> exit itself. But before then, it may do significant work traversing\n>> the object graph, compressing deltas, etc, which will all be\n>> pointless. So let's make sure to kill as soon as we know that the\n>> caller will not read the result.\n>\n> Thanks, that reads well.\n>\n\nThe patch is trivial, you don't need my sign-off. You could record Peff\nas its author, as he contributed the most to the version in seen.\n\nBefore I could submit that one (or something similar) formally, I'd need\nto understand what's happening here a lot better and witness the effect\nof the patch.\n\nI understand that the main benefit of stopping the child upon\ntermination of the parent is to avoid using CPU cycles on a heavy task\nwhose results will just go to waste. But wouldn't the orphaned child\nthen become a zombie? Init would reap it eventually, but are there\nperhaps init-less deployments (containerized daemon?) where such\nzombies could pile up?\n\nFor a test, winning the race condition should be easy if we cheat by\nletting the child loop forever. But I struggle even with the most\nbasic task: Making upload-pack invoked by clone call pack-objects.\n(Feeling a bit silly.)\n\nRené\n"},{"id":"410916","messageId":"X8B9589XMlCQEltA@coredump.intra.peff.net","threadId":"54675","inReplyTo":"bd2c4577-4c8c-851c-6045-ba4b306ca612@web.de","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-27T04:17:43Z","receivedAt":"2020-11-27T04:17:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 26, 2020 at 09:04:35PM +0100, René Scharfe wrote:\n\n> Before I could submit that one (or something similar) formally, I'd need\n> to understand what's happening here a lot better and witness the effect\n> of the patch.\n> \n> I understand that the main benefit of stopping the child upon\n> termination of the parent is to avoid using CPU cycles on a heavy task\n> whose results will just go to waste. But wouldn't the orphaned child\n> then become a zombie? Init would reap it eventually, but are there\n> perhaps init-less deployments (containerized daemon?) where such\n> zombies could pile up?\n\nI think an init-less deployment like that is already broken. If we\nencounter any error at all in upload-pack we may quit without reaping\nall of our children. And this could never be protected against entirely;\nwe could be killed by SIGSEGV, SIGKILL, etc.\n\nMy understanding is container deployments often have a tiny pid-1 init\nthat takes care of zombie processes like this (but it's not something\nI've dealt with much myself).\n\n> For a test, winning the race condition should be easy if we cheat by\n> letting the child loop forever. But I struggle even with the most\n> basic task: Making upload-pack invoked by clone call pack-objects.\n> (Feeling a bit silly.)\n\nHere's an easy reproduction. On a clone of something large-ish (by\nnumber of objects) like linux.git:\n\n - make sure you don't have bitmaps on (since they make the enumerating\n phase go quickly). For linux.git it takes ~30s or so to walk the\n whole graph on my machine.\n\n - run \"git clone --no-local -q . dst\"; the \"-q\" is important because\n if pack-objects is writing progress to upload-pack (to get\n multiplexed over the sideband to the client), then it will notice\n pretty quickly the failure to write to stderr\n\n - kill just upload-pack with \"pkill git-upload-pack\" or whatever you\n like\n\n - run \"ps au | grep pack-objects\" (or just \"top\") to see pack-objects\n chugging on 100% CPU (and consuming 1GB+ of RAM)\n\nWith the patch adding clean_on_exit, that last step turns up nothing.\n\nNow the situation above is probably pretty rare. Nobody is usually going\nto kill upload-pack specifically. The more common case is when\nupload-pack realizes that the client (or the network) has gone away,\nbecause it tries to write and finds the connection gone. But what is it\nwriting? Most of the time it's stuff from pack-objects! So in the normal\ncase, pack-objects is continually writing either data or progress\nreports, so it would notice for its next write.\n\nBut again, a client asking for no progress is a problem. upload-pack\nwill be sending keepalives every 5s or so, so it will notice client\ndeath then. But pack-objects will keep running, not generating any\noutput until it starts spewing the pack.\n\nSo you could probably make the scenario above a bit more realistic by\nkilling the parent git-clone process. But don't use ^C; that will send\nSIGINT to all of the processes. Simulate a network failure by killing\nthe \"git clone\" process specifically. This shows the same problem, and\nthe same improvement after the patch (though remember it may take up to\n5 seconds for upload-pack to send a keepalive and notice the problem).\n\n-Peff\n"},{"id":"410935","messageId":"d0fcfa7f-9835-a9e6-c4a4-af4de177ff8c@web.de","threadId":"54675","inReplyTo":"X8B9589XMlCQEltA@coredump.intra.peff.net","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-27T20:43:06Z","receivedAt":"2020-11-27T20:45:54Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.11.20 um 05:17 schrieb Jeff King:\n> On Thu, Nov 26, 2020 at 09:04:35PM +0100, René Scharfe wrote:\n>\n>> Before I could submit that one (or something similar) formally, I'd need\n>> to understand what's happening here a lot better and witness the effect\n>> of the patch.\n>>\n>> I understand that the main benefit of stopping the child upon\n>> termination of the parent is to avoid using CPU cycles on a heavy task\n>> whose results will just go to waste. But wouldn't the orphaned child\n>> then become a zombie? Init would reap it eventually, but are there\n>> perhaps init-less deployments (containerized daemon?) where such\n>> zombies could pile up?\n>\n> I think an init-less deployment like that is already broken. If we\n> encounter any error at all in upload-pack we may quit without reaping\n> all of our children. And this could never be protected against entirely;\n> we could be killed by SIGSEGV, SIGKILL, etc.\n\nThat might be true, but it might still be usable if the rate of zombie\nproduction is low enough. And reducing it slightly might still help by\nincreasing the time between container restarts. Segfaults should be\nvery rare, and people using kill -9 can clean up after themselves..\n\n> My understanding is container deployments often have a tiny pid-1 init\n> that takes care of zombie processes like this (but it's not something\n> I've dealt with much myself).\n\nTrue, e.g. https://github.com/krallin/tini, which is built into newer\nDocker releases already. So this problem is real and has an (optional)\nsolution.\n\nOK, so overall the situation sounds a bit messy to me and perhaps\nthere's room for improvement, but I agree now that we can leave the\nspecialists (init, tini) to deal with our zombies.\n\n>> For a test, winning the race condition should be easy if we cheat by\n>> letting the child loop forever. But I struggle even with the most\n>> basic task: Making upload-pack invoked by clone call pack-objects.\n>> (Feeling a bit silly.)\n>\n> Here's an easy reproduction. On a clone of something large-ish (by\n> number of objects) like linux.git:\n>\n> - make sure you don't have bitmaps on (since they make the enumerating\n> phase go quickly). For linux.git it takes ~30s or so to walk the\n> whole graph on my machine.\n>\n> - run \"git clone --no-local -q . dst\"; the \"-q\" is important because\n> if pack-objects is writing progress to upload-pack (to get\n> multiplexed over the sideband to the client), then it will notice\n> pretty quickly the failure to write to stderr\n>\n> - kill just upload-pack with \"pkill git-upload-pack\" or whatever you\n> like\n>\n> - run \"ps au | grep pack-objects\" (or just \"top\") to see pack-objects\n> chugging on 100% CPU (and consuming 1GB+ of RAM)\n>\n> With the patch adding clean_on_exit, that last step turns up nothing.\n\nI was missing --no-local, d'oh!\n\nTo win the race consistently I used this:\n\n-- >8 --\n\ndiff --git a/run-command.c b/run-command.c\nindex ea4d0fb4b1..a6bf0a86dd 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -672,6 +672,19 @@ int start_command(struct child_process *cmd)\n \tint failed_errno;\n \tchar *str;\n\n+\tconst char *loop_argv[] = { \"while :; do sleep 1; done\", NULL };\n+\tconst char *fail_cmd = getenv(\"GIT_DEBUG_ABANDON_CHILD\");\n+\tconst char *argv0 = cmd->argv ? cmd->argv[0] : cmd->args.v[0];\n+\tint fail = fail_cmd && starts_with(argv0, fail_cmd);\n+\n+\tif (fail) {\n+\t\tfprintf(stderr, \"starting endless loop instead of %s\\n\",\n+\t\t\tcmd->argv ? cmd->argv[0] : cmd->args.v[0]);\n+\t\tcmd->argv = loop_argv;\n+\t\tcmd->use_shell = 1;\n+\t\tcmd->git_cmd = 0;\n+\t}\n+\n \tif (!cmd->argv)\n \t\tcmd->argv = cmd->args.v;\n \tif (!cmd->env)\n@@ -982,6 +995,9 @@ int start_command(struct child_process *cmd)\n \telse if (cmd->err)\n \t\tclose(cmd->err);\n\n+\tif (fail)\n+\t\tdie(\"abandoning child %\"PRIuMAX\"\\n\", (uintmax_t)cmd->pid);\n+\n \treturn 0;\n }\n\n\n--- 8< ---\n\nWe could build tests that always win (or lose, based on your\nperspective) the race condition based on that. Turning on clean_on_exit\nis such a no-brainer that I don't see the need for one, though.\n\n> Now the situation above is probably pretty rare. Nobody is usually going\n> to kill upload-pack specifically. The more common case is when\n> upload-pack realizes that the client (or the network) has gone away,\n> because it tries to write and finds the connection gone. But what is it\n> writing? Most of the time it's stuff from pack-objects! So in the normal\n> case, pack-objects is continually writing either data or progress\n> reports, so it would notice for its next write.\n>\n> But again, a client asking for no progress is a problem. upload-pack\n> will be sending keepalives every 5s or so, so it will notice client\n> death then. But pack-objects will keep running, not generating any\n> output until it starts spewing the pack.\n>\n> So you could probably make the scenario above a bit more realistic by\n> killing the parent git-clone process. But don't use ^C; that will send\n> SIGINT to all of the processes. Simulate a network failure by killing\n> the \"git clone\" process specifically. This shows the same problem, and\n> the same improvement after the patch (though remember it may take up to\n> 5 seconds for upload-pack to send a keepalive and notice the problem).\n\nWith the debug patch above and GIT_DEBUG_ABANDON_CHILD=git-upload-pack I\nneed the following patch get rid of the spawned process:\n\n--- >8 ---\n\ndiff --git a/connect.c b/connect.c\nindex 8b8f56cf6d..e1b1b73ef5 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1369,6 +1369,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n\n \t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n+\t\tconn->clean_on_exit = 1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n\n--- 8< ---\n\nSo is there a downside to clean_on_exit? It doesn't make sense when we\nstart browsers or pagers, but for hooks and helpers (which are probably\nthe majority of started processes) cascading program termination makes\nsense, no?\n\nRené\n\n"},{"id":"410940","messageId":"X8Hub2OeFy3V6MhV@coredump.intra.peff.net","threadId":"54675","inReplyTo":"d0fcfa7f-9835-a9e6-c4a4-af4de177ff8c@web.de","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-28T06:30:07Z","receivedAt":"2020-11-28T22:16:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 27, 2020 at 09:43:06PM +0100, René Scharfe wrote:\n\n> > [...zombie processes...]\n> OK, so overall the situation sounds a bit messy to me and perhaps\n> there's room for improvement, but I agree now that we can leave the\n> specialists (init, tini) to deal with our zombies.\n\nKeep in mind we are not changing anything here, either. clean_on_exit is\nkicking in when we already would be exiting without calling wait(). We\ncould _also_ instruct run-command to wait, but nobody seems to be\ncomplaining about it.\n\n> With the debug patch above and GIT_DEBUG_ABANDON_CHILD=git-upload-pack I\n> need the following patch get rid of the spawned process:\n\nI don't think that is an interesting case, though. We've been discussing\nthe spawn between upload-pack and its child pack-objects, but here\nyou're running a bogus infinite loop instead of upload-pack. It will\nspin forever, because the client is expecting it to say something.\n\nIn a normal setup, a severing of the connection between the client and\nupload-pack will be noticed quickly, because each one is always either\ntrying to read from or write to the other (the exception is while\npack-objects is processing without progress meters, but there we send a\nkeep-alive every 5 seconds).\n\nIf one of them were to spin in a true infinite loop due to a bug (but\nnot break the connection), we would wait forever. But the solution there\nis a timeout on inactivity.\n\nSo...\n\n> --- >8 ---\n> \n> diff --git a/connect.c b/connect.c\n> index 8b8f56cf6d..e1b1b73ef5 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -1369,6 +1369,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \n> \t\tconn->use_shell = 1;\n> \t\tconn->in = conn->out = -1;\n> +\t\tconn->clean_on_exit = 1;\n> \t\tif (protocol == PROTO_SSH) {\n> \t\t\tchar *ssh_host = hostandport;\n> \t\t\tconst char *port = NULL;\n> \n> --- 8< ---\n\nI don't think there's much point here. If the client side dies, it will\nclose the socket, and upload-pack would notice anyway. It's only your\nfake \"spin without doing any I/O\" patch that misbehaves.\n\nMoreover, this wouldn't do _anything_ in many cases, because upload-pack\nis going to be on the far side of a network socket. So really you'd be\nkilling ssh here for those cases (which then likewise would close the\nsocket, but this isn't necessary because ssh will also exit when it sees\nus close _our_ end). And it would do nothing at all for git-over-http,\ngit://, etc.\n\n> So is there a downside to clean_on_exit? It doesn't make sense when we\n> start browsers or pagers, but for hooks and helpers (which are probably\n> the majority of started processes) cascading program termination makes\n> sense, no?\n\nIn general, no, I suspect clean_on_exit wouldn't be hurting anything if\nwe used it more. But generally we get a natural cascade of termination\nbecause exiting processes close the input and output pipes going to the\nother sub-processes.\n\n-Peff\n"},{"id":"411065","messageId":"X8Yz0bY1LOHpKxKA@coredump.intra.peff.net","threadId":"54675","inReplyTo":"bd2c4577-4c8c-851c-6045-ba4b306ca612@web.de","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-01T12:15:13Z","receivedAt":"2020-12-01T12:16:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 26, 2020 at 09:04:35PM +0100, René Scharfe wrote:\n\n> >> We spawn an external pack-objects process to actually send objects to\n> >> the remote side. If we are killed by a signal during this process,\n> >> then pack-objects may continue to run. As soon as it starts producing\n> >> output for the pack, it will see a failure writing to upload-pack and\n> >> exit itself. But before then, it may do significant work traversing\n> >> the object graph, compressing deltas, etc, which will all be\n> >> pointless. So let's make sure to kill as soon as we know that the\n> >> caller will not read the result.\n> >\n> > Thanks, that reads well.\n> >\n> The patch is trivial, you don't need my sign-off. You could record Peff\n> as its author, as he contributed the most to the version in seen.\n\nI didn't want this topic to be forgotten, so here it is with me as the\nauthor, my signoff, and an overview of the reproduction in the commit\nmessage.\n\n(I am perfectly happy for René to be author, but I am mainly interested\nin resolving the signoff issue; I agree most of the work was in the\ndiagnosis, and I did re-type the single line all by myself ;) ).\n\n-- >8 --\nSubject: [PATCH] upload-pack: kill pack-objects helper on signal or exit\n\nWe spawn an external pack-objects process to actually send objects to\nthe remote side. If we are killed by a signal during this process, then\npack-objects may continue to run. As soon as it starts producing output\nfor the pack, it will see a failure writing to upload-pack and exit\nitself. But before then, it may do significant work traversing the\nobject graph, compressing deltas, etc, which will all be pointless. So\nlet's make sure to kill as soon as we know that the caller will not read\nthe result.\n\nThere's no test here, since it's inherently racy, but here's an easy\nreproduction is on a large-ish repo like linux.git:\n\n - make sure you don't have pack bitmaps (since they make the enumerating\n phase go quickly). For linux.git it takes ~30s or so to walk the\n whole graph on my machine.\n\n - run \"git clone --no-local -q . dst\"; the \"-q\" is important because\n if pack-objects is writing progress to upload-pack (to get\n multiplexed over the sideband to the client), then it will notice\n pretty quickly the failure to write to stderr\n\n - kill the client-side clone process in another terminal (don't use\n ^C, as that will send SIGINT to all of the processes)\n\n - run \"ps au | grep git\" or similar to observe upload-pack dying\n within 5 seconds (it will send a keepalive that will notice the\n client has gone away)\n\n - but you'll still see pack-objects consuming 100% CPU (and 1GB+ of\n RAM) during the traversal and delta compression phases. It will exit\n as soon as it starts to write the pack (when it will notice that\n upload-pack went away).\n\nWith this patch, pack-objects exits as soon as upload-pack does.\n\nSigned-off-by: Jeff King \n---\n upload-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5dc8e1f844..1006bebd50 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -321,6 +321,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n+\tpack_objects.clean_on_exit = 1;\n \n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n-- \n2.29.2.893.g57eb4d1d5a\n\n"},{"id":"411153","messageId":"ff05986b-5506-3ef1-68bc-e6b46ddf8b98@web.de","threadId":"54675","inReplyTo":"X8Yz0bY1LOHpKxKA@coredump.intra.peff.net","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-12-02T11:45:34Z","receivedAt":"2020-12-02T11:47:56Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.12.20 um 13:15 schrieb Jeff King:\n> On Thu, Nov 26, 2020 at 09:04:35PM +0100, René Scharfe wrote:\n>> The patch is trivial, you don't need my sign-off. You could record Peff\n>> as its author, as he contributed the most to the version in seen.\n>\n> I didn't want this topic to be forgotten, so here it is with me as the\n> author, my signoff, and an overview of the reproduction in the commit\n> message.\n\nThank you!\n\nRené\n"},{"id":"411179","messageId":"xmqq1rg7n9de.fsf@gitster.c.googlers.com","threadId":"54675","inReplyTo":"ff05986b-5506-3ef1-68bc-e6b46ddf8b98@web.de","subject":"Re: Bug report: orphaned pack-objects after killing upload-pack on [","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-02T22:14:37Z","receivedAt":"2020-12-02T22:15:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 01.12.20 um 13:15 schrieb Jeff King:\n>> On Thu, Nov 26, 2020 at 09:04:35PM +0100, René Scharfe wrote:\n>>> The patch is trivial, you don't need my sign-off. You could record Peff\n>>> as its author, as he contributed the most to the version in seen.\n>>\n>> I didn't want this topic to be forgotten, so here it is with me as the\n>> author, my signoff, and an overview of the reproduction in the commit\n>> message.\n>\n> Thank you!\n\nThanks, both.\n"}]}