[PATCH] daemon: enable SO_KEEPALIVE for all sockets

[Eric Wong-2]

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 <[hidden email]>
---
 daemon.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

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)),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Jeff King]

On Wed, May 25, 2016 at 03:15:05AM +0000, Eric Wong wrote:

> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Junio C Hamano]

Eric Wong <[hidden email]> writes:

> 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 <[hidden email]>
> ---

This makes sense as a follow-up to e47a8583 (enable SO_KEEPALIVE for
connected TCP sockets, 2011-12-06), I think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Wong-2]

Junio C Hamano <[hidden email]> 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@...

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:

--- 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
 }
 ------------
 +
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html