|
|
Descriptionnet/http: add Server.SetKeepAlivesEnabled
Part of graceful shutdown.
Update Issue 4674
Patch Set 1 #Patch Set 2 : diff -r a834e3b6891d https://go.googlecode.com/hg/ #Patch Set 3 : diff -r a834e3b6891d https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 4 : diff -r a834e3b6891d https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 7a7a12d7b8c0 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 3d37606fb793 https://go.googlecode.com/hg/ #MessagesTotal messages: 12
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/
Sign in to reply to this message.
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, > > >
Sign in to reply to this message.
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. >
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
Hello adg@golang.org, josharian@gmail.com, r@rcrowley.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** 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
Sign in to reply to this message.
|