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

Jump to bottom

x/net/http2: TestTransportGroupsPendingDials failures due to missing Close #52996

Open
bcmills opened this issue May 19, 2022 · 5 comments
Open

x/net/http2: TestTransportGroupsPendingDials failures due to missing Close #52996

bcmills opened this issue May 19, 2022 · 5 comments
Assignees
@neild
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.23

Comments

@bcmills
Copy link
Contributor

bcmills commented May 19, 2022 

--- FAIL: TestTransportGroupsPendingDials (0.02s)
    transport_test.go:401: saw 0 closes; want 1
FAIL
FAIL	golang.org/x/net/http2	17.546s

greplogs -l -e 'FAIL: TestTransportGroupsPendingDials .*(?:\n[ ]{4}.*) saw 0 closes' --since=2021-01-01
2022-05-18T15:25:04-183a9ca-1f9f7db/linux-amd64-clang
2022-04-29T02:01:27-2871e0c-e7c56fe/freebsd-arm-paulzhol
2022-03-04T14:10:38-27dd868-c9b6063/freebsd-arm64-dmgk

This may be a symptom of #50027; see previously #43176.

(attn @neild @tombergan)
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 19, 2022
@bcmills bcmills added this to the Go1.19 milestone May 19, 2022
@bcmills
Copy link
Contributor Author

bcmills commented May 19, 2022

This failure mode has been observed on linux/amd64 (which is a first class port), so — especially since x/net/http2 is bundled into the standard library — this is a release-blocker for Go 1.19.

(If this isn't a priority to fix, a skip can be added to the test for this failure mode and then the issue can be moved to the backlog.)

@dmitshur dmitshur added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 1, 2022
@neild neild self-assigned this Jun 8, 2022
@gopherbot
Copy link

Change https://go.dev/cl/411294 mentions this issue: http2: enable VerboseLogs in TestTransportGroupsPendingDials

@neild
Copy link
Contributor

neild commented Jun 8, 2022

I'm not sure what's going on here.

This is not #50027: This test is expecting net.Conn.Close to have been called, which happens synchronously in CloseIdleConnections, not for the connection goroutines to have returned.

The only thing that makes sense is that a request has stayed live past Response.Body.Close being called (and is thus holding the connection live), which can happen in general, but I don't see how it happens in this test.

Flakes are super rare, and I haven't managed to reproduce it. Sent a CL to enable verbose HTTP/2 logging during this test; perhaps that'll point us at the problem the next time this flakes.

@neild
Copy link
Contributor

neild commented Jun 8, 2022

Removing the release-blocker label: If there's a real underlying issue, it does not need to block the release. CL 411294 will hopefully let us collect more information towards resolving the flake.

@neild neild removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Jun 8, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 11, 2022
gopherbot pushed a commit to golang/net that referenced this issue Jun 17, 2022
@neild
http2: enable VerboseLogs in TestTransportGroupsPendingDials
355a448 
This test is very, very rarely flaky. Enable additional logs to help
debug what's going on.

For golang/go#52996.

Change-Id: Ibccbfff94f51d2c813d48b48077537090bea4612
Reviewed-on: https://go-review.googlesource.com/c/net/+/411294
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@ianlancetaylor
Copy link
Contributor

Rolling forward to 1.20. Please comment if you disagree. Thanks.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @neild
http2: enable VerboseLogs in TestTransportGroupsPendingDials
29e20dc 
This test is very, very rarely flaky. Enable additional logs to help
debug what's going on.

For golang/go#52996.

Change-Id: Ibccbfff94f51d2c813d48b48077537090bea4612
Reviewed-on: https://go-review.googlesource.com/c/net/+/411294
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@neild neild

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Release Status & Release Blockers
Status: No status
Milestone
Go1.23
Development

No branches or pull requests
6 participants
@neild @dmitshur @ianlancetaylor @bcmills @gopherbot @seankhliao