threads / patch / 42446

patchdaemon: enable SO_KEEPALIVE for all sockets

Subject: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

## tl;dr

4 messages between May 25, 2016 and May 25, 2016. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Eric Wong· May 25, 2016, 03:15 UTC · lore

While --init-timeout and --timeout options exist and I've never run git-daemon without them, some users may forget to set them and encounter hung daemon processes when connections fail. Enable socket-level timeouts so the kernel can send keepalive probes as necessary to detect failed connections.

Signed-off-by: Eric Wong <e@80x24.org>
---
 daemon.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Show changes to daemon.c +14 −0
diff --git a/daemon.c b/daemon.c
index 8d45c33..46dddac 100644
--- a/daemon.c
+++ b/daemon.c
@@ -669,6 +669,15 @@ static void hostinfo_clear(struct hostinfo *hi)
 	strbuf_release(&hi->tcp_port);
 }
 
+static void set_keep_alive(int sockfd)
+{
+	int ka = 1;
+
+	if (setsockopt(sockfd, SOL_SOCKET, SO_KEEPALIVE, &ka, sizeof(ka)) < 0)
+		logerror("unable to set SO_KEEPALIVE on socket: %s",
+			strerror(errno));
+}
+
 static int execute(void)
 {
 	char *line = packet_buffer;
@@ -681,6 +690,7 @@ static int execute(void)
 	if (addr)
 		loginfo("Connection from %s:%s", addr, port);
 
+	set_keep_alive(0);
 	alarm(init_timeout ? init_timeout : timeout);
 	pktlen = packet_read(0, NULL, NULL, packet_buffer, sizeof(packet_buffer), 0);
 	alarm(0);
@@ -951,6 +961,8 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 			continue;
 		}
 
+		set_keep_alive(sockfd);
+
 		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			logerror("Could not bind to %s: %s",
 				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
@@ -1010,6 +1022,8 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 		return 0;
 	}
 
+	set_keep_alive(sockfd);
+
 	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
 		logerror("Could not bind to %s: %s",
 			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
Jeff King· May 25, 2016, 22:42 UTC · re: Eric Wong · lore

Re: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

On Wed, May 25, 2016 at 03:15:05AM +0000, Eric Wong wrote:
Show 5 quoted lines
> While --init-timeout and --timeout options exist and I've never
> run git-daemon without them, some users may forget to set them
> and encounter hung daemon processes when connections fail.
> Enable socket-level timeouts so the kernel can send keepalive
> probes as necessary to detect failed connections.

Makes sense. I wondered if there were any portability issues here, but it looks like the same code is found on the client side (but we'd still want it here for cases where the client thinks the connection is dead but the server does not realize it).

-Peff
Junio C Hamano· May 25, 2016, 22:44 UTC · re: Eric Wong · lore

Re: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

Eric Wong <e@80x24.org> writes:
Show 8 quoted lines
> While --init-timeout and --timeout options exist and I've never
> run git-daemon without them, some users may forget to set them
> and encounter hung daemon processes when connections fail.
> Enable socket-level timeouts so the kernel can send keepalive
> probes as necessary to detect failed connections.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---

This makes sense as a follow-up to e47a8583 (enable SO_KEEPALIVE for connected TCP sockets, 2011-12-06), I think.

Eric Wong· May 25, 2016, 23:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

Junio C Hamano <gitster@pobox.com> wrote:
> This makes sense as a follow-up to e47a8583 (enable SO_KEEPALIVE for
> connected TCP sockets, 2011-12-06), I think.
Yes, a15d069a19867 for http, too; hat trick :>

Anyways, it might've helped Savannah when they had networking problems:

  http://mid.gmane.org/20160524214102858920068@bob.proulx.com

They might be running an old version that didn't send keepalive heartbeats during packing, too. But SO_KEEPALIVE will still help during init when --init-timeout is not set.

Perhaps it also makes sense to squash the following xinetd setting into giteveryday.txt, too; since some users could be running out-of-date git but reading new documentation on the web:

Show changes to Documentation/giteveryday.txt +1 −0
--- a/Documentation/giteveryday.txt
+++ b/Documentation/giteveryday.txt
@@ -390,6 +390,7 @@ service git
 	server          = /usr/bin/git-daemon
 	server_args     = --inetd --export-all --base-path=/pub/scm
 	log_on_failure  += USERID
+	flags           = KEEPALIVE
 }
 ------------
 +

← back to recent threads