[PATCH] Setting SO_KEEPALIVE on outbound connections

Hi Squid devs,

Below I include a patch to enable configurable SO_KEEPALIVE on outbound
connections from Squid. Amos helped me out with some advice on
squid-users@ and I've diverged a bit from the advice -- I'd like to
explain my reasoning:

1) This patch simply enables SO_KEEPALIVE for outbound connections if a
global "tcp_keepalive on" is configured. TCP keepalive settings are
distinct from protocol-level timeouts and I do not want to conflate them.

2) I did not use commSetTcpKeepalive() as I believe it may have an issue
in its handling of TCP_KEEPCNT. There are four relevant keepalive values:

  a) SO_KEEPALIVE, boolean. The only portable value.

  b) TCP_KEEPIDLE, idle time in seconds before the socket is placed in
     keepalive mode. AKA tcp_keepalive_time on linux

  c) TCP_KEEPINTVL, seconds between probes, once keepalive mode is
     engaged. AKA tcp_keepalive_intvl on linux

  d) TCP_KEEPCNT, the maximum number of keepalive probes sent before
     dropping the connection.

commSetTcpKeepalive() appears to treat TCP_KEEPCNT as a time value, rather
than a count value:

> if (timeout && interval) {
> int count = (timeout + interval - 1) / interval;
> if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &count, sizeof(on)) < 0)

I would suggest a larger patch modifying commSetTcpKeepalive() to replace
"int timeout" with "int keep_count", and replace the above three lines
with only:

> if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keep_count, sizeof(on)) < 0)

If squid-dev is amenable to this change I will submit another patch for
this issue. It's a behavior change so I wanted to keep it separate from
the feature patch I include below. I would recommend:

 * Introduce new keep_count= parameter
 * Retain current behavior if keep_count= is not present and timeout= is
   set to avoid changing existing behavior
 * Document deprecation of timeout= parameter

3) I only include a boolean tcp_keepalive, rather than passing in args for
the remaining 3 parameters. I'm a bit confused as to how I should use
cf.data.pre to pass config parameters. If someone could point me at how to
supply something like this I'd appreciate it:

tcp_keepalive[=idle,interval,keep_count]

Patch:

diff -ur a/src/cf.data.pre b/src/cf.data.pre
--- a/src/cf.data.pre 2013-12-30 11:33:27.000000000 +0000
+++ b/src/cf.data.pre 2014-01-28 04:44:09.823041592 +0000
@@ -7093,6 +7093,14 @@
         the server generating a directory listing.
 DOC_END
 
+NAME: tcp_keepalive
+TYPE: onoff
+LOC: Config.onoff.tcp_keepalive
+DEFAULT: off
+DOC_START
+ This directive enables TCP keepalive for outbound connections.
+DOC_END
+
 NAME: short_icon_urls
 TYPE: onoff
 LOC: Config.icons.use_short_names
diff -ur a/src/comm.cc b/src/comm.cc
--- a/src/comm.cc 2013-12-30 11:33:27.000000000 +0000
+++ b/src/comm.cc 2014-01-28 05:50:23.242620051 +0000
@@ -702,10 +702,14 @@
         commSetTcpNoDelay(new_socket);
 
 #endif
-
     if (Config.tcpRcvBufsz > 0 && sock_type == SOCK_STREAM)
         commSetTcpRcvbuf(new_socket, Config.tcpRcvBufsz);
 
+ if (Config.onoff.tcp_keepalive) {
+ int on = 1;
+ setsockopt(new_socket, SOL_SOCKET, SO_KEEPALIVE, (char *) &on,
sizeof(on));
+ }
+
     return new_socket;
 }
 
diff -ur a/src/SquidConfig.h b/src/SquidConfig.h
--- a/src/SquidConfig.h 2013-12-30 11:33:27.000000000 +0000
+++ b/src/SquidConfig.h 2014-01-28 05:06:59.588199383 +0000
@@ -341,6 +341,7 @@
         int emailErrData;
         int httpd_suppress_version_string;
         int global_internal_static;
+ int tcp_keepalive;
 
 #if FOLLOW_X_FORWARDED_FOR
         int acl_uses_indirect_client;
Received on Tue Jan 28 2014 - 18:29:51 MST