Skip to content
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
Closed

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

gopherbot opened this issue Jan 18, 2013 · 45 comments
Milestone

Comments

@gopherbot
Copy link

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
Copy link
Contributor

alberts commented Jan 30, 2013

Comment 1:

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

@adg
Copy link
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
Copy link
Contributor

adg commented Jan 30, 2013

Comment 3:

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

@gopherbot
Copy link
Author

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
Copy link
Author

Comment 5 by patrick.allen.higgins:

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

@gopherbot
Copy link
Author

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
Copy link
Contributor

robpike commented Mar 7, 2013

Comment 7:

Labels changed: removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 8:

Labels changed: added go1.2.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 9:

Labels changed: added feature.

@robpike
Copy link
Contributor

robpike commented Aug 29, 2013

Comment 10:

Won't make it in time for 1.2.

Labels changed: removed go1.2.

@daaku
Copy link

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
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 12:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 13:

Labels changed: removed feature.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 14:

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

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 15:

Labels changed: added repo-main.

@gopherbot
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

Comment 18:

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

@bradfitz
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
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
@leakingtapan
Copy link

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

cespare commented Oct 29, 2016

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

@dominikh
Copy link
Member

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

@bradfitz
Copy link
Contributor

"but would basically always return nil" :-)

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

@gopherbot
Copy link
Author

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

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

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
Copy link
Author

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

gopherbot pushed a commit to golang/net that referenced this issue Oct 30, 2016
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>
@deankarn
Copy link

deankarn 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
Copy link
Contributor

bradfitz commented Nov 1, 2016

@joeybloggs, filed #17721

@deankarn
Copy link

deankarn 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
Copy link
Contributor

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
Copy link

this issue has been dragging along since 1.1

@bradfitz
Copy link
Contributor

@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.
Labels
None yet
Projects
None yet
Development

No branches or pull requests