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

x/net/http2: TestUnreadFlowControlReturned_Server failures due to INTERNAL_ERROR since 2022-03-15 #52051

Closed
bcmills opened this issue Mar 30, 2022 · 6 comments
Closed

x/net/http2: TestUnreadFlowControlReturned_Server failures due to INTERNAL_ERROR since 2022-03-15 #52051

bcmills opened this issue Mar 30, 2022 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.19

Comments

@bcmills
Copy link
Contributor

bcmills commented Mar 30, 2022 

--- FAIL: TestUnreadFlowControlReturned_Server (16.39s)
    --- FAIL: TestUnreadFlowControlReturned_Server/body-closed (14.15s)
        server_test.go:3844: body-closed timedout
        server_test.go:3868: body-closed stream error: stream ID 5; INTERNAL_ERROR; received from peer
FAIL
FAIL	golang.org/x/net/http2	184.809s

greplogs --dashboard -md -l -e 'FAIL: TestUnreadFlowControlReturned_.*' --since=2022-01-01

2022-03-30T02:16:17-de3da57-d1060d8/plan9-386-0intro
2022-03-25T01:24:44-27dd868-8ab42a9/plan9-386-0intro
2022-03-18T19:32:40-27dd868-a682a5c/plan9-386-0intro
2022-03-18T16:57:07-27dd868-0a49f70/plan9-386-0intro
2022-03-15T01:00:36-27dd868-44a0da4/plan9-386-0intro

(attn @0intro; CC @neild)

See previously #49645.
@bcmills bcmills added OS-Plan9 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-386 Issues solely affecting the 32-bit x86 architecture labels Mar 30, 2022
@bcmills bcmills added this to the Backlog milestone Mar 30, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Jun 3, 2022

This has also happened on a netbsd/arm64 builder recently:

https://build.golang.org/log/ebd81abf53025263ed94ef3462193b7efc319315

This is the only open bug for this problem I'm finding. @bcmills Do you think it's better to expand the scope of this one to be non-Plan9 specific or file a separate tracking one for netbsd/arm64?

@bcmills
Copy link
Contributor Author

bcmills commented Jun 3, 2022 •
edited
Loading

The output in the test log looks exactly the same to me, so I'd be inclined to expand the scope. The fact that there are no GOOS or GOARCH elements in common suggests that this is a platform-independent bug (quite possibly timing related).

@dmitshur dmitshur removed OS-Plan9 arch-386 Issues solely affecting the 32-bit x86 architecture labels Jun 3, 2022
@dmitshur dmitshur changed the title x/net/http2: TestUnreadFlowControlReturned_Server failures due to INTERNAL_ERROR on plan9-386-0intro since 2022-03-15 x/net/http2: TestUnreadFlowControlReturned_Server failures due to INTERNAL_ERROR since 2022-03-15 Jun 3, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Jun 3, 2022

Yep, here's the problem: another arbitrary timeout. Should be a trivial fix, so I'll go ahead and mail it.

https://cs.opensource.google/go/x/net/+/master:http2/server_test.go;l=3828-3829;drc=c960675eff93b7ce235fa4b1578d83fa48ea129c

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 3, 2022
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Jun 3, 2022
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure. and removed Testing An issue that has been verified to require only test changes, not just a test failure. labels Jun 3, 2022
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Jun 3, 2022
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 3, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 3, 2022
@bcmills bcmills self-assigned this Jun 3, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410096 mentions this issue: http2: remove arbitrary timeouts in server_test.go

@bcmills
Copy link
Contributor Author

bcmills commented Jun 3, 2022

Moving to the Go 1.19 milestone since x/net/http2 is bundled into the main repo — especially given that it's a test-only fix and has a pending CL.

@bcmills bcmills modified the milestones: Unreleased, Go1.19 Jun 3, 2022
@bcmills bcmills 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 3, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Jun 3, 2022 •
edited
Loading

x/net/http2 is bundled into the main repo [...] it's a test-only fix

I think Go1.19 milestone is fine, just note that only the http2 package code is bundled into the standard library and not its tests, so nothing in the main repo will change when the next dependency update is performed.

dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
@gopherbot
http2: remove arbitrary timeouts in server_test.go
aa149f2 
If the test gets completely stuck at these points, we will want a
goroutine dump in order to debug the hang, which log.Fatalf will
not produce (but a test timeout will).

If the test does not get completely stuck (as on a slow or overloaded
builder), then we should let it continue to run until the overall test
timeout, which (unlike hard-coded constants) should already take the
speed of the builder into account.

As a side-effect, this also moves some t.Fatalf calls out of
background goroutines and into the main test-function goroutines where
they belong (see golang/go#24678).

Fixes golang/go#52051.

Change-Id: I37504081e6fdf0b4c244305fc83c575e30b7b453
Reviewed-on: https://go-review.googlesource.com/c/net/+/410096
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
@rsc rsc unassigned bcmills Jun 22, 2022
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
http2: remove arbitrary timeouts in server_test.go
e2d866b 
If the test gets completely stuck at these points, we will want a
goroutine dump in order to debug the hang, which log.Fatalf will
not produce (but a test timeout will).

If the test does not get completely stuck (as on a slow or overloaded
builder), then we should let it continue to run until the overall test
timeout, which (unlike hard-coded constants) should already take the
speed of the builder into account.

As a side-effect, this also moves some t.Fatalf calls out of
background goroutines and into the main test-function goroutines where
they belong (see golang/go#24678).

Fixes golang/go#52051.

Change-Id: I37504081e6fdf0b4c244305fc83c575e30b7b453
Reviewed-on: https://go-review.googlesource.com/c/net/+/410096
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot