New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: add built-in graceful shutdown support to Server #4674

Closed
gopherbot opened this Issue Jan 18, 2013 · 45 comments

Comments

Projects
None yet
@gopherbot

gopherbot commented Jan 18, 2013

by patrick.allen.higgins:

http.Server only offers flavors of Serve() without a way to shut them down. Closing the
listener should make the server stop, but there seems to be a race in TLS servers where
this does not always work.

Further, in-progress requests cannot complete before exiting without some kind of
wrapper for net.Listener and net.Conn which includes synchronization between
Listener.Accept() and Conn.Close().

See https://groups.google.com/d/topic/golang-nuts/i4_kPK-rVxI/discussion for some
details.

A use-case for this is zero-downtime restarts: a master process opens a listener and
then spawns a slave, passing it the listener. When the master receives a SIGHUP, it
spawns a new slave. When the slave starts servicing requests, it signals the master and
the master signals the old slave to shutdown. The old slave must shutdown gracefully to
prevent service interruption.
@alberts

This comment has been minimized.

Contributor

alberts commented Jan 30, 2013

Comment 1:

Sounds familiar... http://tomayko.com/writings/unicorn-is-unix
@adg

This comment has been minimized.

Contributor

adg commented Jan 30, 2013

Comment 2:

You mention a few separate issues.
1. net/http doesn't offer a convenience function for net.Listen followed by http.Serve.
2. closing the underlying net.Listener doesn't shut down the http.Serve loop (because of
a race condition?)
3. there's no mechanism to gracefully shut down an http.Serve loop, waiting for any
in-flight requests to complete
I think the second point is a priority. The first doesn't seem very interesting to me.
The third point is a feature that we should explore further.
This issue should be about the third point only. Can you please file issues for the
other two points? I would especially like as much information as you have about the
second point.
Thanks

Status changed to Accepted.

@adg

This comment has been minimized.

Contributor

adg commented Jan 30, 2013

Comment 3:

Labels changed: added priority-later, go1.1maybe, packagechange, removed priority-triage, go1.1.

@gopherbot

This comment has been minimized.

gopherbot commented Jan 31, 2013

Comment 4 by patrick.allen.higgins:

I will try to boil down a test case for the second point. I didn't file an issue for it
because fixing the third point would likely fix the second as well.
@gopherbot

This comment has been minimized.

gopherbot commented Jan 31, 2013

Comment 5 by patrick.allen.higgins:

I created issue #4737, which I believe is related to the second point.
@gopherbot

This comment has been minimized.

gopherbot commented Feb 2, 2013

Comment 6 by patrick.allen.higgins:

My proposal for this issue is to put an optional *sync.WaitGroup into http.Server and
modify (*Server).Serve to call Add(1) and Done() around the creation of servicing
goroutines. That would accomplish what my net.Listener wrappers are doing.
@robpike

This comment has been minimized.

Contributor

robpike commented Mar 7, 2013

Comment 7:

Labels changed: removed go1.1maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 30, 2013

Comment 8:

Labels changed: added go1.2.

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 30, 2013

Comment 9:

Labels changed: added feature.

@robpike

This comment has been minimized.

Contributor

robpike commented Aug 29, 2013

Comment 10:

Won't make it in time for 1.2.

Labels changed: removed go1.2.

@daaku

This comment has been minimized.

daaku commented Sep 9, 2013

Comment 11:

It would be nice to have a Server.CloseIdleConnections (to kill keep-alive connections)
much like we have Transport.CloseIdleConnections for the Client. There isn't a way for
an external library to do this since the state is not exposed.
@rsc

This comment has been minimized.

Contributor

rsc commented Nov 27, 2013

Comment 12:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 27, 2013

Comment 13:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 14:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 15:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

gopherbot commented Feb 24, 2014

Comment 16 by r@rcrowley.org:

After making some headway on this problem outside the standard library I decided to take
a stab at it in net/http itself.
https://golang.org/cl/67730046/
@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 26, 2014

Comment 17:

Sent out https://golang.org/cl/69260044/ for discussion. It should be
sufficient to implement shutdown outside of net/http, with varying levels of
gracefulness.

Labels changed: added release-go1.3maybe, removed priority-later, release-none.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 27, 2014

Comment 18:

Also sent out https://golang.org/cl/69670043 for disabling keep-alives (sending
"Connection: close" in Handlers)
@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 28, 2014

Comment 19:

This issue was updated by revision 67b8bf3.

This allows for all sorts of graceful shutdown policies,
without picking a policy (e.g. lameduck period) and without
adding lots of locking to the server core. That policy and
locking can be implemented outside of net/http now.
LGTM=adg
R=golang-codereviews, josharian, r, adg, dvyukov
CC=golang-codereviews
https://golang.org/cl/69260044
@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 28, 2014

Comment 20:

This issue was updated by revision 916682e.

LGTM=adg, josharian
R=adg, josharian, r
CC=golang-codereviews
https://golang.org/cl/69670043
@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 2, 2014

Comment 21:

This issue was updated by revision 7124ee5.

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://golang.org/cl/70410044
Committer: Brad Fitzpatrick 
@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 5, 2014

Comment 22:

Status update: Go 1.3 will have all the necessary net/http changes (Server.ConnState and
Server.SetKeepAlivesEnabled) for other people to implement graceful shutdown properly,
outside the net/http package.
But because there are various policy questions of how quickly and how politely to shut
down a server, we're going to refrain from unilaterally making that policy decision now
(in the form of e.g. a Server.Close method) and instead make people pick their own
policy for now.
It's possible in the future (after some experience using different shutdown strategies
in Go 1.3) we might see the common patterns and add a convenience shutdown/close method,
perhaps with a timeout or options, once we know what the set of options are.
So punting this to Go 1.4, even though most of the work will already be in Go 1.3.

Labels changed: added release-go1.4, removed release-go1.3maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Sep 16, 2014

Comment 23:

This didn't happen for 1.4. Maybe 1.5.

Labels changed: added release-go1.5, removed release-go1.4.

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014

@bradfitz bradfitz removed the release-go1.5 label Dec 16, 2014

@leakingtapan

This comment has been minimized.

leakingtapan commented Oct 29, 2016

Any progress on this issue?

@bradfitz bradfitz modified the milestones: Go1.9, Unplanned Oct 29, 2016

@bradfitz bradfitz self-assigned this Oct 29, 2016

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 29, 2016

No, but I think a plan has started to form. It'll be a blocking:

func (s *Server) Shutdown(context.Context) error {}
func (s *Server) Close() error {}

Where the first one is graceful and takes a context (so you can abort early if it's taking too long), and the second one is forced and immediate. It would only return an error to fit the normal io.Closer signature, but would basically always return nil, even if it broke a bunch of in-use connections.

The Go 1.8 dev tree closes Sunday night, so I'll mark this as Go 1.9.

@dominikh

This comment has been minimized.

Member

dominikh commented Oct 29, 2016

Is there actually any point in implementing io.Closer? I don't think that an HTTP server really falls into that category, or that one would want to use it interchangeably with other io.Closers

@cespare

This comment has been minimized.

Contributor

cespare commented Oct 29, 2016

@dominikh Given that Listener.Close returns an error, wouldn't you want to pass that on?

@dominikh

This comment has been minimized.

Member

dominikh commented Oct 29, 2016

@cespare Good point. I was thrown off by Brad saying that it would always return nil.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 29, 2016

"but would basically always return nil" :-)

Yeah, passing on the net.Listener(s)' return value makes sense.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 30, 2016

CL https://golang.org/cl/32329 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.8, Go1.9 Oct 30, 2016

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 30, 2016

I managed to implement this in a few hours before the freeze, and people have been wanting this forever, so Go 1.8 it is.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 30, 2016

CL https://golang.org/cl/32412 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 30, 2016

http2: add support for graceful shutdown of Server
This adds support for gracefully shutting down the Server, including
sending a GOAWAY frame to clients and shutting down when things are
idle. For now this support is only when integrated with the standard
library's Server.

A future change could export some of this.

Updates golang/go#4674

Change-Id: I78cd4f58ca529bf9d149054f929d9089e7685875
Reviewed-on: https://go-review.googlesource.com/32412
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@gopherbot gopherbot closed this in 53fc330 Nov 1, 2016

@joeybloggs

This comment has been minimized.

joeybloggs commented Nov 1, 2016

I may be missing something, but it looks like hijacked connections are just removed from the tracked connections and if I'm not mistaken WebSockets hijack the connection; so my question is, with this change how would one ensure graceful shutdown of connections including WebSockets?

It appears that they would be interrupted/closed after all normal http idle connections are completed and the process ends.

I've only been able to do it using a custom listener that keeps track of the connections created and closed, ConnState for keeping track of idle KeepAlive connections all bundled in a library, kms, that exposes a ShutdownInitialized channel that WebSockets, and anything else, can use to gracefully shut themselves down with.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 1, 2016

@joeybloggs

This comment has been minimized.

joeybloggs commented Nov 1, 2016

@bradfitz out of curiosity along with the current implementation, why not provide a ShutdownInitialized channel that is accessible via a function from the *Server(or some other means like http.Request which would be more convenient) and then keep track of all of the connections via a WaitGroup, by wrapping the Server's listener and then hijacked connections would have the option of gracefully shutting down by using the ShutDownInitialized.

then server.Shutdown() can wait using the WaitGroup(), context and Timeout.

It just seems like having graceful shutdown built into net/http that doesn't support some pretty common cases seems incomplete/wrong somehow.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 1, 2016

Let's comment on new issue(s) instead of a closed issue. The new issue(s) can reference this one, and then they'll be cross-linked. Also, it's more constructive (at least for me) if you explain the problematic or desired behavior, and not the implementation. The implementation is generally straight-forward given the requirements.

@rami-dabain

This comment has been minimized.

rami-dabain commented Apr 27, 2017

this issue has been dragging along since 1.1

@bradfitz

This comment has been minimized.

Member

bradfitz commented Apr 27, 2017

@rami-dabain, this is a closed issue. This was added to Go 1.8: https://golang.org/doc/go1.8#http_shutdown

@golang golang locked and limited conversation to collaborators Apr 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.