use TCP keep-alives by default in HTTP

[Brad Fitzpatrick]

For discussion.

Any objections on something like this?

TCP keep-alives are really the right solution, compared to application-level read/write timeouts.  Hanging connections are popular and we shouldn't close them just because they've been open a long time.  We only care about closing things from people whose TCP stacks have gone away.  Unlike SSH or other protocols, HTTP isn't meant to be opened, forgotten about, and returned to hours or days later.  A keep-alive is appropriate.

This CL below doesn't address TLS connections yet.

---------- Forwarded message ----------
From: <brad...@golang.org>
Date: Mon, Jan 6, 2014 at 5:07 PM
Subject: code review 48300043: net/http: use TCP keep-alives by default (issue 48300043)
To: golang-co...@googlegroups.com
Cc: re...@codereview-hr.appspotmail.com


Reviewers: golang-codereviews,

Message:
Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
net/http: use TCP keep-alives by default

Our default behavior (ListenAndServe, etc) shouldn't lead to
leaked TCP connections (e.g. from people closing laptops) when
their Go servers are exposed to the open Internet without a
proxy in front.

Too many users on golang-nuts have learned this the hard way.

Provide a knob for people who really care.

Please review this at https://codereview.appspot.com/48300043/

Affected files (+15, -0 lines):
  M src/pkg/net/http/server.go


Index: src/pkg/net/http/server.go
===================================================================
--- a/src/pkg/net/http/server.go
+++ b/src/pkg/net/http/server.go
@@ -418,8 +418,18 @@
 // with a verbose logging wrapper.
 const debugServerConnections = false

+const defaultTCPKeepAlivePeriod = 1 * time.Minute
+
 // Create new connection from rwc.
 func (srv *Server) newConn(rwc net.Conn) (c *conn, err error) {
+       period := srv.TCPKeepAlivePeriod
+       if period == 0 {
+               period = defaultTCPKeepAlivePeriod
+       }
+       if tc, ok := rwc.(*net.TCPConn); ok && period > 0 {
+               tc.SetKeepAlive(true)
+               tc.SetKeepAlivePeriod(period)
+       }
        c = new(conn)
        c.remoteAddr = rwc.RemoteAddr().String()
        c.server = srv
@@ -1573,6 +1583,11 @@
        MaxHeaderBytes int           // maximum size of request headers, DefaultMaxHeaderBytes if 0
        TLSConfig      *tls.Config   // optional TLS config, used by ListenAndServeTLS

+       // TCPKeepAlivePeriod specifies the TCP keep-alive period.
+       // If zero, a default is used.
+       // If negative, TCP keep-alives are not used.
+       TCPKeepAlivePeriod time.Duration
+
        // TLSNextProto optionally specifies a function to take over
        // ownership of the provided TLS connection when an NPN
        // protocol upgrade has occurred.  The map key is the protocol

[Andy Balholm]

On Monday, January 6, 2014 5:10:16 PM UTC-8, Brad Fitzpatrick wrote:

For discussion.

Any objections on something like this?

TCP keep-alives are really the right solution, compared to application-level read/write timeouts.  Hanging connections are popular and we shouldn't close them just because they've been open a long time.  We only care about closing things from people whose TCP stacks have gone away.  Unlike SSH or other protocols, HTTP isn't meant to be opened, forgotten about, and returned to hours or days later.  A keep-alive is appropriate.

I have more problems on the client side: often when I try to use a connection from a Transport's keep-alive pool, it is actually dead and I just get EOF. I have been running a goroutine that calls CloseIdleConnections every 10 seconds, but I wish there were a better solution. 

[Brad Fitzpatrick]

Okay, but that's unrelated.  If you can capture a pcap, that'd be more interesting.  I've seen servers says it supports keep-alive and then immediately hangs up on you. I don't know how common that is, or whether that's what you're seeing.

This proposal is about TCP-level (not HTTP-level) keep-alives, so internet-facing servers don't need to be restarted regularly due the fd limit exhausted from closed laptops and whatnot.

[Caleb Spare]

Is there some way to configure my net/http server to use TCP keep-alives today?

--
 
---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Kamil Kisiel]

Implement your own net.Listener that wraps TCPListener and set it before you return the conn in Accept?

[Brad Fitzpatrick]

Yeah, that's the current way.

But people don't do that.  We should have it on by default.

[Kamil Kisiel]

SGTM. One less surprise for people to run in to.

Kamil

[Russ Cox]

I'm a little sad that net/http will now know the type net.TCPConn. That seems like a step backward, and it doesn't help people doing http over something over TCP. You mentioned something = TLS but there might be others.

That said, I don't have a better idea so I'm not saying we can't do it.

Russ

[Brad Fitzpatrick]

I care about the common case.

People doing fancy configs don't have this problem

[Caleb Spare]

Why not make a keep-alive listener in ListenAndServe(TLS)? Then newConn doesn't have to know about net.TCPConn and it covers the common HTTP and HTTPS cases.

If you're using Serve rather than ListenAndServe you're already getting slightly fancy.

--

[Andy Balholm]

On Jan 7, 2014, at 8:37 AM, Brad Fitzpatrick <brad...@golang.org> wrote:

On Tue, Jan 7, 2014 at 8:25 AM, Andy Balholm <andyb...@gmail.com> wrote:

I have more problems on the client side: often when I try to use a connection from a Transport's keep-alive pool, it is actually dead and I just get EOF. I have been running a goroutine that calls CloseIdleConnections every 10 seconds, but I wish there were a better solution.

Okay, but that's unrelated.  If you can capture a pcap, that'd be more interesting.  I've seen servers says it supports keep-alive and then immediately hangs up on you. I don't know how common that is, or whether that's what you're seeing.

That’s not what I’m seeing. What I’m seeing is that the server disconnects after 15 seconds or so of inactivity, but the client doesn’t realize it. So then it tries to reuse the connection and gets EOF as soon as it writes to it.

[Mikio Hara]

I'm fine with applying TCP keepalive option to passive open
connections at HTTP server side by default.

But I'd rather see a bit more abstracted control knob such as
ConnectionKeepAlivePeriod or so than TCPKeepAlivePeriod because, a) we
can reuse this knob when we want another underlying stream transport
such as MPTCP w/ subflow keepalive, SCTP w/ multipath hearbeat, or TLS
over TCP/MPTCP/SCTP w/ session maintenance heartbeat, b) for upcoming
HTTP/2, it clearly declares the logical separation of sessions (aka
streams in the spec) and underlying transport connections to achieve a
bit fancy traffic coordination.

> This CL below doesn't address TLS connections yet.

Ouch. http://tools.ietf.org/search/rfc6520, anyone?

[minux]

On Tue, Jan 7, 2014 at 5:18 PM, Caleb Spare <ces...@gmail.com> wrote:

Why not make a keep-alive listener in ListenAndServe(TLS)? Then newConn doesn't have to know about net.TCPConn and it covers the common HTTP and HTTPS cases.

Yeah, I think this should solve most of the common problems without teaching more

net/http stuff about TCP.

If you're using Serve rather than ListenAndServe you're already getting slightly fancy.

Agreed. I suspect in most cases, people creating Server just to set ReadTimeout and

WriteTimeout, so users of Server should be already aware of the problem, and there is

no need to force a new behavior on them.

If we want to do tcp keepalive also to users of Server, we probably should introduce

a new bool in Server.

[Brad Fitzpatrick]

SGTM.  PTAL.

No API change now.  We can always add a knob later, if needed, but probably not.