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: Allow upgrading connections even if DisableKeepAlives is set #36381

Closed
nhooyr opened this issue Jan 4, 2020 · 1 comment
Closed

net/http: Allow upgrading connections even if DisableKeepAlives is set #36381

nhooyr opened this issue Jan 4, 2020 · 1 comment
Labels
Milestone

Comments

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Jan 4, 2020

If the http transport has DisableKeepAlives enabled, then connection upgrading does not work. See nhooyr/websocket#185

If DisableKeepAlives is set, then the client sets Connection: close on every request which is completely fine.

The problem is that when the server encounters such a request, it prevents any http.Handler from setting the Connection header.

See

} else if !w.req.ProtoAtLeast(1, 1) || w.wantsClose {

If that is changed to:

	} else if !w.req.ProtoAtLeast(1, 1) ||
		// Only close if the request indicates the client wants to close
		// and we are not upgrading the connection.
		(w.wantsClose && header.get("Connection") != "Upgrade") {
		w.closeAfterReply = true
	}

Then everything works fine.

nhooyr added a commit to nhooyr/go that referenced this issue Jan 4, 2020
Closes golang#36381
nhooyr added a commit to nhooyr/go that referenced this issue Jan 4, 2020
Fixes golang#36381
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 4, 2020

Change https://golang.org/cl/213277 mentions this issue: net/http: allow upgrading non keepalive connections

nhooyr added a commit to nhooyr/go that referenced this issue Jan 6, 2020
Fixes golang#36381
nhooyr added a commit to nhooyr/go that referenced this issue Jan 7, 2020
Fixes golang#36381
nhooyr added a commit to nhooyr/go that referenced this issue Jan 7, 2020
Fixes golang#36381
nhooyr added a commit to nhooyr/go that referenced this issue Jan 7, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
nhooyr added a commit to nhooyr/go that referenced this issue Jan 7, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
@toothrot toothrot added this to the Backlog milestone Jan 7, 2020
nhooyr added a commit to nhooyr/go that referenced this issue Jan 7, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
nhooyr added a commit to nhooyr/go that referenced this issue Jan 9, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
nhooyr added a commit to nhooyr/go that referenced this issue Jan 9, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
nhooyr added a commit to nhooyr/go that referenced this issue Jun 12, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
nhooyr added a commit to nhooyr/go that referenced this issue Jun 12, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
nhooyr added a commit to nhooyr/go that referenced this issue Jun 12, 2020
Fixes golang#36381

If one was using http.Transport with DisableKeepAlives and trying
to upgrade a connection against net/http's Server, the Server
would not allow a "Connection: Upgrade" header to be written
and instead override it to "Connection: Close" which would
break the handshake.

This commit ensures net/http's Server does not override the
connection header for successful protocol switch responses.
@gopherbot gopherbot closed this in 50b16f9 Dec 1, 2020
@dmitshur dmitshur modified the milestones: Backlog, Go1.16 Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants