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.

Sign up for GitHub

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

net/http: Re-connect with upgraded HTTP2 connection fails to send Request.body [1.14 backport] #39279

Closed
gopherbot opened this issue May 27, 2020 · 5 comments
Closed

net/http: Re-connect with upgraded HTTP2 connection fails to send Request.body [1.14 backport] #39279

gopherbot opened this issue May 27, 2020 · 5 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.14.9

Comments

@gopherbot
Copy link
Contributor

@networkimprov requested issue #32441 to be considered for backport to the next 1.14 minor release.

@gopherbot please backport, as Kubernetes is affected.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label May 27, 2020
@gopherbot gopherbot mentioned this issue May 27, 2020
Closed
@gopherbot gopherbot added this to the Go1.14.4 milestone May 27, 2020
@andybons
Copy link
Member

Approved as this is a serious enough regression with no suitable workaround.

@andybons andybons added the CherryPickApproved Used during the release process for point releases label May 27, 2020
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label May 27, 2020
@dmitshur dmitshur modified the milestones: Go1.14.4, Go1.14.5 Jun 1, 2020
@gopherbot
Copy link
Contributor Author

Change https://golang.org/cl/242117 mentions this issue: [release-branch.go1.14] net/http: handle body rewind in HTTP/2 connection loss better

@andybons andybons modified the milestones: Go1.14.5, Go1.14.6 Jul 14, 2020
@andybons
Copy link
Member

This change caused a number of issues when tested internally, and it seems that the fix to #39533 must also be backported if we want to avoid the same fate externally. This backport cannot be landed until the follow-up fix is cherry-picked.

As we are aiming to release the next point release today, I’m pushing this off for now. Apologies for the inconvenience.

@andybons andybons modified the milestones: Go1.14.6, Go1.14.7 Jul 16, 2020
@toothrot toothrot modified the milestones: Go1.14.7, Go1.14.8 Aug 6, 2020
@dcarney-stripe dcarney-stripe mentioned this issue Aug 12, 2020
Merged
@dmitshur dmitshur mentioned this issue Aug 22, 2020
Closed
@gopherbot gopherbot mentioned this issue Aug 22, 2020
Closed
@dmitshur
Copy link
Contributor

dmitshur commented Aug 22, 2020 •
edited
Loading

This change caused a number of issues when tested internally, and it seems that the fix to #39533 must also be backported if we want to avoid the same fate externally. This backport cannot be landed until the follow-up fix is cherry-picked.

Opened #40973 as a backport issue for that fix, and made the cherry-pick CL 249880.

@dmitshur dmitshur mentioned this issue Aug 24, 2020
Closed
@gopherbot gopherbot mentioned this issue Aug 24, 2020
Closed
@gopherbot
Copy link
Contributor Author

Closed by merging 0d8ee35 to release-branch.go1.14.

gopherbot pushed a commit that referenced this issue Aug 27, 2020
@rsc @dmitshur
[release-branch.go1.14] net/http: handle body rewind in HTTP/2 connec…
0d8ee35 
…tion loss better

In certain cases the HTTP/2 stack needs to resend a request.
It obtains a fresh body to send by calling req.GetBody.
This call was missing from the path where the HTTP/2
round tripper returns ErrSkipAltProtocol, meaning fall back
to HTTP/1.1. The result was that the HTTP/1.1 fallback
request was sent with no body at all.

This CL changes that code path to rewind the body before
falling back to HTTP/1.1. But rewinding the body is easier
said than done. Some requests have no GetBody function,
meaning the body can't be rewound. If we need to rewind and
can't, that's an error. But if we didn't read anything, we don't
need to rewind. So we have to track whether we read anything,
with a new ReadCloser wrapper. That in turn requires adding
to the couple places that unwrap Body values to look at the
underlying implementation.

This CL adds the new rewinding code in the main retry loop
as well.

The new rewindBody function also takes care of closing the
old body before abandoning it. That was missing in the old
rewind code.

Thanks to Aleksandr Razumov for CL 210123
and to Jun Chen for CL 234358, both of which informed
this CL.

Updates #32441.
Fixes #39279.

Change-Id: Id183758526c087c6b179ab73cf3b61ed23a2a46a
Reviewed-on: https://go-review.googlesource.com/c/go/+/234894
Run-TryBot: Russ Cox <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>
(cherry picked from commit e3491c4)
Reviewed-on: https://go-review.googlesource.com/c/go/+/242117
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Emmanuel Odeke <[email protected]>
@dmitshur dmitshur modified the milestones: Go1.14.8, Go1.14.9 Sep 1, 2020
@golang golang locked and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.14.9
Development

No branches or pull requests
4 participants
@toothrot @andybons @dmitshur @gopherbot