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

x/net/http2: support graceful shutdown of http2 server handlers #15386

Closed
devnev opened this issue Apr 20, 2016 · 10 comments
Closed

x/net/http2: support graceful shutdown of http2 server handlers #15386

devnev opened this issue Apr 20, 2016 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@devnev
Copy link

devnev commented Apr 20, 2016

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.

@bradfitz
Copy link
Contributor

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.

@bradfitz bradfitz added this to the Unplanned milestone Apr 20, 2016
@bradfitz bradfitz changed the title net/http2: support graceful shutdown of http2 server handlers x/net/http2: support graceful shutdown of http2 server handlers Apr 20, 2016
@devnev
Copy link
Author

devnev commented Apr 21, 2016

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.

@bradfitz
Copy link
Contributor

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.

@devnev
Copy link
Author

devnev commented Apr 21, 2016

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.

@devnev
Copy link
Author

devnev commented May 8, 2016

Hey Brad, any feedback? I still think an implementation of the HTTP2 spec's shutdown behaviour would be useful.

@bradfitz
Copy link
Contributor

bradfitz commented May 9, 2016

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.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned May 9, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@quentinmit
Copy link
Contributor

ping @devnev @bradfitz It's time to look at this more seriously :)

@bradfitz
Copy link
Contributor

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.

@tpng
Copy link

tpng commented Oct 17, 2016

grpc implemented GOAWAY on server side for graceful shutdown. grpc/grpc-go#774

@bradfitz
Copy link
Contributor

The other relevant bug that's already open is #9478. Fixing it will also involve sending GOAWAY frames for HTTP/2 connections.

@golang golang locked and limited conversation to collaborators Oct 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants