net/http: TestRequestLimit/h2 becomes significantly more expensive and slower after x/[email protected] [1.21 backport] #66697
Labels
Milestone
Comments
gopherbot
added
the
CherryPickCandidate
|
Approved as this is a serious performance regression that was introduced by a fix. |
cagedmantis
added
the
CherryPickApproved
gopherbot
removed
the
CherryPickCandidate
|
Change https://go.dev/cl/578336 mentions this issue: |
|
Change https://go.dev/cl/578335 mentions this issue: |
gopherbot
pushed a commit
to golang/net
that referenced
this issue
Apr 12, 2024
…d flakes This test causes the server to send a GOAWAY and close a connection. The server GOAWAY path writes a GOAWAY frame asynchronously, and closes the connection if the write doesn't complete within 1s. This is causing failures on some builders, when the frame write doesn't complete in time. The important aspect of this test is that the connection be closed. Drop the check for the GOAWAY frame. This is a test-only fix that has no effect on the vendored content, helps tests on this branch, and avoids a merge conflict in next CL. For golang/go#66697. Change-Id: I099413be9c4dfe71d8fe83d2c6242e82e282293e Reviewed-on: https://go-review.googlesource.com/c/net/+/576235 Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Than McIntosh <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/net/+/578335 Reviewed-by: Damien Neil <[email protected]> Auto-Submit: Dmitri Shuralyov <[email protected]>
gopherbot
pushed a commit
to golang/net
that referenced
this issue
Apr 12, 2024
…tream-caused GOAWAY When closing a connection because a stream contained a request we didn't like (for example, because the request headers exceed the maximum we will accept), set the LastStreamID in the GOAWAY frame to include the offending stream. This informs the client that retrying the request is unlikely to succeed, and avoids retry loops. This change requires passing the stream ID of the offending stream from Framer.ReadFrame up to the caller. The most sensible way to do this would probably be in the error. However, ReadFrame currently returns a defined error type for connection-ending errors (ConnectionError), and that type is a uint32 with no place to put the stream ID. Rather than changing the returned errors, ReadFrame now returns an error along with a non-nil Frame containing the stream ID, when a stream is responsible for a connection-ending error. Merge conflicts were avoided by cherry-picking CL 576235 (test deflake) prior to this, and then by squashing CL 576175 (typo fix) into this CL. For golang/go#66668. For golang/go#66697. Change-Id: Iba07ccbd70ab4939aa56903605474d01703ac6e4 Reviewed-on: https://go-review.googlesource.com/c/net/+/576756 Reviewed-by: Jonathan Amsterdam <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Auto-Submit: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/net/+/578336 Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Auto-Submit: Dmitri Shuralyov <[email protected]>
|
Change https://go.dev/cl/578357 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Apr 12, 2024
Pull in CL 578336: ef58d90f http2: send correct LastStreamID in stream-caused GOAWAY For #66668. Fixes #66697. Change-Id: I91fc8a67f21fadcb1801ff29d5e2b0453db89617 Reviewed-on: https://go-review.googlesource.com/c/go/+/578357 Reviewed-by: Carlos Amedee <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
Closed by merging 891ac91 to release-branch.go1.21. |
This was referenced Jun 26, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
@neild requested issue #66668 to be considered for backport to the next 1.21 minor release.
The text was updated successfully, but these errors were encountered: