[PATCH] git daemon: avoid waking up too often

To avoid waking up unnecessarily, a pipe is set up that is only ever
written to by child_handler(), when a child disconnects, as suggested
per Junio.

This avoids waking up the main process every second to see if a child
was disconnected.

Signed-off-by: Johannes Schindelin <redacted>
---

	I have this in production ever since I posted it the first time,
	without problems.

	Oh, and it removes more lines than it introduces ;-)

 daemon.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/daemon.c b/daemon.c
index 7df41a6..850f6f9 100644
--- a/daemon.c
+++ b/daemon.c

@@ -16,6 +16,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"

@@ -788,6 +789,7 @@ static void child_handler(int signo)
 				pid = -pid;
 			dead_child[reaped % MAX_CHILDREN] = pid;
 			children_reaped = reaped + 1;
+			write(child_handler_pipe[1], &status, 1);
 			continue;
 		}
 		break;

@@ -933,12 +935,17 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	pfd = xcalloc(socknum, sizeof(struct pollfd));
+	if (pipe(child_handler_pipe) < 0)
+		die ("Could not set up pipe for child handler");
+
+	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
+	pfd[socknum].fd = child_handler_pipe[0];
+	pfd[socknum].events = POLLIN;
 
 	signal(SIGCHLD, child_handler);
 

@@ -946,16 +953,7 @@ static int service_loop(int socknum, int *socklist)
 		int i;
 		int timeout;
 
-		/*
-		 * This 1-sec timeout could lead to idly looping but it is
-		 * here so that children culled in child_handler() are reported
-		 * without too much delay.  We could probably set up a pipe
-		 * to ourselves that we poll, and write to the fd from child_handler()
-		 * to wake us up (and consume it when the poll() returns...
-		 */
-		timeout = (children_spawned != children_deleted) ? 1000 : -1;
-		i = poll(pfd, socknum, timeout);
-		if (i < 0) {
+		if (poll(pfd, socknum + 1, -1) < 0) {
 			if (errno != EINTR) {
 				error("poll failed, resuming: %s",
 				      strerror(errno));

@@ -963,9 +961,9 @@ static int service_loop(int socknum, int *socklist)
 			}
 			continue;
 		}
-		if (i == 0) {
+		if (pfd[socknum].revents & POLLIN) {
+			read(child_handler_pipe[0], &i, 1);
 			check_dead_children();
-			continue;
 		}
 
 		for (i = 0; i < socknum; i++) {

-- 
1.6.0.rc0.22.gf2096d.dirty

To avoid waking up unnecessarily, a pipe is set up that is only ever
written to by child_handler(), when a child disconnects, as suggested
per Junio.

This avoids waking up the main process every second to see if a child
was disconnected.

Signed-off-by: Johannes Schindelin <redacted>
---

	Darn.  Forgot to commit after fixing the compiler warning:
	The now-unused variable timeout is gone.

	That is the only difference to the version I have running in
	production ;-)

 daemon.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/daemon.c b/daemon.c
index 7df41a6..4540e8d 100644
--- a/daemon.c
+++ b/daemon.c

@@ -16,6 +16,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"

@@ -788,6 +789,7 @@ static void child_handler(int signo)
 				pid = -pid;
 			dead_child[reaped % MAX_CHILDREN] = pid;
 			children_reaped = reaped + 1;
+			write(child_handler_pipe[1], &status, 1);
 			continue;
 		}
 		break;

@@ -933,29 +935,24 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	pfd = xcalloc(socknum, sizeof(struct pollfd));
+	if (pipe(child_handler_pipe) < 0)
+		die ("Could not set up pipe for child handler");
+
+	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
+	pfd[socknum].fd = child_handler_pipe[0];
+	pfd[socknum].events = POLLIN;
 
 	signal(SIGCHLD, child_handler);
 
 	for (;;) {
 		int i;
-		int timeout;
 
-		/*
-		 * This 1-sec timeout could lead to idly looping but it is
-		 * here so that children culled in child_handler() are reported
-		 * without too much delay.  We could probably set up a pipe
-		 * to ourselves that we poll, and write to the fd from child_handler()
-		 * to wake us up (and consume it when the poll() returns...
-		 */
-		timeout = (children_spawned != children_deleted) ? 1000 : -1;
-		i = poll(pfd, socknum, timeout);
-		if (i < 0) {
+		if (poll(pfd, socknum + 1, -1) < 0) {
 			if (errno != EINTR) {
 				error("poll failed, resuming: %s",
 				      strerror(errno));

@@ -963,9 +960,9 @@ static int service_loop(int socknum, int *socklist)
 			}
 			continue;
 		}
-		if (i == 0) {
+		if (pfd[socknum].revents & POLLIN) {
+			read(child_handler_pipe[0], &i, 1);
 			check_dead_children();
-			continue;
 		}
 
 		for (i = 0; i < socknum; i++) {

-- 
1.6.0.rc0.22.gf2096d.dirty

Johannes Schindelin schrieb:

To avoid waking up unnecessarily, a pipe is set up that is only ever
written to by child_handler(), when a child disconnects, as suggested
per Junio.

This avoids waking up the main process every second to see if a child
was disconnected.

This makes porting this beast to Windows practically impossible because we
cannot have a poll() implementation that waits both on a listening socket
and a pipe. :-(

-- Hannes

Hi,

On Wed, 23 Jul 2008, Johannes Sixt wrote:

Johannes Schindelin schrieb:

quoted

To avoid waking up unnecessarily, a pipe is set up that is only ever
written to by child_handler(), when a child disconnects, as suggested
per Junio.

This avoids waking up the main process every second to see if a child
was disconnected.

This makes porting this beast to Windows practically impossible because we
cannot have a poll() implementation that waits both on a listening socket
and a pipe. :-(

Umm.  We also do not have signal handlers, do we?  AFAICT you wanted to 
simulate _this_ program's fork() with a start_async(), right?  All you 
have to do is to refactor the code with the pipe to do the update 
(properly guarded by a mutex, which is called "CriticalSection" in Windows 
speak, since it is impossible for Microsoft to accept common conventions).

The biggest part to get this beast running on Windows is to move it to the 
start_async() method, not above-mentioned refactoring.

Ciao,
Dscho

On 7/23/08, Johannes Sixt [off-list ref] wrote:

This makes porting this beast to Windows practically impossible because we
 cannot have a poll() implementation that waits both on a listening socket
 and a pipe. :-(

I have worked around such problems in the past by having a thread
whose job it is to read from the pipe and simply write it to a socket.
 The trick works for "console" objects, too, which in win32 are even
less agreeable than pipes.

(In case your life wasn't disgusting enough already today :))

Alternatively, you could use something like socketpair() instead of a
pipe for this purpose.  Naturally, Win32 helps you out here by somehow
forgetting to include socketpair() in winsock, although it's sort of
easy to emulate.

Have fun,

Avery

Hi,

On Wed, 23 Jul 2008, Avery Pennarun wrote:

On 7/23/08, Johannes Sixt [off-list ref] wrote:

quoted

This makes porting this beast to Windows practically impossible 
because we cannot have a poll() implementation that waits both on a 
listening socket and a pipe. :-(

I have worked around such problems in the past by having a thread whose 
job it is to read from the pipe and simply write it to a socket. The 
trick works for "console" objects, too, which in win32 are even less 
agreeable than pipes.

(In case your life wasn't disgusting enough already today :))

Thanks.  I thought the code I looked at today was ugly, but you convinced 
me that there is no limit.

Alternatively, you could use something like socketpair() instead of a 
pipe for this purpose.  Naturally, Win32 helps you out here by somehow 
forgetting to include socketpair() in winsock, although it's sort of 
easy to emulate.

Or alternatively, you could read my response to Hannes where I explain 
that the pipe() is not even needed with threaded start_async() (as opposed 
to fork()ed one).

Ciao,
Dscho