code review 7273049: net/http: allow graceful server shutdown

net/http: allow graceful server shutdown

Adds Close method to Server.
Fixes issue 4674.

Benchmark results from Intel(R) Core(TM)2 Duo CPU     P8700  @ 2.53GHz linux/amd64:

benchmark            old ns/op    new ns/op    delta
BenchmarkServer         164948       169391   +2.69%
BenchmarkServer-2       139033       142452   +2.46%

[pah, 2013-02-04 20:12:56]

Hello bradfitz@golang.org, golang-dev@googlegroups.com (cc:
golang-dev@googlegroups.com, patrick.allen.higgins@gmail.com),

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

[bradfitz, 2013-02-04 20:41:05]

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

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:790: c.server.mu.RLock()
if we have this notion of closing, we'd probably want to use it in the policy of
whether to send the client "Connection: close" HTTP responses, too.

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:796: w, err := c.readRequest()
this could be blocked for a very long time.

we'd probably want to keep track of:

   inKeepAlive map[net.Conn]bool

so then on Close, we could close any net.Conn which is being read from.

but this is all starts to feel like a lot of work for something that'll be
rarely used.

makes me wonder what's the minimum we could provide so others can do this
themselves?

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1242: wg sync.WaitGroup // keeps track of all service
goroutines
s/service/handler/?

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1308: defer srv.wg.Done()
just put this in c.serve() at the top?

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1320: func (srv *Server) Close() {
Close() conventionally returns error (e.g. io.Closer).

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1322: srv.closing = true
what about people already stuck in Accept?  Don't you also want to close the
listener?

[pah, 2013-02-04 21:16:05]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com,
patrick.allen.higgins@gmail.com),

Please take another look.

[pah, 2013-02-04 21:17:09]

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

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:790: c.server.mu.RLock()
On 2013/02/04 20:41:05, bradfitz wrote:
> if we have this notion of closing, we'd probably want to use it in the policy
of
> whether to send the client "Connection: close" HTTP responses, too.

Done.

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:796: w, err := c.readRequest()
On 2013/02/04 20:41:05, bradfitz wrote:
> this could be blocked for a very long time.
> 
> we'd probably want to keep track of:
> 
>    inKeepAlive map[net.Conn]bool
> 
> so then on Close, we could close any net.Conn which is being read from.
> 
> but this is all starts to feel like a lot of work for something that'll be
> rarely used.
> 
> makes me wonder what's the minimum we could provide so others can do this
> themselves?

This is why Close documents that it should be called after closing the listener
and setting a ReadTimeout on the server.

There are test cases for the slow path (no read timeout and unclosed listener)
and the fast(er) path.

I agree that it would be good to provide something simpler so that more complex
policies can be implemented by others.

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1242: wg sync.WaitGroup // keeps track of all service
goroutines
On 2013/02/04 20:41:05, bradfitz wrote:
> s/service/handler/? 

Done. Also added note that the accept loop is tracked.

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1308: defer srv.wg.Done()
On 2013/02/04 20:41:05, bradfitz wrote:
> just put this in c.serve() at the top?

Done.

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1320: func (srv *Server) Close() {
On 2013/02/04 20:41:05, bradfitz wrote:
> Close() conventionally returns error (e.g. io.Closer).

Done.

https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1322: srv.closing = true
On 2013/02/04 20:41:05, bradfitz wrote:
> what about people already stuck in Accept?  Don't you also want to close the
> listener?

I don't know how this could be done without making the listener a part of the
Server struct. Right now I think Serve could be called on the same *Server
multiple times with different listeners, no?

This is why Close documents that the listener should be closed before calling
Close.

[bradfitz, 2013-02-04 21:59:22]

We should decide what Close should even do before implementing it.

I think it should probably close the listener (which means retaining a
private reference to it), close any keep-alive connections, wait for
existing handlers to finish.  But do we force-kill open connections after
some timeout?  Or do we return an error from Close if it's taking too long
and/or there are no Read/WriteTimeouts set on the Server?

etc, etc.

But I think the currently API is kinda weird and requires a lot of reading.

Or we go super light and only do the WaitGroup thing and let other people
pass in a net.Listener which does all the tracking/closing.


On Mon, Feb 4, 2013 at 1:17 PM, <patrick.allen.higgins@gmail.com> wrote:

>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go>
> File src/pkg/net/http/server.go (right):
>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go#newcode790<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#newcode790>
> src/pkg/net/http/server.go:**790: c.server.mu.RLock()
> On 2013/02/04 20:41:05, bradfitz wrote:
>
>> if we have this notion of closing, we'd probably want to use it in the
>>
> policy of
>
>> whether to send the client "Connection: close" HTTP responses, too.
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go#newcode796<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#newcode796>
> src/pkg/net/http/server.go:**796: w, err := c.readRequest()
> On 2013/02/04 20:41:05, bradfitz wrote:
>
>> this could be blocked for a very long time.
>>
>
>  we'd probably want to keep track of:
>>
>
>      inKeepAlive map[net.Conn]bool
>>
>
>  so then on Close, we could close any net.Conn which is being read
>>
> from.
>
>  but this is all starts to feel like a lot of work for something
>>
> that'll be
>
>> rarely used.
>>
>
>  makes me wonder what's the minimum we could provide so others can do
>>
> this
>
>> themselves?
>>
>
> This is why Close documents that it should be called after closing the
> listener and setting a ReadTimeout on the server.
>
> There are test cases for the slow path (no read timeout and unclosed
> listener) and the fast(er) path.
>
> I agree that it would be good to provide something simpler so that more
> complex policies can be implemented by others.
>
>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go#newcode1242<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#newcode1242>
> src/pkg/net/http/server.go:**1242: wg sync.WaitGroup // keeps track of all
> service goroutines
> On 2013/02/04 20:41:05, bradfitz wrote:
>
>> s/service/handler/?
>>
>
> Done. Also added note that the accept loop is tracked.
>
>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go#newcode1308<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#newcode1308>
> src/pkg/net/http/server.go:**1308: defer srv.wg.Done()
> On 2013/02/04 20:41:05, bradfitz wrote:
>
>> just put this in c.serve() at the top?
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go#newcode1320<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#newcode1320>
> src/pkg/net/http/server.go:**1320: func (srv *Server) Close() {
> On 2013/02/04 20:41:05, bradfitz wrote:
>
>> Close() conventionally returns error (e.g. io.Closer).
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7273049/diff/4001/src/pkg/**
>
net/http/server.go#newcode1322<https://codereview.appspot.com/7273049/diff/4001/src/pkg/net/http/server.go#newcode1322>
> src/pkg/net/http/server.go:**1322: srv.closing = true
> On 2013/02/04 20:41:05, bradfitz wrote:
>
>> what about people already stuck in Accept?  Don't you also want to
>>
> close the
>
>> listener?
>>
>
> I don't know how this could be done without making the listener a part
> of the Server struct. Right now I think Serve could be called on the
> same *Server multiple times with different listeners, no?
>
> This is why Close documents that the listener should be closed before
> calling Close.
>
>
https://codereview.appspot.**com/7273049/<https://codereview.appspot.com/7273...
>

[pah, 2013-02-04 22:27:18]

I agree that the API is weird. I was thinking it's OK if it's a little
weird if it's expected to be rarely-used and means we don't have to do
something heavyweight for everyone.

Tracking all the keep-alive connections seems heavyweight, but it does make
the API cleaner and could put an upper-bound on shutdown time.

The shutdown timeout and force-kill behavior could be parameters to Close
(making it no longer an io.Closer), as the timeout is likely app-specific.

I'll have to think about what the net.Listener which does all the tracking
would look like. I don't know how it would be able to shutdown a long-lived
persistent connection cleanly.

[bradfitz, 2013-05-14 23:23:25]

If you'd like to resume discussing this, we now can.

[pah, 2013-06-06 04:07:51]

On 2013/05/14 23:23:25, bradfitz wrote:
> If you'd like to resume discussing this, we now can.

I am still interested and started working on it again tonight. I'd like to
measure the overhead of tracking all the keep-alive connections. I'm having a
hard time getting the tests to pass, though. Apparently a Transport is leaking
somehow and 19 allocations have been added to TestChunkReaderAllocs.

[gobot, 2013-12-20 16:21:41]

Replacing golang-dev with golang-codereviews.

[rsc, 2014-03-05 20:00:43]

R=close

timed out