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: if http1 Server's keep-alives are disabled, only do one request #17717

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

Comments

@bradfitz
Copy link
Contributor

@bradfitz 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

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Nov 1, 2016

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

This comment has been minimized.

Copy link
Contributor Author

@bradfitz 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
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 12, 2016

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

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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
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 and removed NeedsDecision labels Nov 14, 2016
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 14, 2016

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

@gopherbot gopherbot closed this in bb00a8d Nov 14, 2016
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.