{"thread":{"id":"14273","subject":"Non-inetd git-daemon hangs in syslog(3)/fclose(3) if --syslog --verbose accessing non-repositories","startedAt":"2008-07-03T12:00:36Z","lastAt":"2008-07-07T06:54:39Z","messageCount":13,"participants":["Brian Foster","Johannes Schindelin","Junio C Hamano","Stephen R. van den Berg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"82105","messageId":"200807031400.36315.brian.foster@innova-card.com","threadId":"14273","inReplyTo":null,"subject":"Non-inetd git-daemon hangs in syslog(3)/fclose(3) if --syslog --verbose accessing non-repositories","fromName":"Brian Foster","fromEmail":"brian.foster@innova-card.com","sentAt":"2008-07-03T12:00:36Z","receivedAt":"2008-07-03T12:00:36Z","isPatch":false,"sender":{"key":"brian.foster@innova-card.com","avatar":null},"body":"\n I've seen several reports of what seems to be the following\n problem, but no fixes.  I do not understand the root-cause.\n I have found, however, what seems to be a work-around.\n\n I'm starting v1.5.2.5 (the Kubuntu 7.10 package) git-daemon\n (as a normal-user, *not* super-user) as:\n\n     export GIT_TRACE=/tmp/LOG-git-daemon\n     exec  \\\n        git daemon --detach --syslog --verbose --base-path=/pub/scm\n\n The repositories being served are simple (non-bare) clones,\n with nothing strange/weird.  The remote machine (CentOS),\n running a self-built un-modified v1.5.5, can access them Ok:\n\n     $ git ls-remote git://SERVER/repo\n     ... works ...\n     $ \n\n However, an invalid path causes both the git-daemon and the\n client to hang (I'm too impatient and do not know if either\n times out):\n\n     $ git ls-remote git://SERVER/repo/garbage\n     ... hangs ...\n\n I can ^C the client, but the server is still hung, and will\n not respond to *any* requests.  I must kill the git-daemon.\n The GIT_TRACE log's contents do not seem to be interesting.\n The last entries in the syslog are:\n\n     git-daemon: [3705] Connection from <REMOTE>\n     git-daemon: [3705] Extended attributes (17 bytes) exist <...>\n     git-daemon: [3705] Request upload-pack for '/repo/garbage'\n     git-daemon: [3705] '/pub/scm/repo/garbage': unable to chdir or not a git archive\n\n Annoyingly, strace(1)ing git-daemon causes the problem to\n vanish!  Everything then seems to be work as expected.\n But now the syslog contains an additional, 5th, line:\n\n     git-daemon: [3705] Disconnected (with error)\n\n (The \"with error\" is present only in the .../garbage case.)\n\n Attaching to a hung git-daemon with strace shows that it's\n hung in futex(2):\n\n     $ strace -p3705\n     Process 3705 attached - interrupt to quit\n     futex(0x2b502cbf3980, FUTEX_WAIT, 2, NULL <unfinished ...>\n     ... hangs ...\n\n Attaching to a hung git-daemon with gdb(1) results in the\n following backtrace:\n\n     (gdb) where\n     #0  0x00002b502c97d1d8 in ?? () from /lib/libc.so.6\n     #1  0x00002b502c913698 in ?? () from /lib/libc.so.6\n     #2  0x00002b502c912960 in realloc () from /lib/libc.so.6\n     #3  0x00002b502c905eb4 in ?? () from /lib/libc.so.6\n     #4  0x00002b502c8fd897 in fclose () from /lib/libc.so.6\n     #5  0x00002b502c96d371 in __vsyslog_chk () from /lib/libc.so.6\n     #6  0x00002b502c96d8a0 in syslog () from /lib/libc.so.6\n     #7  0x0000000000403896 in ?? ()\n     #8  <signal handler called>\n     #9  0x00002b502c9373ab in fork () from /lib/libc.so.6\n     #10 0x0000000000404443 in ?? ()\n     #11 0x0000000000404c99 in ?? ()\n     #12 0x00002b502c8bab44 in __libc_start_main () from /lib/libc.so.6\n     #13 0x0000000000403179 in ?? ()\n     #14 0x00007fff7e63eab8 in ?? ()\n     #15 0x0000000000000000 in ?? ()\n     (gdb) \n\n It's fairly clear it's hung doing the syslog(3) of the (missing)\n \"Disconnected (with error)\" message.  A interesting point is the\n SIGCHLD handler in (git-)daemon.c appears to have been called\n before the parent's fork(2) returned.  I presume there is a race\n here, but admit I do not see it.  This (broadly) makes sense; the\n server is a Very Fast machine.  And strace'ing slows things down.\n\n The workaround is to omit --verbose and hence never try to syslog\n the \"Disconnected ...\" message.\n\ncheers!\n\t-blf-\n\n-- \n“How many surrealists does it take to   | Brian Foster\n change a lightbulb? Three. One calms   | somewhere in south of France\n the warthog, and two fill the bathtub  |   Stop E$$o (ExxonMobil)!\n with brightly-coloured machine tools.” |      http://www.stopesso.com\n"},{"id":"82119","messageId":"alpine.DEB.1.00.0807031343440.9925@racer","threadId":"14273","inReplyTo":"200807031400.36315.brian.foster@innova-card.com","subject":"Re: Non-inetd git-daemon hangs in syslog(3)/fclose(3) if --syslog --verbose accessing non-repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-03T12:45:28Z","receivedAt":"2008-07-03T12:45:28Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 3 Jul 2008, Brian Foster wrote:\n\n>  I've seen several reports of what seems to be the following\n>  problem, but no fixes.\n>\n> [describes that git-daemon -v syslog()s in a signal handler, which is \n>  unsupported]\n\nI reported this bug earlier, and my workaround was to comment out the \nsyslog() in the signal handler, but I have no real fix for that, either.\n\nUnfortunately, the wise people on this list did not have an idea either, \nat least they did not share it with me.\n\nCiao,\nDscho\n"},{"id":"82129","messageId":"200807031552.26615.brian.foster@innova-card.com","threadId":"14273","inReplyTo":"alpine.DEB.1.00.0807031343440.9925@racer","subject":"Re: Non-inetd git-daemon hangs in syslog(3)/fclose(3) if --syslog --verbose accessing non-repositories","fromName":"Brian Foster","fromEmail":"brian.foster@innova-card.com","sentAt":"2008-07-03T13:52:26Z","receivedAt":"2008-07-03T13:52:26Z","isPatch":false,"sender":{"key":"brian.foster@innova-card.com","avatar":null},"body":"On Thursday 03 July 2008 Johannes Schindelin wrote:\n> On Thu, 3 Jul 2008, Brian Foster wrote:\n> >[... describes that git-daemon -v syslog()s in a signal handler,\n> >  which is unsupported ...]\n> \n> I reported this bug earlier [ ... ]\n\n Ah, yes, I've (now) found the (long!) thread,\n about log-rotation (which, as you observe, is\n not the problem).  Sorry for the duplication.\n\ncheers!\n\t-blf-\n\n-- \n“How many surrealists does it take to   | Brian Foster\n change a lightbulb? Three. One calms   | somewhere in south of France\n the warthog, and two fill the bathtub  |   Stop E$$o (ExxonMobil)!\n with brightly-coloured machine tools.” |      http://www.stopesso.com\n"},{"id":"82130","messageId":"alpine.DEB.1.00.0807031531320.9925@racer","threadId":"14273","inReplyTo":"200807031552.26615.brian.foster@innova-card.com","subject":"Re: Non-inetd git-daemon hangs in syslog(3)/fclose(3) if --syslog --verbose accessing non-repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-03T14:32:23Z","receivedAt":"2008-07-03T14:32:23Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 3 Jul 2008, Brian Foster wrote:\n\n> On Thursday 03 July 2008 Johannes Schindelin wrote:\n> > On Thu, 3 Jul 2008, Brian Foster wrote:\n> > >[... describes that git-daemon -v syslog()s in a signal handler,\n> > >  which is unsupported ...]\n> > \n> > I reported this bug earlier [ ... ]\n> \n>  Ah, yes, I've (now) found the (long!) thread, about log-rotation \n>  (which, as you observe, is not the problem).\n\nYeah, sorry, should have mentioned that.\n\n> Sorry for the duplication.\n\nNo need to be sorry.  It may raise awareness so much that somebody gets a \nclever idea how to cope with it.\n\nCiao,\nDscho\n"},{"id":"82131","messageId":"alpine.DEB.1.00.0807031624020.9925@racer","threadId":"14273","inReplyTo":"alpine.DEB.1.00.0807031531320.9925@racer","subject":"[PATCH] git daemon: avoid calling syslog() from a signal handler","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-03T15:27:24Z","receivedAt":"2008-07-03T15:27:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nSignal handlers should never call syslog(), as that can raise signals\nof its own.\n\nInstead, call the syslog() from the master process.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tOn Thu, 3 Jul 2008, Johannes Schindelin wrote:\n\n\t> It may raise awareness so much that somebody gets a clever idea \n\t> how to cope with it.\n\n\tOkay, it might not be clever, but I think this is pretty \n\tstraight-forward.\n\n\tHowever, this part of the code is tricky, as it can (and will) be \n\tinterrupted by signal handlers, so I would appreciate several \n\tcareful reviews (but maybe it is not necessary to ask for it, \n\tsince I am no longer trusted).\n\n daemon.c |   61 +++++++++++++++++++++++++++++++++++++++++--------------------\n 1 files changed, 41 insertions(+), 20 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 63cd12c..35fd439 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -694,23 +694,47 @@ static void kill_some_children(int signo, unsigned start, unsigned stop)\n \t}\n }\n \n+static void check_dead_children(void)\n+{\n+\tunsigned spawned, reaped, deleted;\n+\n+\tspawned = children_spawned;\n+\treaped = children_reaped;\n+\tdeleted = children_deleted;\n+\n+\twhile (deleted < reaped) {\n+\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n+\t\tconst char *dead = pid < 0 ? \" (with error)\" : \"\";\n+\n+\t\tif (pid < 0)\n+\t\t\tpid = -pid;\n+\n+\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n+\t\tif (verbose) {\n+\t\t\tif (log_syslog)\n+\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\",\n+\t\t\t\t\t\tpid, dead);\n+\t\t\telse\n+\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\",\n+\t\t\t\t\t\tpid, dead);\n+\t\t}\n+\t\tremove_child(pid, deleted, spawned);\n+\t\tdeleted++;\n+\t}\n+\tchildren_deleted = deleted;\n+}\n+\n static void check_max_connections(void)\n {\n \tfor (;;) {\n \t\tint active;\n-\t\tunsigned spawned, reaped, deleted;\n+\t\tunsigned spawned, deleted;\n+\n+\t\tcheck_dead_children();\n \n \t\tspawned = children_spawned;\n-\t\treaped = children_reaped;\n \t\tdeleted = children_deleted;\n \n-\t\twhile (deleted < reaped) {\n-\t\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n-\t\t\tremove_child(pid, deleted, spawned);\n-\t\t\tdeleted++;\n-\t\t}\n-\t\tchildren_deleted = deleted;\n-\n \t\tactive = spawned - deleted;\n \t\tif (active <= max_connections)\n \t\t\tbreak;\n@@ -760,18 +784,10 @@ static void child_handler(int signo)\n \n \t\tif (pid > 0) {\n \t\t\tunsigned reaped = children_reaped;\n+\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\t\t\tpid = -pid;\n \t\t\tdead_child[reaped % MAX_CHILDREN] = pid;\n \t\t\tchildren_reaped = reaped + 1;\n-\t\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n-\t\t\tif (verbose) {\n-\t\t\t\tconst char *dead = \"\";\n-\t\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n-\t\t\t\t\tdead = \" (with error)\";\n-\t\t\t\tif (log_syslog)\n-\t\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\", pid, dead);\n-\t\t\t\telse\n-\t\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\", pid, dead);\n-\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tbreak;\n@@ -929,7 +945,8 @@ static int service_loop(int socknum, int *socklist)\n \tfor (;;) {\n \t\tint i;\n \n-\t\tif (poll(pfd, socknum, -1) < 0) {\n+\t\ti = poll(pfd, socknum, 1);\n+\t\tif (i < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n \t\t\t\t      strerror(errno));\n@@ -937,6 +954,10 @@ static int service_loop(int socknum, int *socklist)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\tif (i == 0) {\n+\t\t\tcheck_dead_children();\n+\t\t\tcontinue;\n+\t\t}\n \n \t\tfor (i = 0; i < socknum; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n-- \n1.5.6.1.376.g6b0fd\n"},{"id":"82274","messageId":"7vej68u6mr.fsf@gitster.siamese.dyndns.org","threadId":"14273","inReplyTo":"alpine.DEB.1.00.0807031624020.9925@racer","subject":"Re: [PATCH] git daemon: avoid calling syslog() from a signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-05T07:34:04Z","receivedAt":"2008-07-05T07:34:04Z","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> Signal handlers should never call syslog(), as that can raise signals\n> of its own.\n>\n> Instead, call the syslog() from the master process.\n\nEarlier parts seem to make sense but I am puzzled by these changes.\n\n> @@ -929,7 +945,8 @@ static int service_loop(int socknum, int *socklist)\n>  \tfor (;;) {\n>  \t\tint i;\n>  \n> -\t\tif (poll(pfd, socknum, -1) < 0) {\n> +\t\ti = poll(pfd, socknum, 1);\n> +\t\tif (i < 0) {\n>  \t\t\tif (errno != EINTR) {\n>  \t\t\t\terror(\"poll failed, resuming: %s\",\n>  \t\t\t\t      strerror(errno));\n> @@ -937,6 +954,10 @@ static int service_loop(int socknum, int *socklist)\n>  \t\t\t}\n>  \t\t\tcontinue;\n>  \t\t}\n> +\t\tif (i == 0) {\n> +\t\t\tcheck_dead_children();\n> +\t\t\tcontinue;\n> +\t\t}\n\nSo you will check every 1ms to see if there are new dead children, but why\nis this necessary?\n"},{"id":"82277","messageId":"alpine.DEB.1.00.0807051201320.3334@eeepc-johanness","threadId":"14273","inReplyTo":"7vej68u6mr.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git daemon: avoid calling syslog() from a signal handler","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-05T10:05:26Z","receivedAt":"2008-07-05T10:05:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 5 Jul 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > Signal handlers should never call syslog(), as that can raise signals \n> > of its own.\n> >\n> > Instead, call the syslog() from the master process.\n> \n> Earlier parts seem to make sense but I am puzzled by these changes.\n> \n> > @@ -929,7 +945,8 @@ static int service_loop(int socknum, int *socklist)\n> >  \tfor (;;) {\n> >  \t\tint i;\n> >  \n> > -\t\tif (poll(pfd, socknum, -1) < 0) {\n> > +\t\ti = poll(pfd, socknum, 1);\n> > +\t\tif (i < 0) {\n> >  \t\t\tif (errno != EINTR) {\n> >  \t\t\t\terror(\"poll failed, resuming: %s\",\n> >  \t\t\t\t      strerror(errno));\n> > @@ -937,6 +954,10 @@ static int service_loop(int socknum, int *socklist)\n> >  \t\t\t}\n> >  \t\t\tcontinue;\n> >  \t\t}\n> > +\t\tif (i == 0) {\n> > +\t\t\tcheck_dead_children();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> \n> So you will check every 1ms to see if there are new dead children, but why\n> is this necessary?\n\nThis comes from me not reading the man page for poll() properly.  Of \ncourse, I want to check every second: syslog timestamps the messages with \na resolution of 1 second, AFAIR, or at least some of them do.\n\nSo if you could just squash in this patch, that would be smashing:\n\n-- snipsnap --\n@@ -945,8 +945,8 @@ static int service_loop(int socknum, int *socklist)\n \tfor (;;) {\n \t\tint i;\n \n-\t\ti = poll(pfd, socknum, 1);\n+\t\ti = poll(pfd, socknum, 1000);\n \t\tif (i < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n \t\t\t\t      strerror(errno));\n"},{"id":"82291","messageId":"7vabgwtf6m.fsf@gitster.siamese.dyndns.org","threadId":"14273","inReplyTo":"alpine.DEB.1.00.0807051201320.3334@eeepc-johanness","subject":"Re: [PATCH] git daemon: avoid calling syslog() from a signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-05T17:26:57Z","receivedAt":"2008-07-05T17:26:57Z","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>> So you will check every 1ms to see if there are new dead children, but why\n>> is this necessary?\n>\n> This comes from me not reading the man page for poll() properly.  Of \n> course, I want to check every second: syslog timestamps the messages with \n> a resolution of 1 second, AFAIR, or at least some of them do.\n\nHmm.\n\nThe question was not about the millisecond typo, but about why time-out at\nall.\n\nWe would need to somehow break out of poll() after handling the SIGCHLD\nsignal and I guess timing the syscall out would be the most obvious way,\nbut somehow it felt awkward.\n\nAnother way would be to set up a pipe to ourself that is included in the\npoll() and write a byte to the pipe from the signal handler.\n"},{"id":"82321","messageId":"alpine.DEB.1.00.0807060337480.3557@eeepc-johanness","threadId":"14273","inReplyTo":"7vabgwtf6m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git daemon: avoid calling syslog() from a signal handler","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-06T01:42:09Z","receivedAt":"2008-07-06T01:42:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 5 Jul 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> So you will check every 1ms to see if there are new dead children, \n> >> but why is this necessary?\n> >\n> > This comes from me not reading the man page for poll() properly.  Of \n> > course, I want to check every second: syslog timestamps the messages \n> > with a resolution of 1 second, AFAIR, or at least some of them do.\n> \n> Hmm.\n> \n> The question was not about the millisecond typo, but about why time-out \n> at all.\n\nBecause I do not want to change the semantics!\n\nATM, in those cases where it works (as opposed to hanging!), git-daemon \n--verbose reports in the syslog when a client disconnected, possibly with \nan error.  It does so with a timestamp so that you can see how long the \nconnection lasted.  That is what logs are useful for.\n\nNow, syslog has timestamps at second-resolution (at least here it does), \nand I wanted to imitate that.\n\nThe alternative would be to deprive all users of an (mostly) accurate \ntimestamp of the disconnect time.\n\n> Another way would be to set up a pipe to ourself that is included in the \n> poll() and write a byte to the pipe from the signal handler.\n\nIt still would need to break out of the poll(), in which case the effect \nwould be _exactly_ the same, but with a lot of more trouble, and \nopportunities for me to bring in new bugs, right?\n\nCiao,\nDscho\n"},{"id":"82338","messageId":"7vmykvo87w.fsf@gitster.siamese.dyndns.org","threadId":"14273","inReplyTo":"alpine.DEB.1.00.0807060337480.3557@eeepc-johanness","subject":"Re: [PATCH] git daemon: avoid calling syslog() from a signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-06T06:08:35Z","receivedAt":"2008-07-06T06:08:35Z","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>> The question was not about the millisecond typo, but about why time-out \n>> at all.\n>\n> Because I do not want to change the semantics!\n>\n> ATM, in those cases where it works (as opposed to hanging!), git-daemon \n> --verbose reports in the syslog when a client disconnected, possibly with \n> an error.  It does so with a timestamp so that you can see how long the \n> connection lasted.  That is what logs are useful for.\n>\n> Now, syslog has timestamps at second-resolution (at least here it does), \n> and I wanted to imitate that.\n\nYes, we do not want to change the semantics, but that is not a reason for\nan unused daemon to wake up every second, isn't it?\n\n> The alternative would be to deprive all users of an (mostly) accurate \n> timestamp of the disconnect time.\n>\n>> Another way would be to set up a pipe to ourself that is included in the \n>> poll() and write a byte to the pipe from the signal handler.\n>\n> It still would need to break out of the poll(), in which case the effect \n> would be _exactly_ the same,...\n\nI do not think so.\n\nIn the solution I suggested, you would set up a pipe to yourself, and give\nthe read end of the pipe and the accepting socket to poll() with infinite\ntimeout.  And when you do reap in the signal handler, you write a byte to\nthe write end of the pipe (and that would be the only codepath that would\nwrite to that pipe).  That would make the pipe you are polling redable,\nand allow you to break out of poll().  When poll() returns thusly, you\nnotice you are in that condition and read out that byte to keep the pipe\nclean, and do the check_dead_children() thing.\n\nThat way, you would wake up only when you actually have something useful\nto do.  Otherwise you will stay dormant, waiting for something to happen,\neither by socket becoming ready to accept, or child dying and raising\nSIGCHLD.\n\nI agree that it is a bit too elaborate change that we do not want to have\nin 'maint'.\n\nEven then, for 'maint', we probably would at least want something like\nthis on top of your patch, so that when we know we have absolutely nothing\nto do, we do not have to spin.\n\n daemon.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 6dc96aa..ce3a6f5 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -944,6 +944,7 @@ static int service_loop(int socknum, int *socklist)\n \n \tfor (;;) {\n \t\tint i;\n+\t\tint timeout;\n \n \t\t/*\n \t\t * This 1-sec timeout could lead to idly looping but it is\n@@ -952,7 +953,8 @@ static int service_loop(int socknum, int *socklist)\n \t\t * to ourselves that we poll, and write to the fd from child_handler()\n \t\t * to wake us up (and consume it when the poll() returns...\n \t\t */\n-\t\ti = poll(pfd, socknum, 1000);\n+\t\ttimeout = (children_spawned != children_deleted) ? 1000 : -1;\n+\t\ti = poll(pfd, socknum, timeout);\n \t\tif (i < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n"},{"id":"82359","messageId":"alpine.LSU.1.00.0807061414320.3486@wbgn129.biozentrum.uni-wuerzburg.de","threadId":"14273","inReplyTo":"7vmykvo87w.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] git daemon: avoid calling syslog() from a signal handler","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-06T12:23:20Z","receivedAt":"2008-07-06T12:23:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nSignal handlers should never call syslog(), as that can raise signals\nof its own.\n\nInstead, call the syslog() from the master process.\n\nTo avoid waking up unnecessarily, a pipe is set up that is only ever\nwritten to by child_handler(), when a child disconnects, as suggested\nper Junio.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tOn Sat, 5 Jul 2008, Junio C Hamano wrote:\n\n\t> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\t> \n\t> > Junio wrote:\n\t> >> Another way would be to set up a pipe to ourself that is \n\t> >> included in the poll() and write a byte to the pipe from the signal \n\t> >> handler.\n\t> >\n\t> > It still would need to break out of the poll(), in which case \n\t> > the effect would be _exactly_ the same,...\n\t> \n\t> I do not think so.\n\n\tOkay.\n\n\tNote that we still have to check for dead children in \n\tcheck_max_connections(), and since child_handler() knows nothing \n\tabout that, it still will write to the pipe, waking up the loop \n\tunnecessarily.\n\n\tBut that will be rare.\n\n\tThis is no longer as trivial as I wanted it to be, so I'd \n\tappreciate a few eyeballs on this patch.\n\n daemon.c |   69 +++++++++++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 48 insertions(+), 21 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 63cd12c..620a288 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -16,6 +16,7 @@\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n+static int child_handler_pipe[2];\n \n static const char daemon_usage[] =\n \"git-daemon [--verbose] [--syslog] [--export-all]\\n\"\n@@ -694,23 +695,47 @@ static void kill_some_children(int signo, unsigned start, unsigned stop)\n \t}\n }\n \n+static void check_dead_children(void)\n+{\n+\tunsigned spawned, reaped, deleted;\n+\n+\tspawned = children_spawned;\n+\treaped = children_reaped;\n+\tdeleted = children_deleted;\n+\n+\twhile (deleted < reaped) {\n+\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n+\t\tconst char *dead = pid < 0 ? \" (with error)\" : \"\";\n+\n+\t\tif (pid < 0)\n+\t\t\tpid = -pid;\n+\n+\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n+\t\tif (verbose) {\n+\t\t\tif (log_syslog)\n+\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\",\n+\t\t\t\t\t\tpid, dead);\n+\t\t\telse\n+\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\",\n+\t\t\t\t\t\tpid, dead);\n+\t\t}\n+\t\tremove_child(pid, deleted, spawned);\n+\t\tdeleted++;\n+\t}\n+\tchildren_deleted = deleted;\n+}\n+\n static void check_max_connections(void)\n {\n \tfor (;;) {\n \t\tint active;\n-\t\tunsigned spawned, reaped, deleted;\n+\t\tunsigned spawned, deleted;\n+\n+\t\tcheck_dead_children();\n \n \t\tspawned = children_spawned;\n-\t\treaped = children_reaped;\n \t\tdeleted = children_deleted;\n \n-\t\twhile (deleted < reaped) {\n-\t\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n-\t\t\tremove_child(pid, deleted, spawned);\n-\t\t\tdeleted++;\n-\t\t}\n-\t\tchildren_deleted = deleted;\n-\n \t\tactive = spawned - deleted;\n \t\tif (active <= max_connections)\n \t\t\tbreak;\n@@ -760,18 +785,11 @@ static void child_handler(int signo)\n \n \t\tif (pid > 0) {\n \t\t\tunsigned reaped = children_reaped;\n+\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\t\t\tpid = -pid;\n \t\t\tdead_child[reaped % MAX_CHILDREN] = pid;\n \t\t\tchildren_reaped = reaped + 1;\n-\t\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n-\t\t\tif (verbose) {\n-\t\t\t\tconst char *dead = \"\";\n-\t\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n-\t\t\t\t\tdead = \" (with error)\";\n-\t\t\t\tif (log_syslog)\n-\t\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\", pid, dead);\n-\t\t\t\telse\n-\t\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\", pid, dead);\n-\t\t\t}\n+\t\t\twrite(child_handler_pipe[1], &status, 1);\n \t\t\tcontinue;\n \t\t}\n \t\tbreak;\n@@ -917,19 +935,24 @@ static int service_loop(int socknum, int *socklist)\n \tstruct pollfd *pfd;\n \tint i;\n \n-\tpfd = xcalloc(socknum, sizeof(struct pollfd));\n+\tif (pipe(child_handler_pipe) < 0)\n+\t\tdie (\"Could not set up pipe for child handler\");\n+\n+\tpfd = xcalloc(socknum + 1, sizeof(struct pollfd));\n \n \tfor (i = 0; i < socknum; i++) {\n \t\tpfd[i].fd = socklist[i];\n \t\tpfd[i].events = POLLIN;\n \t}\n+\tpfd[socknum].fd = child_handler_pipe[0];\n+\tpfd[socknum].events = POLLIN;\n \n \tsignal(SIGCHLD, child_handler);\n \n \tfor (;;) {\n \t\tint i;\n \n-\t\tif (poll(pfd, socknum, -1) < 0) {\n+\t\tif (poll(pfd, socknum + 1, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n \t\t\t\t      strerror(errno));\n@@ -937,6 +960,10 @@ static int service_loop(int socknum, int *socklist)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\tif (pfd[socknum].revents & POLLIN) {\n+\t\t\tread(child_handler_pipe[0], &i, 1);\n+\t\t\tcheck_dead_children();\n+\t\t}\n \n \t\tfor (i = 0; i < socknum; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n-- \n1.5.6.1.300.gca3f\n"},{"id":"82410","messageId":"7vmykua2et.fsf@gitster.siamese.dyndns.org","threadId":"14273","inReplyTo":"alpine.LSU.1.00.0807061414320.3486@wbgn129.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH v2] git daemon: avoid calling syslog() from a signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-07T01:50:02Z","receivedAt":"2008-07-07T01:50: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> \tNote that we still have to check for dead children in \n> \tcheck_max_connections(), and since child_handler() knows nothing \n> \tabout that, it still will write to the pipe, waking up the loop \n> \tunnecessarily.\n>\n> \tBut that will be rare.\n>\n> \tThis is no longer as trivial as I wanted it to be, so I'd \n> \tappreciate a few eyeballs on this patch.\n\nYeah, I think the fix-up I sent on top of your patch to make 1-sec timeout\nconditional would be more appropriate for 'maint' than this one, even\nthough it is a band-aid to the issue.\n\nAnother thing we might want to consider to make this logic much more\nsimpler would be to move everything out of child_handler(), except the\nwrite() whose sole purpose is to allow us break out of the poll().\n\nThen you do not have to use \"negative is error and positive is success\"\nconvention (which you may regret later when you would need to pass more\nthan one bit of information from the callsite of waitpid() to the callsite\nof syslog()), nor separate \"reap here, report there\" code structure.\n"},{"id":"82426","messageId":"20080707065439.GA25877@cuci.nl","threadId":"14273","inReplyTo":"7vmykua2et.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] git daemon: avoid calling syslog() from a signal handler","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-07-07T06:54:39Z","receivedAt":"2008-07-07T06:54:39Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>Another thing we might want to consider to make this logic much more\n>simpler would be to move everything out of child_handler(), except the\n>write() whose sole purpose is to allow us break out of the poll().\n\nAs a general rule, keep the work done in a signal handler down to the\nbare minimum (like setting an integer flag, and perhaps unblocking the\nmain thread through a write to this signallingpipe).\n-- \nSincerely,\n           Stephen R. van den Berg.\n\nA truly wise man never plays leapfrog with a unicorn.\n"}]}