code review 69670043: net/http: add Server.SetKeepAlivesEnabled

net/http: add Server.SetKeepAlivesEnabled

Part of graceful shutdown.

Update Issue 4674

[bradfitz, 2014-02-27 23:35:58]

Hello adg@golang.org, josharian@gmail.com, r@rcrowley.org (cc:
golang-codereviews@googlegroups.com),

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

[adg, 2014-02-28 00:02:40]

The names here are a bit inconsistent. The field is disableKeepAlives but
the function is SetKeepAlivesEnabled and the var is sendClose.

Can we just use keepAliveEnabled for the field, SetKeepAliveEnabled for the
method, and keepAliveEnabled for the local var?

Otherwise the general functionality and implementation looks good.

On Friday, 28 February 2014, <bradfitz@golang.org> wrote:

> Reviewers: adg, josharian, rcrowley,
>
> Message:
> Hello adg@golang.org, josharian@gmail.com, r@rcrowley.org (cc:
> golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: add Server.SetKeepAlivesEnabled
>
> Part of graceful shutdown.
>
> Update Issue 4674
>
> Please review this at https://codereview.appspot.com/69670043/
>
> Affected files (+40, -3 lines):
>   M src/pkg/net/http/serve_test.go
>   M src/pkg/net/http/server.go
>
>
> Index: src/pkg/net/http/serve_test.go
> ===================================================================
> --- a/src/pkg/net/http/serve_test.go
> +++ b/src/pkg/net/http/serve_test.go
> @@ -2243,6 +2243,22 @@
>         }
>  }
>
> +func TestServerKeepAlivesEnabled(t *testing.T) {
> +       defer afterTest(t)
> +       ts := httptest.NewUnstartedServer(HandlerFunc(func(w
> ResponseWriter, r *Request) {}))
> +       ts.Config.SetKeepAlivesEnabled(false)
> +       ts.Start()
> +       defer ts.Close()
> +       res, err := Get(ts.URL)
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       defer res.Body.Close()
> +       if !res.Close {
> +               t.Errorf("Body.Close == false; want true")
> +       }
> +}
> +
>  func BenchmarkClientServer(b *testing.B) {
>         b.ReportAllocs()
>         b.StopTimer()
> Index: src/pkg/net/http/server.go
> ===================================================================
> --- a/src/pkg/net/http/server.go
> +++ b/src/pkg/net/http/server.go
> @@ -22,6 +22,7 @@
>         "strconv"
>         "strings"
>         "sync"
> +       "sync/atomic"
>         "time"
>  )
>
> @@ -703,6 +704,7 @@
>         cw.wroteHeader = true
>
>         w := cw.res
> +       sendClose := !w.conn.server.doKeepAlives()
>         isHEAD := w.req.Method == "HEAD"
>
>         // header is written out to w.conn.buf below. Depending on the
> @@ -750,7 +752,7 @@
>
>         // If this was an HTTP/1.0 request with keep-alive and we sent a
>         // Content-Length back, we can make this a keep-alive response ...
> -       if w.req.wantsHttp10KeepAlive() {
> +       if w.req.wantsHttp10KeepAlive() && !sendClose {
>                 sentLength := header.get("Content-Length") != ""
>                 if sentLength && header.get("Connection") == "keep-alive" {
>                         w.closeAfterReply = false
> @@ -769,7 +771,7 @@
>                 w.closeAfterReply = true
>         }
>
> -       if header.get("Connection") == "close" {
> +       if header.get("Connection") == "close" || sendClose {
>                 w.closeAfterReply = true
>         }
>
> @@ -851,7 +853,7 @@
>                 return
>         }
>
> -       if w.closeAfterReply && !hasToken(cw.header.get("Connection"),
> "close") {
> +       if w.closeAfterReply && (sendClose ||
!hasToken(cw.header.get("Connection"),
> "close")) {
>                 delHeader("Connection")
>                 if w.req.ProtoAtLeast(1, 1) {
>                         setHeader.connection = "close"
> @@ -1564,6 +1566,7 @@
>  }
>
>  // A Server defines parameters for running an HTTP server.
> +// The zero value for Server is a valid configuration.
>  type Server struct {
>         Addr           string        // TCP address to listen on, ":http"
> if empty
>         Handler        Handler       // handler to invoke,
> http.DefaultServeMux if nil
> @@ -1580,6 +1583,8 @@
>         // and RemoteAddr if not already set.  The connection is
>         // automatically closed when the function returns.
>         TLSNextProto map[string]func(*Server, *tls.Conn, Handler)
> +
> +       disableKeepAlives int32 // accessed atomically.
>  }
>
>  // serverHandler delegates to either the server's Handler or
> @@ -1647,6 +1652,22 @@
>         }
>  }
>
> +func (s *Server) doKeepAlives() bool {
> +       return atomic.LoadInt32(&s.disableKeepAlives) == 0
> +}
> +
> +// SetKeepAlivesEnabled controls whether HTTP keep-alives are enabled.
> +// By default, keep-alives are always enabled. Only very
> +// resource-constrained environments or servers in the process of
> +// shutting down should disable them.
> +func (s *Server) SetKeepAlivesEnabled(v bool) {
> +       if v {
> +               atomic.StoreInt32(&s.disableKeepAlives, 0)
> +       } else {
> +               atomic.StoreInt32(&s.disableKeepAlives, 1)
> +       }
> +}
> +
>  // ListenAndServe listens on the TCP network address addr
>  // and then calls Serve with handler to handle requests
>  // on incoming connections.  Handler is typically nil,
>
>
>

[bradfitz, 2014-02-28 00:08:00]

Zero value
On Feb 27, 2014 4:02 PM, "Andrew Gerrand" <adg@golang.org> wrote:

> The names here are a bit inconsistent. The field is disableKeepAlives but
> the function is SetKeepAlivesEnabled and the var is sendClose.
>
> Can we just use keepAliveEnabled for the field, SetKeepAliveEnabled for
> the method, and keepAliveEnabled for the local var?
>
> Otherwise the general functionality and implementation looks good.
>
> On Friday, 28 February 2014, <bradfitz@golang.org> wrote:
>
>> Reviewers: adg, josharian, rcrowley,
>>
>> Message:
>> Hello adg@golang.org, josharian@gmail.com, r@rcrowley.org (cc:
>> golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> net/http: add Server.SetKeepAlivesEnabled
>>
>> Part of graceful shutdown.
>>
>> Update Issue 4674
>>
>> Please review this at https://codereview.appspot.com/69670043/
>>
>> Affected files (+40, -3 lines):
>>   M src/pkg/net/http/serve_test.go
>>   M src/pkg/net/http/server.go
>>
>>
>> Index: src/pkg/net/http/serve_test.go
>> ===================================================================
>> --- a/src/pkg/net/http/serve_test.go
>> +++ b/src/pkg/net/http/serve_test.go
>> @@ -2243,6 +2243,22 @@
>>         }
>>  }
>>
>> +func TestServerKeepAlivesEnabled(t *testing.T) {
>> +       defer afterTest(t)
>> +       ts := httptest.NewUnstartedServer(HandlerFunc(func(w
>> ResponseWriter, r *Request) {}))
>> +       ts.Config.SetKeepAlivesEnabled(false)
>> +       ts.Start()
>> +       defer ts.Close()
>> +       res, err := Get(ts.URL)
>> +       if err != nil {
>> +               t.Fatal(err)
>> +       }
>> +       defer res.Body.Close()
>> +       if !res.Close {
>> +               t.Errorf("Body.Close == false; want true")
>> +       }
>> +}
>> +
>>  func BenchmarkClientServer(b *testing.B) {
>>         b.ReportAllocs()
>>         b.StopTimer()
>> Index: src/pkg/net/http/server.go
>> ===================================================================
>> --- a/src/pkg/net/http/server.go
>> +++ b/src/pkg/net/http/server.go
>> @@ -22,6 +22,7 @@
>>         "strconv"
>>         "strings"
>>         "sync"
>> +       "sync/atomic"
>>         "time"
>>  )
>>
>> @@ -703,6 +704,7 @@
>>         cw.wroteHeader = true
>>
>>         w := cw.res
>> +       sendClose := !w.conn.server.doKeepAlives()
>>         isHEAD := w.req.Method == "HEAD"
>>
>>         // header is written out to w.conn.buf below. Depending on the
>> @@ -750,7 +752,7 @@
>>
>>         // If this was an HTTP/1.0 request with keep-alive and we sent a
>>         // Content-Length back, we can make this a keep-alive response ...
>> -       if w.req.wantsHttp10KeepAlive() {
>> +       if w.req.wantsHttp10KeepAlive() && !sendClose {
>>                 sentLength := header.get("Content-Length") != ""
>>                 if sentLength && header.get("Connection") == "keep-alive"
>> {
>>                         w.closeAfterReply = false
>> @@ -769,7 +771,7 @@
>>                 w.closeAfterReply = true
>>         }
>>
>> -       if header.get("Connection") == "close" {
>> +       if header.get("Connection") == "close" || sendClose {
>>                 w.closeAfterReply = true
>>         }
>>
>> @@ -851,7 +853,7 @@
>>                 return
>>         }
>>
>> -       if w.closeAfterReply && !hasToken(cw.header.get("Connection"),
>> "close") {
>> +       if w.closeAfterReply && (sendClose ||
!hasToken(cw.header.get("Connection"),
>> "close")) {
>>                 delHeader("Connection")
>>                 if w.req.ProtoAtLeast(1, 1) {
>>                         setHeader.connection = "close"
>> @@ -1564,6 +1566,7 @@
>>  }
>>
>>  // A Server defines parameters for running an HTTP server.
>> +// The zero value for Server is a valid configuration.
>>  type Server struct {
>>         Addr           string        // TCP address to listen on, ":http"
>> if empty
>>         Handler        Handler       // handler to invoke,
>> http.DefaultServeMux if nil
>> @@ -1580,6 +1583,8 @@
>>         // and RemoteAddr if not already set.  The connection is
>>         // automatically closed when the function returns.
>>         TLSNextProto map[string]func(*Server, *tls.Conn, Handler)
>> +
>> +       disableKeepAlives int32 // accessed atomically.
>>  }
>>
>>  // serverHandler delegates to either the server's Handler or
>> @@ -1647,6 +1652,22 @@
>>         }
>>  }
>>
>> +func (s *Server) doKeepAlives() bool {
>> +       return atomic.LoadInt32(&s.disableKeepAlives) == 0
>> +}
>> +
>> +// SetKeepAlivesEnabled controls whether HTTP keep-alives are enabled.
>> +// By default, keep-alives are always enabled. Only very
>> +// resource-constrained environments or servers in the process of
>> +// shutting down should disable them.
>> +func (s *Server) SetKeepAlivesEnabled(v bool) {
>> +       if v {
>> +               atomic.StoreInt32(&s.disableKeepAlives, 0)
>> +       } else {
>> +               atomic.StoreInt32(&s.disableKeepAlives, 1)
>> +       }
>> +}
>> +
>>  // ListenAndServe listens on the TCP network address addr
>>  // and then calls Serve with handler to handle requests
>>  // on incoming connections.  Handler is typically nil,
>>
>>
>>  --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[adg, 2014-02-28 00:15:17]

Zero values notwithstanding, can you make these changes then?

https://codereview.appspot.com/69670043/diff/40001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/69670043/diff/40001/src/pkg/net/http/server.go...
src/pkg/net/http/server.go:707: sendClose := !w.conn.server.doKeepAlives()
keepAlivesEnabled

https://codereview.appspot.com/69670043/diff/40001/src/pkg/net/http/server.go...
src/pkg/net/http/server.go:1655: func (s *Server) doKeepAlives() bool {
keepAlivesEnabled

[rcrowley, 2014-02-28 01:59:04]

On 2014/02/27 23:35:58, bradfitz wrote:
> Hello mailto:adg@golang.org, mailto:josharian@gmail.com, mailto:r@rcrowley.org
(cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I'm not sure this CL is strictly necessary though it's certainly a nicer
interface than wrapping http.ResponseWriter to inject the Connection: close
header.

Is this intended to work in conjunction with the ConnState CL in order to
preempt idle keepalive connections?

[bradfitz, 2014-02-28 02:09:19]

Done.



On Thu, Feb 27, 2014 at 4:15 PM, <adg@golang.org> wrote:

> Zero values notwithstanding, can you make these changes then?
>
>
> https://codereview.appspot.com/69670043/diff/40001/src/
> pkg/net/http/server.go
> File src/pkg/net/http/server.go (right):
>
> https://codereview.appspot.com/69670043/diff/40001/src/
> pkg/net/http/server.go#newcode707
> src/pkg/net/http/server.go:707: sendClose :=
> !w.conn.server.doKeepAlives()
> keepAlivesEnabled
>
> https://codereview.appspot.com/69670043/diff/40001/src/
> pkg/net/http/server.go#newcode1655
> src/pkg/net/http/server.go:1655: func (s *Server) doKeepAlives() bool {
> keepAlivesEnabled
>
> https://codereview.appspot.com/69670043/
>

[bradfitz, 2014-02-28 02:09:30]

Hello adg@golang.org, josharian@gmail.com, r@rcrowley.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-02-28 02:11:50]

On Thu, Feb 27, 2014 at 5:59 PM, <r@rcrowley.org> wrote:

> On 2014/02/27 23:35:58, bradfitz wrote:
>
>> Hello mailto:adg@golang.org, mailto:josharian@gmail.com,
>>
> mailto:r@rcrowley.org (cc:
>
>> mailto:golang-codereviews@googlegroups.com),
>>
>
>  I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>
> I'm not sure this CL is strictly necessary though it's certainly a nicer
> interface than wrapping http.ResponseWriter to inject the Connection:
> close header.
>
> Is this intended to work in conjunction with the ConnState CL in order
> to preempt idle keepalive connections?


Yes. This + the ConnState change make it pretty trivial to do the shutdown
thing easily, without much grossness.

[adg, 2014-02-28 08:03:50]

LGTM

[josharian, 2014-02-28 13:30:36]

LGTM

[bradfitz, 2014-02-28 15:40:28]

*** Submitted as https://code.google.com/p/go/source/detail?r=d9a3ecc457c5 ***

net/http: add Server.SetKeepAlivesEnabled

Part of graceful shutdown.

Update Issue 4674

LGTM=adg, josharian
R=adg, josharian, r
CC=golang-codereviews
https://codereview.appspot.com/69670043