-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
x/net/http2: support graceful shutdown of http2 server handlers #15386
Comments
Let's first decide whether this bug is about client support or server support. The title says "of server handlers", but then you talk about the net/http2 client's code and changing it, but I believe it already handles this. |
The primary focus of this bug is definitely server support because that's where adding lameducking causes an API change. I think the client isn't quite handling things correctly though, as receiving a GOAWAY should cause the client to close any streams that are above the received LastStreamID, and I can't find any code to do that. I guess that's a bug in the client that we can handle separately. |
The general net/http graceful shutdown bug is #4674 We'd want to do both HTTP/1.x and HTTP/2 when we solved that. |
HTTP/2 has additional facilities related to clean shutdowns that are not present in HTTP/1.x (to my knowledge), i.e. the goaway frames. For cases where a server is HTTP/2 only, providing an API to correctly send goaway frames would allow users to implement an improved graceful shutdown using their own policies. |
Hey Brad, any feedback? I still think an implementation of the HTTP2 spec's shutdown behaviour would be useful. |
I agree it would be useful. My current focus is Go 1.7 bugs, and then I disappear in two weeks for a month. We can look at this more seriously during the Go 1.8 dev cycle. |
This bug is pretty undefined. @devnev, can you explicitly describe what you want to see here, being more explicit than "implementation of the HTTP2 spec's shutdown behaviour"? Note that we already have #4674 open to track Server's graceful shutdown. That applies to HTTP/1 and HTTP/2. The details differ, but not much. We don't plan on adding API around the http2.Transport sending GOAWAY frames. There's no real reason for a client to send GOAWAY. Even gRPC doesn't seem to care, considering that grpc/grpc-go#460 is still open. We already have Transport.CloseIdleConnections, and https://golang.org/cl/30076 to do graceful shutdown of Transport connections. I'm going to close this until I know what this bug is about. |
grpc implemented GOAWAY on server side for graceful shutdown. grpc/grpc-go#774 |
The other relevant bug that's already open is #9478. Fixing it will also involve sending GOAWAY frames for HTTP/2 connections. |
HTTP/2 supports error-free shutdown of connections by allowing peers to transmit GOAWAY frames for announcing connection shutdown and specifying which streams were processed. The gRPC wire format explicitly references this method (http://www.grpc.io/docs/guides/wire.html#goaway-frame) for clean migration of clients away from a server.
net/http2 sends GOAWAY frames for errors as specified by HTTP2, but does not yet support the graceful shutdown scenario. I propose at least adding shutdown functionality that lets all in-flight streams complete but does not accept new ones. Looking at the net/http2 client, I think it requires some minor changes to properly close streams that are invalidated by the GOAWAY.
A completely graceful shutdown goes a little further on the server side: "A server that is attempting to gracefully shut down a connection SHOULD send an initial GOAWAY frame with the last stream identifier set to (2^31)-1 and a NO_ERROR code". The standard goes on to propose waiting one RTT after this initial GOAWAY (measured from http2 pings), but it seems likely that one would have a fix upper bound for the wait time anyway (e.g. 10s) so I'm not convinced the RTT adjustment is particularly useful.
I've started writing code to this effect but would appreciate feedback and guidance through the proposal process.
The text was updated successfully, but these errors were encountered: