Better TCP keepalive support

[Felix Geisendoerfer]

Currently TCP keepalive support in Go is limited and inconsistent.

SetKeepAlivePeriod on Linux sets both the initial timeout as well as the probe interval to the same duration (see issue 8328). On Darwin, only the initial idle timeout is set, even so the probe interval can also be configured (OSX >= 10.8).

IMO it would be useful to be able to configure the initial timeout interval, the probe interval, as well as the probe count individually for the platforms that support it. Since this can be done in userland I've implemented a stopgap package for this:

http://godoc.org/github.com/felixge/tcpkeepalive

If this seems acceptable, I'd be happy to provide a patch that adds these methods. SetKeepAlivePeriod() should probably be marked as deprecated.

If this is not acceptable for some reason, I'd like to improve the documentation for SetKeepAlivePeriod() to save other users the trouble of figuring out what it does.

Cheers,

Felix

[Ian Lance Taylor]

I think that tweaks like this should go into the go.net repository if
possible.

Improving the docs for the existing function is always a good idea.

Ian

[Felix Geisendörfer]

On 26.08.2014, at 17:36, Ian Lance Taylor <ia...@golang.org> wrote:

> I think that tweaks like this should go into the go.net repository if
> possible.

I guess I don’t feel strongly where TCP keep alive support is implemented.

But IMO SetKeepAlivePeriod has very questionable behavior, as explained in issue 8328 [1].

I’m especially confused that the initial patch separated the idle timeout and the probe interval, but russ asked for it to be combined, and merged it regardless of nkatsaros' objection [2].

So if TCP keep alive should go into go.net, I’m wondering if usage of SetKeepAlivePeriod should be discouraged by the documentation?

[1] https://code.google.com/p/go/issues/detail?id=8328
[2] https://codereview.appspot.com/11130044/#ps3001

> Improving the docs for the existing function is always a good idea.

Sure. I’d rather improve the code, but I’ll do whatever you all think is best.

Cheers,
Felix

[Mikio Hara]

hi,

> I'm especially confused that the initial patch separated the idle timeout and the probe interval, but russ asked for it to be combined, and merged it regardless of nkatsaros' objection.

looks like it's a simple bug and perhaps the reviewers overlooked the
suggestion.

KeepAlivePeriod = ???
To = duration of observation
Tr = prod(N, Ti)
Ti = interval btw probes
N = maximum number of probes

the fix would be either we treat KeepAlivePeriod as To or treat it as
To+Tr. the latter case looks cleaner but it requires a bit more code
to fetch the kernel state; on almost all the platforms it's a
write-only option so we cannot use getsockopt simply.

does this sound reasonable to you?

---
ps: i have a similar package too; http://godoc.org/github.com/mikioh/tcp
in my experience mostly nothing bad using such external package on top
of the tcp. though, there's an itch that needs to be scratched in the
case of composition with net/http or crypto/tls+net/http.
unfortunately net/http/server.go only accepts CloseWrite method of
net.TCPConn. under the go1 contract, we cannot change net.Conn
interface and i.m not keen on introducing a shim to the net/http
package like below;

if tcp, ok := c.rwc.(*net.TCPConn); ok {
tcp.CloseWrite()
} else if m, ok := reflect.TypeOf(c.rwc).MethodByName("CloseWrite"); ok {
m.Func.Call([]reflect.Value{reflect.ValueOf(c.rwc)})
}

so a workaround would be simply to replace http.Server with own stuff.

[Felix Geisendörfer]

Hi,

On 27.08.2014, at 07:26, Mikio Hara <mikioh...@gmail.com> wrote:

>> I'm especially confused that the initial patch separated the idle timeout and the probe interval, but russ asked for it to be combined, and merged it regardless of nkatsaros' objection.
>
> looks like it's a simple bug and perhaps the reviewers overlooked the
> suggestion.
>
> KeepAlivePeriod = ???
> To = duration of observation
> Tr = prod(N, Ti)
> Ti = interval btw probes
> N = maximum number of probes
>
> the fix would be either we treat KeepAlivePeriod as To or treat it as
> To+Tr. the latter case looks cleaner but it requires a bit more code
> to fetch the kernel state; on almost all the platforms it's a
> write-only option so we cannot use getsockopt simply.
>
> does this sound reasonable to you?

If SetKeepAlivePeriod would modify the To+Tr duration, that seems reasonable / useful.

However, due to the implementation issues you mentioned, I’d probably favor exposing the individual options (To, Ti, N).

> ---
> ps: i have a similar package too; http://godoc.org/github.com/mikioh/tcp
> in my experience mostly nothing bad using such external package on top
> of the tcp. though, there's an itch that needs to be scratched in the
> case of composition with net/http or crypto/tls+net/http.
> unfortunately net/http/server.go only accepts CloseWrite method of
> net.TCPConn. under the go1 contract, we cannot change net.Conn
> interface and i.m not keen on introducing a shim to the net/http
> package like below;
>
> if tcp, ok := c.rwc.(*net.TCPConn); ok {
> tcp.CloseWrite()
> } else if m, ok := reflect.TypeOf(c.rwc).MethodByName("CloseWrite"); ok {
> m.Func.Call([]reflect.Value{reflect.ValueOf(c.rwc)})
> }
>
> so a workaround would be simply to replace http.Server with own stuff.

Wouldn’t defining a CloseWriter interface and check for that be more appropriate?

Also: The problem you mention only occurs when passing around a TCPConn embedded inside another struct, right? It seems like this can be worked around by just passing the TCPConn directly after the options have been applied to it?

Cheers,
Felix

[Mikio Hara]

On Wed, Aug 27, 2014 at 3:56 PM, Felix Geisendörfer <haim...@gmail.com> wrote:

> Wouldn’t defining a CloseWriter interface and check for that be more appropriate?

a related issue: golang.org/issue/8579, but i'm not sure wether it's worth it.

> Also: The problem you mention only occurs when passing around a TCPConn embedded inside another struct, right?

right. there's no problem unless we have SCTPConn, MPTCPConn-like
stuff and use it with net/http package.