Skip to content

net/http: allow upgrading non keepalive connections #36382

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

Closed
wants to merge 1 commit into from

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Jan 4, 2020

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 change ensures net/http's Server does not override the
connection header for successful protocol switch responses.

Fixes #36381.

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jan 4, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: 1a74c9e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: f7fcdcf) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 2:

(4 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 67084c3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 3:

Please revert all the unrelated cmd/go/testdata go.mod changes.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: fa35c25) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 3:

(5 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 45fe378) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: df639f8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 6: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 6:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ec0ce490


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: dbb003f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 7: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=8946b309


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 7: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@bradfitz bradfitz changed the title net/http: allow upgrading non keepalive connections net/http: fix Transport upgrading responses for non-keep-alive requests Jan 8, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: 716912d) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: 80325a2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Anmol Sethi:

Patch Set 10:

This should be good now @brad


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 21, 2020

@bradfitz From whom should I request further reviews to get this merged?

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 10:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=13146348


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 10: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 10:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@nhooyr nhooyr changed the title net/http: fix Transport upgrading responses for non-keep-alive requests net/http: allow upgrading non keepalive connections Jun 12, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: 702ce2a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: da7a944) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

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
Copy link
Contributor

This PR (HEAD: dc0de83) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/213277 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Anmol Sethi:

Patch Set 15:

(10 comments)

Thanks for the review guys.

This was in my draft for a while. Can't remember if I was still working on it but please let me know if anything else needs to be done.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Anmol Sethi:

Patch Set 15:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 6:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ec0ce490


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=8946b309


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 7: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 10:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=13146348


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 10: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 15: Code-Review+2


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 15: Run-TryBot+1 Trust+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 15:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=0210f5cd


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 15:

Build is still in progress...
This change failed on (x/tools) linux-amd64:
See https://storage.googleapis.com/go-build-log/0210f5cd/linux-amd64_e6abd2c5.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 15: TryBot-Result-1

2 of 20 TryBots failed:
Failed on (x/tools) linux-amd64: https://storage.googleapis.com/go-build-log/0210f5cd/linux-amd64_e6abd2c5.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/0210f5cd/linux-amd64_e204a81c.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 16: Trust+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Damien Neil:

Patch Set 16: Code-Review+2


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Damien Neil:

Patch Set 16: -Code-Review

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/213277.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Dec 1, 2020
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 change ensures net/http's Server does not override the
connection header for successful protocol switch responses.

Fixes #36381.

Change-Id: I882aad8539e6c87ff5f37c20e20b3a7fa1a30357
GitHub-Last-Rev: dc0de83
GitHub-Pull-Request: #36382
Reviewed-on: https://go-review.googlesource.com/c/go/+/213277
Trust: Emmanuel Odeke <[email protected]>
Trust: Brad Fitzpatrick <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/213277 has been merged.

@gopherbot gopherbot closed this Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

net/http: Allow upgrading connections even if DisableKeepAlives is set
3 participants