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

net/http: TestCancelRequestWhenSharingConnection failures on longtest builders #42942

Closed
bcmills opened this issue Dec 2, 2020 · 9 comments
Closed

net/http: TestCancelRequestWhenSharingConnection failures on longtest builders #42942

bcmills opened this issue Dec 2, 2020 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.16

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 2, 2020 •
edited

2020-12-02T14:20:12-0433845/linux-amd64-longtest
2020-12-01T21:42:10-7fca39a/windows-amd64-longtest
2020-12-01T20:42:53-20e2518/linux-amd64-longtest
2020-12-01T19:47:12-50b16f9/linux-amd64-longtest

Marking as release-blocker because this test was introduced Dec. 1 (in CL 257818).

CC @fraenkel @neild @bradfitz @golang/release
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Dec 2, 2020
@bcmills bcmills added this to the Go1.16 milestone Dec 2, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Dec 2, 2020

There is also what appears to be a deadlock in this test:
2020-12-01T21:41:39-7430266/windows-amd64-longtest

@bcmills bcmills mentioned this issue Dec 2, 2020
Closed
@fraenkel
Copy link
Contributor

fraenkel commented Dec 2, 2020

This fix is incorrect and should be reverted.

@fraenkel
Copy link
Contributor

fraenkel commented Dec 2, 2020

Sorry, the fix is mostly correct. Confusing myself on the issue.
I need to see why the test fails sometimes which still means there is more to this.

@fraenkel
Copy link
Contributor

fraenkel commented Dec 2, 2020

CL 257818 did fix the case where:
Req 1 does a GET, the readLoop puts the persistconn back
Req 2 gets the pc, and has its context closed which drives cancelRequest -> pc.closech prior to Req 1 leaving roundtrip.

The new issue is similar but makes me believe there is a bigger issue afoot.
The first request starts to cancel (roundtrip), but puts the pc back into the pool (readLoop).
A second request picks up the pc.
The cancel completes, and the second request is cancelled.

@fraenkel
Copy link
Contributor

fraenkel commented Dec 2, 2020

There are multiple goroutines that fiddle with the cancelFn but most are blindly setting it rather than looking to see if its already been "cancelled".

@fraenkel fraenkel mentioned this issue Dec 3, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/274973 mentions this issue: net/http: add connections back that haven't been cancelled

@bcmills bcmills mentioned this issue Dec 8, 2020
Closed
@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 Dec 9, 2020
@gopherbot
Copy link

Change https://golang.org/cl/297910 mentions this issue: [release-branch.go1.15] net/http: add connections back that haven't been canceled

@networkimprov
Copy link

@fraenkel will this get backport for #41600?

@dmitshur
Copy link
Contributor

dmitshur commented Mar 2, 2021

@networkimprov Yes, see #42935 (comment).

gopherbot pushed a commit that referenced this issue Mar 2, 2021
@fraenkel @dmitshur
[release-branch.go1.15] net/http: add connections back that haven't b…
1c60e0d 
…een canceled

Issue #41600 fixed the issue when a second request canceled a connection
while the first request was still in roundTrip.
This uncovered a second issue where a request was being canceled (in
roundtrip) but the connection was put back into the idle pool for a
subsequent request.
The fix is the similar except its now in readLoop instead of roundTrip.
A persistent connection is only added back if it successfully removed
the cancel function; otherwise we know the roundTrip has started
cancelRequest.

Fixes #42935.
Updates #42942.

Change-Id: Ia56add20880ccd0c1ab812d380d8628e45f6f44c
Reviewed-on: https://go-review.googlesource.com/c/go/+/274973
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 854a2f8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/297910
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@cagedmantis cagedmantis mentioned this issue Apr 2, 2021
Closed
@golang golang locked and limited conversation to collaborators Mar 2, 2022
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. release-blocker
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
5 participants
@networkimprov @fraenkel @dmitshur @bcmills @gopherbot