Skip to content

/ go

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
NeedsFix release-blocker
Milestone
Go1.16

Comments

@bcmills
Copy link
Member

@bcmills 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 this to the Go1.16 milestone Dec 2, 2020
@bcmills
Copy link
Member Author

@bcmills 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 fraenkel commented Dec 2, 2020

This fix is incorrect and should be reverted.
@fraenkel
Copy link
Contributor

@fraenkel 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 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 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

@gopherbot gopherbot commented Dec 3, 2020

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 and removed NeedsInvestigation labels Dec 9, 2020
@gopherbot gopherbot closed this in 854a2f8 Dec 9, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2021

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

@networkimprov networkimprov commented Mar 2, 2021

@fraenkel will this get backport for #41600?
@dmitshur
Copy link
Member

@dmitshur 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
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix release-blocker
Projects
None yet
Milestone
Go1.16
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
@networkimprov @fraenkel @dmitshur @bcmills @gopherbot