Issue 67730046: code review 67730046: net/http: graceful stop for http.Server
Description net/http: graceful stop for http.Server
This patch adds a Close method to http.Server which waits for all open
connections to be closed.
A chan struct{} indicates to goroutines that they should stop.
Busy connections close themselves after responding to the client if the
channel is closed.
bufio.Reader.Peek is used to detect idle connections earlier than
http.Server.ReadTimeout typically allows.
Fixes issue 4674.
Patch Set 1 #Patch Set 2 : diff -r 70499e5fbe5b https://code.google.com/p/go #Patch Set 3 : diff -r 3cf533be5e36 https://code.google.com/p/go #Patch Set 4 : diff -r 3cf533be5e36 https://code.google.com/p/go #Patch Set 5 : diff -r 3cf533be5e36 https://code.google.com/p/go #Patch Set 6 : diff -r 3cf533be5e36 https://code.google.com/p/go #Patch Set 7 : diff -r 3cf533be5e36 https://code.google.com/p/go #
Total comments: 22
Patch Set 8 : diff -r 3cf533be5e36 https://code.google.com/p/go #
Total comments: 2
Patch Set 9 : diff -r 3cf533be5e36 https://code.google.com/p/go #MessagesTotal messages: 23
Hello golang-codereviews@googlegroups.com (cc: bradfitz@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
What is the effect on performance? In particular I am interested in: $ go get code.google.com/p/go.benchmarks/bench $ go build code.google.com/p/go.benchmarks/bench $ GOMAXPROCS=your_number_of_cores ./bench -bench=http
Sign in to reply to this message.
On 2014/02/24 09:55:44, dvyukov wrote: > What is the effect on performance? > > In particular I am interested in: > > $ go get code.google.com/p/go.benchmarks/bench > $ go build code.google.com/p/go.benchmarks/bench > $ GOMAXPROCS=your_number_of_cores ./bench -bench=http I develop in a single-virtual-CPU VirtualBox VM on a dual-core 2GHz i7 MacBook Air. Here are benchmark results at GOMAXPROCS=1 and GOMAXPROCS=2: https://gist.github.com/rcrowley/9192703 Additionally the net/http test suite takes about half a second longer to run.
Sign in to reply to this message.
Wade Simmons pointed out that this patch will only Close the last net.Listener passed to http.Server.Serve. I think it should track all of them in a []net.Listener and Close all of them in http.Server.Close.
Sign in to reply to this message.
On 2014/02/24 18:44:33, rcrowley wrote: > Wade Simmons pointed out that this patch will only Close the last net.Listener > passed to http.Server.Serve. I think it should track all of them in a > []net.Listener and Close all of them in http.Server.Close. This is addressed in the sixth revision of this patch.
Sign in to reply to this message.
On Mon, Feb 24, 2014 at 10:20 PM, <r@rcrowley.org> wrote: > On 2014/02/24 09:55:44, dvyukov wrote: >> >> What is the effect on performance? > > >> In particular I am interested in: > > >> $ go get code.google.com/p/go.benchmarks/bench >> $ go build code.google.com/p/go.benchmarks/bench >> $ GOMAXPROCS=your_number_of_cores ./bench -bench=http > > > I develop in a single-virtual-CPU VirtualBox VM on a dual-core 2GHz i7 > MacBook Air. Here are benchmark results at GOMAXPROCS=1 and > GOMAXPROCS=2: > > https://gist.github.com/rcrowley/9192703 > > Additionally the net/http test suite takes about half a second longer to > run. > > https://codereview.appspot.com/67730046/ 5.62% slower with GOMAXPROCS=2 when/if we decide to submit it, let me do more extensive tests on a machine with 32 hardware threads
Sign in to reply to this message.
> 5.62% slower with GOMAXPROCS=2 > > when/if we decide to submit it, let me do more extensive tests on a > machine with 32 hardware threads I have seen variances in this benchmark in my environment of about 8% so a more pristine benchmarking environment would be very welcome. I have been profiling a bit more on my own and found that about half of the performance regression can be attributed to the Peek loop in conn.readRequest and the other half to the addition of the sync.WaitGroup and friends.
Sign in to reply to this message.
https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:119: remove this blank line. the following is also guarded by mu. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:120: ch <-chan struct{} // semaphore indicating the server is stopping // shutdownc is closed when Server.Close is called. // If nil, the server has already been closed. shutdownc <-chan struct{} But if you make this a named type, like: type shutdownSignal chan struct{} Then you can put methods on it, like isClosed() or isClosing(), etc. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:121: waitGroup *sync.WaitGroup // coordination for gracfully stopping the server these fields should be named something more than their type. Note that "clientGone" above isn't declared as: "boolThing bool" https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:334: ch <-chan struct{} // semaphore indicating the server is stopping something more than "ch". And more descriptive comment, avoiding the word semaphore. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:582: for dPeek, dPeekDone := idleConnTimeout, d-idleConnTimeout; d == 0 || dPeek < dPeekDone; dPeek += idleConnTimeout { this line gives me a headache I'm not sure the dPrefix is helping make clear that it's a duration. Not a convention I've seen elsewhere. Can this be simplified? https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:896: select { I'd make this a method. Then we might find that an atomic memory read is a better implementation anyway, but this code won't change once it's a method call. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1635: mutex sync.Mutex // synchronize access to ch and listeners mutex is typically named "mu". And you put it before what it guards with space before and after the guarded section. comment should say "guards foo", not "synchronize". https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1636: waitGroup sync.WaitGroup // coordination for gracefully stopping the server needs better field name. then comment might be redundant and you could use its space (or put it on the line before) to say when it goes up and down. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1679: srv.listeners = []net.Listener{} unnecessary. you can append on to nil. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1704: select { method https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1731: if srv.listeners != nil { unnecessary. you can range over nil.
Sign in to reply to this message.
What if we make this opt-in and add a new field to Server and require it be set before Serve is called? Anyway, I've left some comments. But in addition to the comments, I think making this opt-in might be the only way to be acceptable performance-wise for the default case. Then Close can return an error if you didn't configure it to be closeable? On Tue, Feb 25, 2014 at 10:22 AM, <r@rcrowley.org> wrote: > 5.62% slower with GOMAXPROCS=2 >> > > when/if we decide to submit it, let me do more extensive tests on a >> machine with 32 hardware threads >> > > I have seen variances in this benchmark in my environment of about 8% so > a more pristine benchmarking environment would be very welcome. > > I have been profiling a bit more on my own and found that about half of > the performance regression can be attributed to the Peek loop in > conn.readRequest and the other half to the addition of the > sync.WaitGroup and friends. > > https://codereview.appspot.com/67730046/ >
Sign in to reply to this message.
This refactor addresses all of Brad's comments but not the proposal to make graceful stop optional. I've updated the benchmarks, too, and it's now only 2.96% slower at GOMAXPROCS=2: https://gist.github.com/rcrowley/9192703. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:119: On 2014/02/25 19:15:10, bradfitz wrote: > remove this blank line. the following is also guarded by mu. Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:120: ch <-chan struct{} // semaphore indicating the server is stopping On 2014/02/25 19:15:10, bradfitz wrote: > // shutdownc is closed when Server.Close is called. > // If nil, the server has already been closed. > shutdownc <-chan struct{} > > But if you make this a named type, like: > > type shutdownSignal chan struct{} > > Then you can put methods on it, like isClosed() or isClosing(), etc. Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:121: waitGroup *sync.WaitGroup // coordination for gracfully stopping the server On 2014/02/25 19:15:10, bradfitz wrote: > these fields should be named something more than their type. > > Note that "clientGone" above isn't declared as: > > "boolThing bool" I looked through the standard library and didn't find any inspiration for naming this so I changed it to wg to follow convention. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:334: ch <-chan struct{} // semaphore indicating the server is stopping On 2014/02/25 19:15:10, bradfitz wrote: > something more than "ch". And more descriptive comment, avoiding the word > semaphore. Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:582: for dPeek, dPeekDone := idleConnTimeout, d-idleConnTimeout; d == 0 || dPeek < dPeekDone; dPeek += idleConnTimeout { On 2014/02/25 19:15:10, bradfitz wrote: > this line gives me a headache > > I'm not sure the dPrefix is helping make clear that it's a duration. Not a > convention I've seen elsewhere. > > Can this be simplified? Sort of. I've broken it apart into two for loops that are much easier to read but because of the break and return they look a little duplicative. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:896: select { On 2014/02/25 19:15:10, bradfitz wrote: > I'd make this a method. Then we might find that an atomic memory read is a > better implementation anyway, but this code won't change once it's a method > call. Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1635: mutex sync.Mutex // synchronize access to ch and listeners On 2014/02/25 19:15:10, bradfitz wrote: > mutex is typically named "mu". And you put it before what it guards with space > before and after the guarded section. comment should say "guards foo", not > "synchronize". Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1636: waitGroup sync.WaitGroup // coordination for gracefully stopping the server On 2014/02/25 19:15:10, bradfitz wrote: > needs better field name. then comment might be redundant and you could use its > space (or put it on the line before) to say when it goes up and down. As above, I looked through the standard library and didn't find any inspiration for naming this so I changed it to wg to follow convention. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1679: srv.listeners = []net.Listener{} On 2014/02/25 19:15:10, bradfitz wrote: > unnecessary. you can append on to nil. Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1704: select { On 2014/02/25 19:15:10, bradfitz wrote: > method Done. https://codereview.appspot.com/67730046/diff/120001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:1731: if srv.listeners != nil { On 2014/02/25 19:15:10, bradfitz wrote: > unnecessary. you can range over nil. Done.
Sign in to reply to this message.
On 2014/02/25 19:16:07, bradfitz wrote: > What if we make this opt-in and add a new field to Server and require it be > set before Serve is called? > > Anyway, I've left some comments. But in addition to the comments, I think > making this opt-in might be the only way to be acceptable performance-wise > for the default case. Then Close can return an error if you didn't > configure it to be closeable? If I recall correctly, Brad, your main concern with this was the potential to confuse new users. I agree that a flag is preferable to a new GracefulServer type so I'll refactor to support that tonight. And while we're debating specifics, how do you feel about calling the method Stop instead of Close? In the code, the comments, and out loud, Stop feels much more natural.
Sign in to reply to this message.
On Wednesday, February 26, 2014 8:52:11 PM UTC+1, r...@rcrowley.org wrote: > And while we're debating specifics, how do you feel about calling the > method Stop instead of Close? In the code, the comments, and out loud, > Stop feels much more natural. is foo.Stop() foo.Serve(listener) a valid sequence? If not, then Shutdown might sound more final.
Sign in to reply to this message.
On Wednesday, February 26, 2014 12:25:29 PM UTC-8, night...@googlemail.com wrote: > On Wednesday, February 26, 2014 8:52:11 PM UTC+1, r...@rcrowley.org wrote: > > And while we're debating specifics, how do you feel about calling the > > method Stop instead of Close? In the code, the comments, and out loud, > > Stop feels much more natural. > > is > > foo.Stop() > foo.Serve(listener) > > a valid sequence? If not, then Shutdown might sound more final. net.Listener uses Close() and so does httptest.Server
Sign in to reply to this message.
On 2014/02/26 20:25:30, ioe wrote: > On Wednesday, February 26, 2014 8:52:11 PM UTC+1, mailto:r...@rcrowley.org wrote: > > And while we're debating specifics, how do you feel about calling the > > method Stop instead of Close? In the code, the comments, and out loud, > > Stop feels much more natural. > > is > > foo.Stop() > foo.Serve(listener) > > a valid sequence? If not, then Shutdown might sound more final. Yes, it is. I should add a test if we want to preserve that possibility.
Sign in to reply to this message.
> net.Listener uses Close() and so does httptest.Server I'm reading this as an argument in favor of consistency. Is that right? I think that's a very valid reason to call it Close. I have been thinking about it and don't feel like implementing io.Closer is very meaningful so that really leaves the consistency argument.
Sign in to reply to this message.
On Wed, Feb 26, 2014 at 1:28 PM, <r@rcrowley.org> wrote: > net.Listener uses Close() and so does httptest.Server >> > > I'm reading this as an argument in favor of consistency. Is that right? > I think that's a very valid reason to call it Close. > > I have been thinking about it and don't feel like implementing io.Closer > is very meaningful so that really leaves the consistency argument. Yeah, just for consistency's sake. httptest.Server doesn't conform to io.Closer either, and it seems like net.Listener implementing that is more incidental than anything since you can't do any other kind of io on it. Anyway, that's just my $0.0178
Sign in to reply to this message.
On 2014/02/26 21:34:52, kisielk wrote: > On Wed, Feb 26, 2014 at 1:28 PM, <mailto:r@rcrowley.org> wrote: > > > net.Listener uses Close() and so does httptest.Server > >> > > > > I'm reading this as an argument in favor of consistency. Is that right? > > I think that's a very valid reason to call it Close. > > > > I have been thinking about it and don't feel like implementing io.Closer > > is very meaningful so that really leaves the consistency argument. > > > Yeah, just for consistency's sake. httptest.Server doesn't conform to > io.Closer either, and it seems like net.Listener implementing that is more > incidental than anything since you can't do any other kind of io on it. > > Anyway, that's just my $0.0178 Close it is, then! My ambivalence isn't nearly enough to change it.
Sign in to reply to this message.
Richard, I've sent out https://codereview.appspot.com/69260044 --- would that allow you do what you need without this CL? I'm a little worried about the invasiveness of this CL so late in the cycle, and picking a shutdown policy. (e.g. no grace period where we set ReadTimeout to something low-ish like 5 seconds on idle connections to prevent really active clients from getting interrupted?). It's possible we could provide a server.Shutdown(grace time.Duration) or server.Close() or something in the future, but I'd rather not decide now. If the connection state tracking hook is enough, I'm more comfortable with that and maintaining it going forward. It's very low cost and easy to document and also to ignore. On Wed, Feb 26, 2014 at 1:45 PM, <r@rcrowley.org> wrote: > On 2014/02/26 21:34:52, kisielk wrote: > > On Wed, Feb 26, 2014 at 1:28 PM, <mailto:r@rcrowley.org> wrote: >> > > > net.Listener uses Close() and so does httptest.Server >> >> >> > >> > I'm reading this as an argument in favor of consistency. Is that >> > right? > >> > I think that's a very valid reason to call it Close. >> > >> > I have been thinking about it and don't feel like implementing >> > io.Closer > >> > is very meaningful so that really leaves the consistency argument. >> > > > Yeah, just for consistency's sake. httptest.Server doesn't conform to >> io.Closer either, and it seems like net.Listener implementing that is >> > more > >> incidental than anything since you can't do any other kind of io on >> > it. > > Anyway, that's just my $0.0178 >> > > Close it is, then! My ambivalence isn't nearly enough to change it. > > https://codereview.appspot.com/67730046/ >
Sign in to reply to this message.
https://codereview.appspot.com/67730046/diff/130001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/67730046/diff/130001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:913: if w.conn.server.stopping() { this accessing stoppignc without a lock, and another call to Serve(net.Listener) would mutate it as you're trying to access it. the race detector would flag this if there were a test that did a concurrent request and + call to Serve.
Sign in to reply to this message.
https://codereview.appspot.com/67730046/diff/130001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/67730046/diff/130001/src/pkg/net/http/server.g... src/pkg/net/http/server.go:913: if w.conn.server.stopping() { On 2014/02/26 22:49:19, bradfitz wrote: > this accessing stoppignc without a lock, and another call to Serve(net.Listener) > would mutate it as you're trying to access it. the race detector would flag > this if there were a test that did a concurrent request and + call to Serve. There was one other place where this was the case and a further problem with the channel when Serve is called multiple times. To address both of these problems I've changed the implementation to use an int32 and the atomic.LoadInt32 and atomic.StoreInt32 APIs. The results on the benchmarks were promising and now the GOMAXPROCS=2 benchmark trails by 3.41% in my environment.
Sign in to reply to this message.
On Thu, Feb 27, 2014 at 11:14 AM, <r@rcrowley.org> wrote: > > https://codereview.appspot.com/67730046/diff/130001/src/pkg/net/http/server.go > File src/pkg/net/http/server.go (right): > > https://codereview.appspot.com/67730046/diff/130001/src/pkg/net/http/server.g... > src/pkg/net/http/server.go:913: if w.conn.server.stopping() { > On 2014/02/26 22:49:19, bradfitz wrote: >> >> this accessing stoppignc without a lock, and another call to > > Serve(net.Listener) >> >> would mutate it as you're trying to access it. the race detector > > would flag >> >> this if there were a test that did a concurrent request and + call to > > Serve. > > There was one other place where this was the case and a further problem > with the channel when Serve is called multiple times. To address both > of these problems I've changed the implementation to use an int32 and > the atomic.LoadInt32 and atomic.StoreInt32 APIs. The results on the > benchmarks were promising and now the GOMAXPROCS=2 benchmark trails by > 3.41% in my environment. If it's slowdown due to shared data structures, then they tend to degrade superlinearly when number of threads increases. So it easily can be 50% with GOMAXPROCS=32.
Sign in to reply to this message.
On 2014/02/26 22:27:18, bradfitz wrote: > Richard, > > I've sent out https://codereview.appspot.com/69260044 --- would that allow > you do what you need without this CL? I'm a little worried about the > invasiveness of this CL so late in the cycle, and picking a shutdown > policy. (e.g. no grace period where we set ReadTimeout to something > low-ish like 5 seconds on idle connections to prevent really active clients > from getting interrupted?). > > It's possible we could provide a server.Shutdown(grace time.Duration) or > server.Close() or something in the future, but I'd rather not decide now. > > If the connection state tracking hook is enough, I'm more comfortable with > that and maintaining it going forward. It's very low cost and easy to > document and also to ignore. I understand where you're coming from and this is all partially my fault for thinking the merge window closed April 1 but allow me to make a case for this API. The original requirement was to minimize new exports and we decided that implementing io.Closer met that requirement. We wanted to minimize new exports to avoid confusing new users. And to that end, we tentatively decided to turn it on by default because the performance regression was expected to be small. I believe that performance regression (currently about 3.41% on `GOMAXPROCS=2 ./bench -bench=http`) is something that should be shouldered by the standard library for a number of reasons. Stopping gracefully is an oft-requested feature that is difficult to implement entirely outside net/http. When it is implemented it comes with caveats like having to micromanage ReadTimeout, rely on Connection: close headers, and nest http.Handler and http.ResponseWriter wrappers all to approximate behavior that isn't exported by net/http. Averaged across the Go community, I bet the performance penalty of the various partial graceful stop implementations in the wild is worse than 3.41%. Lastly, 3.41% overstates the impact because real services using net/http do real work in their handlers. I don't know how to get any more scientific about that assertion, though. I'm about to work through the ConnState proposal to see how well it works in practice.
Sign in to reply to this message.
|
