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

Jump to bottom

x/net/http2: if http1 Server's keep-alives are disabled, only do one request #17717

Closed
bradfitz opened this issue Nov 1, 2016 · 5 comments
Closed

x/net/http2: if http1 Server's keep-alives are disabled, only do one request #17717

bradfitz opened this issue Nov 1, 2016 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Nov 1, 2016

As part of #9478, I noticed the HTTP/2 server doesn't respect SetKeepAlivesEnabled.

Do that.

After the first http2 Server response, check:

func (s *Server) doKeepAlives() bool {

And if it's false, start a graceful GOAWAY shutdown.

/cc @tombergan
@bradfitz bradfitz added this to the Go1.8 milestone Nov 1, 2016
@bradfitz bradfitz self-assigned this Nov 1, 2016
@tombergan
Copy link
Contributor

HTTP/2 does not support Connection: keep-alive or Connection: close.

http://httpwg.org/specs/rfc7540.html#rfc.section.8.1.2

HTTP/2 does not use the Connection header field to indicate connection-specific header fields

The HTTP/2 protocol is centered around the idea of multiple requests per connection. If someone wants one request per connection, they should just use HTTP/1. Maybe just update the SetKeepAliveEnabled docs?

@bradfitz
Copy link
Contributor Author

bradfitz commented Nov 1, 2016

Yeah, simplest case is just documentation.

Or we could even change the advertised ALPN protos at runtime to exclude "h2" if keep-alives are disabled. But h2 + immediate GOAWAY after first response isn't really worse than h1 + "Connection: close".

@odeke-em odeke-em changed the title x/net/http2: if htttp1 Server's keep-alives are disabled, only do one request x/net/http2: if http1 Server's keep-alives are disabled, only do one request Nov 7, 2016
@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 7, 2016
@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Nov 14, 2016

Sounds like there was a decision since there is now a CL. Change to NeedsFix?

gopherbot pushed a commit to golang/net that referenced this issue Nov 14, 2016
@bradfitz
http2: make Server respect http1 Server's SetKeepAlivesEnabled
6dfeb34 
If the user is using Server.SetKeepAlivesEnabled(false), assume they
have a reason and respect it. Make HTTP/2 behave like HTTP/1 (except a
bit nicer, since we have GOAWAY).

Updates golang/go#17717

Change-Id: I4a5ce324f0ac8653d83e75029063cd2e270a1048
Reviewed-on: https://go-review.googlesource.com/33153
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 14, 2016
@gopherbot
Copy link

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

@tombergan tombergan mentioned this issue Oct 27, 2017
Closed
@golang golang locked and limited conversation to collaborators Nov 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@bradfitz @rsc @quentinmit @gopherbot @tombergan