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

net/http: deadlock after leaked transport detected in TestConsumingBodyOnNextConn #42943

Closed
bcmills opened this issue Dec 2, 2020 · 13 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 2, 2020

2020-12-01T21:42:10-7fca39a/linux-386-longtest

This is the only failure in the logs matching the regexp (?ms)Test appears to have leaked a Transport.*test timed out. It may be a secondary failure mode associated with #42942, or a different regression from CL 213277, or a latent bug that just happened to turn up now.

CC @neild @fraenkel @bradfitz @nhooyr

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 2, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Dec 2, 2020

Marking as release-blocker for 1.16 at least until we understand whether this is a regression. (CC @golang/release)

@bcmills bcmills changed the title net/http net/http: deadlock after leaked transport detected in TestConsumingBodyOnNextConn Dec 2, 2020
@bcmills bcmills added this to the Go1.16 milestone Dec 2, 2020
@nhooyr
Copy link
Contributor

nhooyr commented Dec 2, 2020

I believe this issue is unrelated to the recent change I submitted. My change only modifies how http.Server behaves when keep alives are disabled but they are enabled in the problematic test.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 2, 2020

As far as I can tell, the affected test also doesn't use http.Transport anywhere. It's a leak from a different test.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 2, 2020

Ah I see, the test I submitted isn't closing rwc. Pushing a fix.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 2, 2020

Actually nvm, the body is being closed above before being asserted into a rwc.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 2, 2020

As far as I can tell, the affected test also doesn't use http.Transport anywhere. It's a leak from a different test.

So in order for the afterTest leak test to run, t.Short() must be false which also means the tests aren't running in parallel, so I guess I'm wrong, the test must be using http.Transport somewhere. Or there's a test that doesn't have the preceding setParallel call.

@fraenkel
Copy link
Contributor

fraenkel commented Dec 3, 2020

Most likely related to #42942 given the goroutine stuck on line 2605.

@toothrot
Copy link
Contributor

/cc @neild

@toothrot toothrot 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 Dec 10, 2020
@toothrot
Copy link
Contributor

Labeling as okay-after-beta1, but still considering this a release blocking issue. Please comment if you disagree.

@toothrot toothrot removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Dec 17, 2020
@dmitshur
Copy link
Contributor

We discussed this in a release meeting and @neild is planning to look. The goal (from #42943 (comment)) is to understand whether this is a regression. Please update this issue as needed.

@fraenkel
Copy link
Contributor

This isn't a regression per say but introduced by a fix, #41600.

@neild
Copy link
Contributor

neild commented Jan 22, 2021

I am unable to reproduce this, and believe it has been fixed by https://golang.org/cl/274973 (the fix for #42942).

@dmitshur
Copy link
Contributor

I understand this issue resolved (i.e., the unusual failure was investigated, and the investigation did not uncover any new reproducible problems to fix), closing.

@golang golang locked and limited conversation to collaborators Jan 25, 2022
@rsc rsc unassigned neild Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants