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: HTTP/2 connections incorrectly marked as "reused" #60636

Closed
neild opened this issue Jun 6, 2023 · 4 comments
Closed

net/http: HTTP/2 connections incorrectly marked as "reused" #60636

neild opened this issue Jun 6, 2023 · 4 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@neild
Copy link
Contributor

neild commented Jun 6, 2023

The Transport retry loop checks to see if a request was sent on a fresh connection or a reused one (persistConn.shouldRetryRequest). Requests sent on a fresh connection are not retried, to avoid looping forever if the server is hanging up on the request for some reason.

Connections are marked as reused when added to the idle conn pool (Transport.tryPutIdleConn).

HTTP/2 connections are added to the idle conn pool as soon as they're made, since the connection can be reused for additional HTTP/2 requests while the first is in flight.

Thus, we incorrectly retry requests to an HTTP/2 server that hangs up without giving a response (say, because we sent a malformed request), because HTTP/2 connections are always marked as "reused".
@neild
Copy link
Contributor Author

neild commented Jun 6, 2023

...also, the HTTP/2 side has its own competing retry loop which doesn't apply the "don't retry if the first request on a connection fails" heuristic, so fixing this only on the net/http side is insufficient.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 6, 2023
@neild neild mentioned this issue Apr 5, 2024
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/576895 mentions this issue: http2: don't retry the first request on a connection on GOAWAY error

@dmitshur dmitshur added this to the Go1.23 milestone Apr 5, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 5, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/576976 mentions this issue: all: update vendored golang.org/x/net

gopherbot pushed a commit that referenced this issue Apr 8, 2024
@dmitshur @gopherbot
all: update vendored golang.org/x/net
8008998 
Pull in CL 576895:

	ec05fdcd http2: don't retry the first request on a connection on GOAWAY error

For #66668.
Fixes #60636.

Change-Id: I9903607e3d432a5db0325da82eb7f4b378fbddde
Reviewed-on: https://go-review.googlesource.com/c/go/+/576976
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@jianfengtony
Copy link

As i known, this issue will affect go1.22 and go1.21. why not backport this to these old version? thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
5 participants
@neild @dmitshur @dr2chase @gopherbot @jianfengtony