{"thread":{"id":"48275","subject":"[PATCH 0/2] Fix early EOF with GfW daemon","startedAt":"2018-04-12T21:24:51Z","lastAt":"2018-04-19T23:18:06Z","messageCount":15,"participants":["Kim Gybels","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"344561","messageId":"20180412210757.7792-1-kgybels@infogroep.be","threadId":"48275","inReplyTo":null,"subject":"[PATCH 0/2] Fix early EOF with GfW daemon","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-04-12T21:07:55Z","receivedAt":"2018-04-12T21:24:51Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"It has been reported [1] that cloning from a Git-for-Windows daemon would\nsometimes fail with an early EOF error:\n\n  $ git clone git://server/test\n  Cloning into 'test'...\n  remote: Counting objects: 36, done.\n  remote: Compressing objects: 100% (24/24), done.\n  fatal: read error: Invalid argument\n  fatal: early EOF\n  fatal: index-pack failed\n\nThese patches solve the issue by only changing git-daemon, its child processes\ncan remain unaware that stdin/stdout are actually network connections.\n\n[1] https://github.com/git-for-windows/git/issues/304\n\nKim Gybels (2):\n  daemon: use timeout for uninterruptible poll\n  daemon: graceful shutdown of client connection\n\n daemon.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\n-- \n2.17.0.windows.1\n\n"},{"id":"344562","messageId":"20180412210757.7792-2-kgybels@infogroep.be","threadId":"48275","inReplyTo":"20180412210757.7792-1-kgybels@infogroep.be","subject":"[PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-04-12T21:07:56Z","receivedAt":"2018-04-12T21:24:53Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"The poll provided in compat/poll.c is not interrupted by receiving\nSIGCHLD. Use a timeout for cleaning up dead children in a timely manner.\n\nSigned-off-by: Kim Gybels <kgybels@infogroep.be>\n---\n daemon.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex fe833ea7de..6dc95c1b2f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1147,6 +1147,7 @@ static int service_loop(struct socketlist *socklist)\n {\n \tstruct pollfd *pfd;\n \tint i;\n+\tint poll_timeout = -1;\n \n \tpfd = xcalloc(socklist->nr, sizeof(struct pollfd));\n \n@@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)\n \t\tint i;\n \n \t\tcheck_dead_children();\n-\n-\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n+#ifdef NO_POLL\n+\t\tpoll_timeout = live_children ? 100 : -1;\n+#endif\n+\t\tint ret = poll(pfd, socklist->nr, poll_timeout);\n+\t\tif  (ret == 0) {\n+\t\t\tcontinue;\n+\t\t} else if (ret < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n \t\t\t\t      strerror(errno));\n-- \n2.17.0.windows.1\n\n"},{"id":"344563","messageId":"20180412210757.7792-3-kgybels@infogroep.be","threadId":"48275","inReplyTo":"20180412210757.7792-1-kgybels@infogroep.be","subject":"[PATCH 2/2] daemon: graceful shutdown of client connection","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-04-12T21:07:57Z","receivedAt":"2018-04-12T21:24:56Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"On Windows, a connection is shutdown when the last open handle to it is\nclosed. When that last open handle is stdout of our child process, an\nabortive shutdown is triggered when said process exits. Ensure a\ngraceful shutdown of the client connection by keeping an open handle\nuntil we detect our child process has finished. This allows all the data\nto be sent to the client, instead of being discarded.\n\nFixes https://github.com/git-for-windows/git/issues/304\n\nSigned-off-by: Kim Gybels <kgybels@infogroep.be>\n---\n daemon.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 6dc95c1b2f..97fadd62d1 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -834,9 +834,10 @@ static struct child {\n \tstruct child *next;\n \tstruct child_process cld;\n \tstruct sockaddr_storage address;\n+\tint connection;\n } *firstborn;\n \n-static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)\n+static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen, int connection)\n {\n \tstruct child *newborn, **cradle;\n \n@@ -844,6 +845,7 @@ static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_\n \tlive_children++;\n \tmemcpy(&newborn->cld, cld, sizeof(*cld));\n \tmemcpy(&newborn->address, addr, addrlen);\n+\tnewborn->connection = connection;\n \tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n \t\tif (!addrcmp(&(*cradle)->address, &newborn->address))\n \t\t\tbreak;\n@@ -888,6 +890,7 @@ static void check_dead_children(void)\n \t\t\t*cradle = blanket->next;\n \t\t\tlive_children--;\n \t\t\tchild_process_clear(&blanket->cld);\n+\t\t\tclose(blanket->connection);\n \t\t\tfree(blanket);\n \t\t} else\n \t\t\tcradle = &blanket->next;\n@@ -928,13 +931,13 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t}\n \n \tcld.argv = cld_argv.argv;\n-\tcld.in = incoming;\n+\tcld.in = dup(incoming);\n \tcld.out = dup(incoming);\n \n \tif (start_command(&cld))\n \t\tlogerror(\"unable to fork\");\n \telse\n-\t\tadd_child(&cld, addr, addrlen);\n+\t\tadd_child(&cld, addr, addrlen, incoming);\n }\n \n static void child_handler(int signo)\n-- \n2.17.0.windows.1\n\n"},{"id":"344608","messageId":"nycvar.QRO.7.76.6.1804131433250.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48275","inReplyTo":"20180412210757.7792-2-kgybels@infogroep.be","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-13T12:36:16Z","receivedAt":"2018-04-13T12:37:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kim,\n\nOn Thu, 12 Apr 2018, Kim Gybels wrote:\n\n> The poll provided in compat/poll.c is not interrupted by receiving\n> SIGCHLD. Use a timeout for cleaning up dead children in a timely manner.\n\nMaybe say \"When using this poll emulation, use a timeout ...\"?\n\n> diff --git a/daemon.c b/daemon.c\n> index fe833ea7de..6dc95c1b2f 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1147,6 +1147,7 @@ static int service_loop(struct socketlist *socklist)\n>  {\n>  \tstruct pollfd *pfd;\n>  \tint i;\n> +\tint poll_timeout = -1;\n\nJust reuse the line above:\n\n\tint poll_timeout = -1, i;\n\n> @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)\n>  \t\tint i;\n>  \n>  \t\tcheck_dead_children();\n> -\n> -\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> +#ifdef NO_POLL\n> +\t\tpoll_timeout = live_children ? 100 : -1;\n> +#endif\n> +\t\tint ret = poll(pfd, socklist->nr, poll_timeout);\n> +\t\tif  (ret == 0) {\n> +\t\t\tcontinue;\n> +\t\t} else if (ret < 0) {\n\nI would find it a bit easier on the eyes if this did not use curlies, and\ndropped the unnecessary `else` (`continue` will take care of that):\n\n\t\tif (!ret)\n\t\t\tcontinue;\n\t\tif (ret < 0)\n\t\t\t[...]\n\nThank you for working on this!\n\nCiao,\nDscho\n"},{"id":"344609","messageId":"nycvar.QRO.7.76.6.1804131440100.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48275","inReplyTo":"20180412210757.7792-3-kgybels@infogroep.be","subject":"Re: [PATCH 2/2] daemon: graceful shutdown of client connection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-13T13:03:21Z","receivedAt":"2018-04-13T13:04:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kim,\n\nOn Thu, 12 Apr 2018, Kim Gybels wrote:\n\n> On Windows, a connection is shutdown when the last open handle to it is\n> closed. When that last open handle is stdout of our child process, an\n> abortive shutdown is triggered when said process exits. Ensure a\n> graceful shutdown of the client connection by keeping an open handle\n> until we detect our child process has finished. This allows all the data\n> to be sent to the client, instead of being discarded.\n\nNice explanation!\n\n> @@ -928,13 +931,13 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n>  \t}\n>  \n>  \tcld.argv = cld_argv.argv;\n> -\tcld.in = incoming;\n> +\tcld.in = dup(incoming);\n\nAt first I was worried that somebody might want to remove this in the\nfuture, but then I saw this line (which also calls dup()):\n\n>  \tcld.out = dup(incoming);\n>  \n>  \tif (start_command(&cld))\n>  \t\tlogerror(\"unable to fork\");\n>  \telse\n> -\t\tadd_child(&cld, addr, addrlen);\n> +\t\tadd_child(&cld, addr, addrlen, incoming);\n>  }\n>  \n>  static void child_handler(int signo)\n\nNice work!\n\nI wonder whether you found a reliable way to trigger this? It would be\nnice to have a regression test for this.\n\nCiao,\nDscho\n"},{"id":"344735","messageId":"20180415170859.GA30197@infogroep.be","threadId":"48275","inReplyTo":"nycvar.QRO.7.76.6.1804131433250.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-04-15T17:08:59Z","receivedAt":"2018-04-15T17:19:06Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"On (13/04/18 14:36), Johannes Schindelin wrote:\n> > The poll provided in compat/poll.c is not interrupted by receiving\n> > SIGCHLD. Use a timeout for cleaning up dead children in a timely manner.\n> \n> Maybe say \"When using this poll emulation, use a timeout ...\"?\n\nI will rewrite the commit message when I reroll the patch. Calling the\npoll \"uninterruptible\" might be wrong as well, although the poll\ndoesn't return with EINTR when a child process terminates, it might\nstill be interruptible in other ways. On a related note, the handler\nfor SIGCHLD is simply not called in Git-for-Windows' daemon.\n\n> > diff --git a/daemon.c b/daemon.c\n> > index fe833ea7de..6dc95c1b2f 100644\n> > --- a/daemon.c\n> > +++ b/daemon.c\n> > @@ -1147,6 +1147,7 @@ static int service_loop(struct socketlist *socklist)\n> >  {\n> >  \tstruct pollfd *pfd;\n> >  \tint i;\n> > +\tint poll_timeout = -1;\n> \n> Just reuse the line above:\n> \n> \tint poll_timeout = -1, i;\n\nSure.\n\n> > @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)\n> >  \t\tint i;\n> >  \n> >  \t\tcheck_dead_children();\n> > -\n> > -\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> > +#ifdef NO_POLL\n> > +\t\tpoll_timeout = live_children ? 100 : -1;\n> > +#endif\n> > +\t\tint ret = poll(pfd, socklist->nr, poll_timeout);\n> > +\t\tif  (ret == 0) {\n> > +\t\t\tcontinue;\n> > +\t\t} else if (ret < 0) {\n> \n> I would find it a bit easier on the eyes if this did not use curlies, and\n> dropped the unnecessary `else` (`continue` will take care of that):\n> \n> \t\tif (!ret)\n> \t\t\tcontinue;\n> \t\tif (ret < 0)\n> \t\t\t[...]\n\nFunny, that's how I would normally write it, if I wasn't so focused on\ntrying to follow the coding quidelines. While I'm at it, I will also\nfix that sneaky double space after the if.\n\nIs it ok to add the timeout for all platforms using the poll\nemulation, since I only tested for Windows?\n\nBest regards,\nKim\n"},{"id":"344748","messageId":"20180415202122.GA4657@infogroep.be","threadId":"48275","inReplyTo":"nycvar.QRO.7.76.6.1804131440100.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 2/2] daemon: graceful shutdown of client connection","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-04-15T20:21:22Z","receivedAt":"2018-04-15T20:29:22Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"On (13/04/18 15:03), Johannes Schindelin wrote:\n> I wonder whether you found a reliable way to trigger this? It would be\n> nice to have a regression test for this.\n\nOn my system, it reproduced reliably using Oleg's example [1], below is my bash\nversion of it.\n\nScript to generate repository with some history:\n\n  $ cat example.sh\n  #!/bin/bash\n  \n  git init example\n  cd example\n  \n  git --help > foo.txt\n  \n  for i in $(seq 1 12); do\n      cat foo.txt foo.txt > bar.txt\n      mv bar.txt foo.txt\n      git add foo.txt\n      git commit -m v$i\n  done\n  \n  $ ./example.sh\n  Initialized empty Git repository in C:/git/bug/example/.git/\n  [master (root-commit) 2e44b4a] v1\n   1 file changed, 84 insertions(+)\n   create mode 100644 foo.txt\n  [master 9791332] v2\n   1 file changed, 84 insertions(+)\n  [master 524e672] v3\n   1 file changed, 168 insertions(+)\n  [master afec6ef] v4\n   1 file changed, 336 insertions(+)\n  [master 1bcd9cc] v5\n   1 file changed, 672 insertions(+)\n  [master 2f38a8e] v6\n   1 file changed, 1344 insertions(+)\n  [master 33382fe] v7\n   1 file changed, 2688 insertions(+)\n  [master 6c2cbd6] v8\n   1 file changed, 5376 insertions(+)\n  [master 8d0770f] v9\n   1 file changed, 10752 insertions(+)\n  [master 517d650] v10\n   1 file changed, 21504 insertions(+)\n  [master 9e12406] v11\n   1 file changed, 43008 insertions(+)\n  [master 4c4f600] v12\n   1 file changed, 86016 insertions(+)\n\nServer side:\n\n  $ git daemon --verbose --reuseaddr --base-path=$(pwd) --export-all\n  [4760] Ready to rumble\n  [696] Connection from 127.0.0.1:2054\n  [696] unable to set SO_KEEPALIVE on socket: No such file or directory\n  [696] Extended attribute \"host\": 127.0.0.1\n  [696] Request upload-pack for '/example'\n\nClient side:\n\n  $ git clone git://127.0.0.1/example\n  Cloning into 'example'...\n  remote: Counting objects: 36, done.\n  remote: Compressing objects: 100% (24/24), done.\n  fatal: read error: Invalid argument\n  fatal: early EOF\n  fatal: index-pack failed\n\nSystem information:\n\n  $ git --version --build-options\n  git version 2.17.0.windows.1\n  cpu: x86_64\n  built from commit: e7621d891d081acff6acd1f0ba6ae0adce06dd09\n  sizeof-long: 4\n  \n  $ cmd.exe /c ver\n  \n  Microsoft Windows [Version 10.0.16299.371]\n\nBest regards,\nKim\n\n[1] https://github.com/git-for-windows/git/issues/304#issuecomment-274266897\n"},{"id":"344758","messageId":"xmqq36zw16gv.fsf@gitster-ct.c.googlers.com","threadId":"48275","inReplyTo":"20180412210757.7792-2-kgybels@infogroep.be","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-15T21:54:08Z","receivedAt":"2018-04-15T21:54:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kim Gybels <kgybels@infogroep.be> writes:\n\n> The poll provided in compat/poll.c is not interrupted by receiving\n> SIGCHLD. Use a timeout for cleaning up dead children in a timely manner.\n\nI think you identified the problem and diagnosed it correctly, but I\nfind that the change proposed here introduces a severe layering\nviolation.  The code is still calling what is called poll(), which\nshould not have such a broken semantics.\n\nThe ideal solution would be to fix the emulation so that it also\nproperly works for reaping a dead child process, but if that is not\npossible, another solution that does not break the API layering\nwould probably be to introduce our own version of something similar\nto poll() that helps various platforms that cannot implement the\nreal poll() faithfully for whatever reason.  Such an xpoll() API\nfunction we introduce (and implement in compat/poll.c) may take, in\naddition to the usual parameters to reall poll(), the value of\nlive_children we have at this call site.  With that\n\n - On platforms whose poll() does work correctly for culling dead\n   children will just ignore the live_children paramater in its\n   implementation of xpoll()\n\n - On other platforms, it will shorten the timeout depending on the\n   need to cull dead children, just like your patch did.\n\nThanks.\n\n\n>\n> Signed-off-by: Kim Gybels <kgybels@infogroep.be>\n> ---\n>  daemon.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index fe833ea7de..6dc95c1b2f 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1147,6 +1147,7 @@ static int service_loop(struct socketlist *socklist)\n>  {\n>  \tstruct pollfd *pfd;\n>  \tint i;\n> +\tint poll_timeout = -1;\n>  \n>  \tpfd = xcalloc(socklist->nr, sizeof(struct pollfd));\n>  \n> @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)\n>  \t\tint i;\n>  \n>  \t\tcheck_dead_children();\n> -\n> -\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> +#ifdef NO_POLL\n> +\t\tpoll_timeout = live_children ? 100 : -1;\n> +#endif\n> +\t\tint ret = poll(pfd, socklist->nr, poll_timeout);\n> +\t\tif  (ret == 0) {\n> +\t\t\tcontinue;\n> +\t\t} else if (ret < 0) {\n>  \t\t\tif (errno != EINTR) {\n>  \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n>  \t\t\t\t      strerror(errno));\n"},{"id":"344760","messageId":"xmqqvacsyv1y.fsf@gitster-ct.c.googlers.com","threadId":"48275","inReplyTo":"xmqq36zw16gv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-15T22:16:41Z","receivedAt":"2018-04-15T22:16:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think you identified the problem and diagnosed it correctly, but I\n> find that the change proposed here introduces a severe layering\n> violation.  The code is still calling what is called poll(), which\n> should not have such a broken semantics.\n\nI only mentioned a piece of fact (i.e. \"the code calls poll() after\nthe patch\"), but I guess I should have made it clear what makes that\na bad thing.  Future readers of the code in daemon.c are required to\nbe aware of the limitation of some poll() emulation; they cannot\n\"optimize\" out and made the code unware of the (non-)existence of\nremaining children, for example.  When the callsite uses poll(),\nthose who know how poll() ought to work won't be.  The reason why\nthe xpoll() I mentioned as a possible alternative would be better is\nbecause they will learn why we do not use normal poll() there and\nwhy we maintain and pass live_children (and those who cut and paste\nwithout understanding the existing code _will_ copy the calling site\nof xpoll(), which will automatically copy the need to maintain the\nnumber of remaining children ;-).\n\n\n"},{"id":"345017","messageId":"nycvar.QRO.7.76.6.1804182251070.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48275","inReplyTo":"xmqq36zw16gv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-18T21:07:18Z","receivedAt":"2018-04-18T21:08:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 16 Apr 2018, Junio C Hamano wrote:\n\n> Kim Gybels <kgybels@infogroep.be> writes:\n> \n> > The poll provided in compat/poll.c is not interrupted by receiving\n> > SIGCHLD. Use a timeout for cleaning up dead children in a timely manner.\n> \n> I think you identified the problem and diagnosed it correctly, but I\n> find that the change proposed here introduces a severe layering\n> violation.  The code is still calling what is called poll(), which\n> should not have such a broken semantics.\n\nWhile I have sympathy for your desire to apply pure POSIX functionality,\nthe reality is that we do not have this luxury. Not if we want to support\nGit on the still most prevalent development platform: Windows. On Windows,\nyou simply do not have that poll() that you are looking for.\n\nIn particular, there is no signal handling of the type you seem to want to\nrequire.\n\nAs to the layering violation you mention, first a HN quote, just to loosen\nthe mood, and to at least partially ease the blow delivered by your mail:\n\n\tThere is no such thing as a layering violation. You should be\n\timmediately suspicious of anyone who claims that there are such\n\tthings.\n\n;-)\n\nSeriously again. If you care to have a look at the patch, you will see\nthat the loop (which will now benefit from Kim's timeout on platforms\nwithout POSIX signal handling) *already* contains that call to\nreap_dead_children().\n\nIn other words, you scolded Kim for something that this patch did not\nintroduce, but which was already there.\n\nUnless I am misunderstanding violently what you say, that is, in which\ncase I would like to ask for a clarification why this patch (which does\nnot change a thing unless NO_POLL is defined!) must be rejected, and while\nat it, I would like to ask you how introducing a layer of indirection with\na full new function that is at least moderately misleading (as it would be\nnamed xpoll() despite your desire that it should do things that poll()\ndoes *not* do) would be preferable to this here patch that changes but a\nfew lines to introduce a regular heartbeat check for platforms that\nrequire it?\n\nThank you,\nDscho\n"},{"id":"345020","messageId":"nycvar.QRO.7.76.6.1804182307450.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48275","inReplyTo":"20180415170859.GA30197@infogroep.be","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-18T21:16:03Z","receivedAt":"2018-04-18T21:16:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kim,\n\nOn Sun, 15 Apr 2018, Kim Gybels wrote:\n\n> On (13/04/18 14:36), Johannes Schindelin wrote:\n> > > The poll provided in compat/poll.c is not interrupted by receiving\n> > > SIGCHLD. Use a timeout for cleaning up dead children in a timely\n> > > manner.\n> > \n> > Maybe say \"When using this poll emulation, use a timeout ...\"?\n> \n> I will rewrite the commit message when I reroll the patch. Calling the\n> poll \"uninterruptible\" might be wrong as well, although the poll\n> doesn't return with EINTR when a child process terminates, it might\n> still be interruptible in other ways. On a related note, the handler\n> for SIGCHLD is simply not called in Git-for-Windows' daemon.\n\nRight. There is no signal infrastructure on Windows that is an exact\nequivalent of what Junio desires.\n\n> > > @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)\n> > >  \t\tint i;\n> > >  \n> > >  \t\tcheck_dead_children();\n> > > -\n> > > -\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> > > +#ifdef NO_POLL\n> > > +\t\tpoll_timeout = live_children ? 100 : -1;\n> > > +#endif\n> > > +\t\tint ret = poll(pfd, socklist->nr, poll_timeout);\n> > > +\t\tif  (ret == 0) {\n> > > +\t\t\tcontinue;\n> > > +\t\t} else if (ret < 0) {\n> > \n> > I would find it a bit easier on the eyes if this did not use curlies, and\n> > dropped the unnecessary `else` (`continue` will take care of that):\n> > \n> > \t\tif (!ret)\n> > \t\t\tcontinue;\n> > \t\tif (ret < 0)\n> > \t\t\t[...]\n> \n> Funny, that's how I would normally write it, if I wasn't so focused on\n> trying to follow the coding quidelines. While I'm at it, I will also\n> fix that sneaky double space after the if.\n\n:-)\n\n> Is it ok to add the timeout for all platforms using the poll\n> emulation, since I only tested for Windows?\n\nFrom my reading of the patch, it changes only one thing, and only in the\ncase that the developer asked to build with NO_POLL (which means that the\nplatform does not have a native poll()): instead of waiting indefinitely,\nthe poll() call is interrupted in regular intervals to give\nreap_dead_children() a chance to clean up.\n\nAnd that's all it does.\n\nSo it is a simply heartbeat for platforms that require it, and that\nheartbeat would not even hurt any platform that would *not* require it.\n\nIn short: from my point of view, it is fine to add the timeout for all\nNO_POLL platforms, even if it was only tested on Windows.\n\nOf course, we *do* know that there is one other user of NO_POLL: the\nNonStop platform.\n\nRandall, would you mind testing these two patches on NonStop?\n\nThanks,\nJohannes\n"},{"id":"345028","messageId":"nycvar.QRO.7.76.6.1804182316420.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48275","inReplyTo":"20180415202122.GA4657@infogroep.be","subject":"Re: [PATCH 2/2] daemon: graceful shutdown of client connection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-18T21:48:22Z","receivedAt":"2018-04-18T21:49:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kim,\n\nOn Sun, 15 Apr 2018, Kim Gybels wrote:\n\n> On (13/04/18 15:03), Johannes Schindelin wrote:\n> > I wonder whether you found a reliable way to trigger this? It would be\n> > nice to have a regression test for this.\n> \n> On my system, it reproduced reliably using Oleg's example [1], below is\n> my bash version of it.\n\nOkay.\n\n> Script to generate repository with some history:\n> \n>   $ cat example.sh\n>   #!/bin/bash\n>   \n>   git init example\n>   cd example\n>   \n>   git --help > foo.txt\n>   \n>   for i in $(seq 1 12); do\n>       cat foo.txt foo.txt > bar.txt\n>       mv bar.txt foo.txt\n>       git add foo.txt\n>       git commit -m v$i\n>   done\n\nOkay, so this sets up a minimal repository with a moderate size, inflating\nfoo.txt from the initial Git help text by factor 2**12 = 4096. The help\ntext is around 2kB, so we end up with an ~8MB large file in the end that\ngrew exponentially.\n\n>   $ ./example.sh\n>   Initialized empty Git repository in C:/git/bug/example/.git/\n>   [master (root-commit) 2e44b4a] v1\n>    1 file changed, 84 insertions(+)\n>    create mode 100644 foo.txt\n>   [master 9791332] v2\n>    1 file changed, 84 insertions(+)\n>   [master 524e672] v3\n>    1 file changed, 168 insertions(+)\n>   [master afec6ef] v4\n>    1 file changed, 336 insertions(+)\n>   [master 1bcd9cc] v5\n>    1 file changed, 672 insertions(+)\n>   [master 2f38a8e] v6\n>    1 file changed, 1344 insertions(+)\n>   [master 33382fe] v7\n>    1 file changed, 2688 insertions(+)\n>   [master 6c2cbd6] v8\n>    1 file changed, 5376 insertions(+)\n>   [master 8d0770f] v9\n>    1 file changed, 10752 insertions(+)\n>   [master 517d650] v10\n>    1 file changed, 21504 insertions(+)\n>   [master 9e12406] v11\n>    1 file changed, 43008 insertions(+)\n>   [master 4c4f600] v12\n>    1 file changed, 86016 insertions(+)\n> \n> Server side:\n> \n>   $ git daemon --verbose --reuseaddr --base-path=$(pwd) --export-all\n>   [4760] Ready to rumble\n>   [696] Connection from 127.0.0.1:2054\n>   [696] unable to set SO_KEEPALIVE on socket: No such file or directory\n>   [696] Extended attribute \"host\": 127.0.0.1\n>   [696] Request upload-pack for '/example'\n\nI guess apart from the generated repo size (which would make this an\nexpensive test in and of itself), the fact that we have to run the daemon\n(and that lib-git-daemon.sh requires the PIPE prerequisite which we\ndisabled on Windows) makes it hard to turn this into a regular regression\ntest in t/.\n\nAlthough we *could* of course introduce a test that does not use\nlib-git-daemon.sh and is hidden behind the EXPENSIVE prerequisite or some\nsuch.\n\nBTW I just tested this, and it indeed fixes the problem, so I am eager to\nship it to the Git for Windows users. Let's see whether we can convince\nJunio that your quite small and easy-to-review patches are not too much of\na maintenance burden, and the fact that they fix a long-standing bug\nshould help.\n\nBTW I had to apply this to build with DEVELOPER=1:\n\ndiff --git a/daemon.c b/daemon.c\nindex 97fadd62d10..1cc901e9739 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1162,13 +1162,13 @@ static int service_loop(struct socketlist\n*socklist)\n \tsignal(SIGCHLD, child_handler);\n \n \tfor (;;) {\n-\t\tint i;\n+\t\tint i, ret;\n \n \t\tcheck_dead_children();\n #ifdef NO_POLL\n \t\tpoll_timeout = live_children ? 100 : -1;\n #endif\n-\t\tint ret = poll(pfd, socklist->nr, poll_timeout);\n+\t\tret = poll(pfd, socklist->nr, poll_timeout);\n \t\tif  (ret == 0) {\n \t\t\tcontinue;\n \t\t} else if (ret < 0) {\n\nWould you mind squashing that in?\n\nThanks,\nDscho\n"},{"id":"345029","messageId":"xmqqy3hkfais.fsf@gitster-ct.c.googlers.com","threadId":"48275","inReplyTo":"nycvar.QRO.7.76.6.1804182251070.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-18T21:51:55Z","receivedAt":"2018-04-18T21:52:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Unless I am misunderstanding violently what you say, that is, in which\n> case I would like to ask for a clarification why this patch (which does\n> not change a thing unless NO_POLL is defined!) must be rejected, and while\n> at it, I would like to ask you how introducing a layer of indirection with\n> a full new function that is at least moderately misleading (as it would be\n> named xpoll() despite your desire that it should do things that poll()\n> does *not* do) would be preferable to this here patch that changes but a\n> few lines to introduce a regular heartbeat check for platforms that\n\nOur xwrite() and other xfoo() are to \"fix\" undesirable aspect of the\nunderlying pure POSIX API to make it more suitable for our codebase.\nWhen pure POSIX poll() that requires the implementing or emulating\nplatform pays attention to the children being waited on is not\nappropriate for the codepath we are using (i.e. the place where the\npatch is touching), it would be in line to introduce a \"fixed\" API\nthat allows us to pass that information, so that we can build on top\nof that abstraction that is *not* pure POSIX abstraction, no?  After\nall, you are the one who constantly whine that Git is implemented on\nPOSIX API and it is inconvenient for other platforms.\n\n"},{"id":"345168","messageId":"20180419213322.GA19500@infogroep.be","threadId":"48275","inReplyTo":"xmqqy3hkfais.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-04-19T21:33:22Z","receivedAt":"2018-04-19T21:33:28Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"On (19/04/18 06:51), Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> > In other words, you scolded Kim for something that this patch did not\n> > introduce, but which was already there.\n\nI didn't feel scolded, just Junio raising a concern about maintainability of\nthe code.\n\n> > Unless I am misunderstanding violently what you say, that is, in which\n> > case I would like to ask for a clarification why this patch (which does\n> > not change a thing unless NO_POLL is defined!) must be rejected, and while\n> > at it, I would like to ask you how introducing a layer of indirection with\n> > a full new function that is at least moderately misleading (as it would be\n> > named xpoll() despite your desire that it should do things that poll()\n> > does *not* do) would be preferable to this here patch that changes but a\n> > few lines to introduce a regular heartbeat check for platforms that\n> \n> Our xwrite() and other xfoo() are to \"fix\" undesirable aspect of the\n> underlying pure POSIX API to make it more suitable for our codebase.\n> When pure POSIX poll() that requires the implementing or emulating\n> platform pays attention to the children being waited on is not\n> appropriate for the codepath we are using (i.e. the place where the\n> patch is touching), it would be in line to introduce a \"fixed\" API\n> that allows us to pass that information, so that we can build on top\n> of that abstraction that is *not* pure POSIX abstraction, no?  After\n> all, you are the one who constantly whine that Git is implemented on\n> POSIX API and it is inconvenient for other platforms.\n\nThere is another issue with the existing code that this new \"xpoll\" will need\nto take into account. If a SIGCHLD arrives between the call to\ncheck_dead_children and poll, the poll will not be interupted by it, resulting\nin the child not being reaped until another child terminates or a client\nconnects. Currently, the effect is just a zombie process for a longer time,\nhowever, the proposed patch (daemon: graceful shutdown of client connection)\nrelies on the cleanup to close the client connection.\n\nWhen I have time, I will reroll including a change to ppoll.\n\n-Kim\n"},{"id":"345174","messageId":"xmqq1sfaeqfr.fsf@gitster-ct.c.googlers.com","threadId":"48275","inReplyTo":"20180419213322.GA19500@infogroep.be","subject":"Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T23:18:00Z","receivedAt":"2018-04-19T23:18:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kim Gybels <kgybels@infogroep.be> writes:\n\n>> > In other words, you scolded Kim for something that this patch did not\n>> > introduce, but which was already there.\n>\n> I didn't feel scolded, just Junio raising a concern about maintainability of\n> the code.\n\nFWIW, I didn't mean to scold, either.\n\nRather I was pointing out that the code already maintains the number\nof remaining children, which means that a more portable abstraction\nthan poll(), if we desired to have one, would merely be one step\naway, as we already know that at least need that information to help\nWindows.\n\n> There is another issue with the existing code that this new\n> \"xpoll\" will need to take into account. If a SIGCHLD arrives\n> between the call to check_dead_children and poll, the poll will\n> not be interupted by it, resulting in the child not being reaped\n> until another child terminates or a client connects. Currently,\n> the effect is just a zombie process for a longer time, however,\n> the proposed patch (daemon: graceful shutdown of client\n> connection) relies on the cleanup to close the client connection.\n\nGood analysis.  That consideration may mean that xpoll() as I\nsuggested is useless as a possible more portable abstraction (or,\n\"an abstraction that is implementable easily in both POSIX and\nnon-POSIX world\"), but I suspect we would still want to have an\ninternal \"portable\" API that serves the purpose similar to how we\nwanted POSIX poll() to serve.  The place the patch is touching is\nnot the only place poll() is used in the codebase, and other places\n(and future ones we would add) may benefit from having one.\n\nThanks for being constructive.\n\n\n"}]}