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: apparent deadlocks in TestDisableKeepAliveUpgrade on longtest builders #43073

Closed
bcmills opened this issue Dec 8, 2020 · 12 comments
Closed

net/http: apparent deadlocks in TestDisableKeepAliveUpgrade on longtest builders #43073

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

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 8, 2020 •
edited

2020-12-07T21:01:46-7ad6596/windows-amd64-longtest
2020-12-04T08:49:16-b67b7dd/windows-amd64-longtest
2020-12-02T16:43:58-10240b9/windows-amd64-longtest

Marking as release-blocker for Go 1.16: this is a new test, merged Dec. 1 in CL 213277 for #36381.

CC @bradfitz @nhooyr @neild @fraenkel @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 8, 2020
@bcmills bcmills added this to the Go1.16 milestone Dec 8, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Dec 8, 2020

This regression was partially masked in the builders by #42942.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 9, 2020

Hmm, interesting that it only occurs on Windows.

Looks like the io.ReadFull on line 6495 is stuck as the peer isn't echoing back the written bytes for some reason.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 9, 2020

Nothing stands out, the test may be surfacing an unrelated regression.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 9, 2020

Actually I'm going to push up a change that may fix it.

nhooyr added a commit to nhooyr/go that referenced this issue Dec 9, 2020
@nhooyr
net/http: attempt to fix deadlock in TestDisableKeepAliveUpgrade
a6fd3e1 
1. The test now checks the response status code.
2. The transport does not set `Connection: Close` if DisableKeepAlive is
   set but the request is a HTTP/1.1 protocol upgrade request.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 9, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
acbdd76 
1. The test now checks the response status code.
2. The transport does not set `Connection: Close` if DisableKeepAlive is
   set but the request is a HTTP/1.1 protocol upgrade request.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 9, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
a046d63 
1. The test now checks the response status code.
2. The transport does not set `Connection: Close` if DisableKeepAlive is
   set but the request is a HTTP/1.1 protocol upgrade request.

Updates golang#43073
@nhooyr nhooyr mentioned this issue Dec 9, 2020
Closed
@nhooyr
Copy link
Contributor

nhooyr commented Dec 9, 2020

Opened #43086

nhooyr added a commit to nhooyr/go that referenced this issue Dec 9, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
e31f859 
1. The test now checks the response status code.
2. The transport has been changed to not set `Connection: Close` if
  DisableKeepAlive is set but the request is a HTTP/1.1 protocol upgrade.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 9, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
fd783ef 
1. The test now checks the response status code.
2. The transport has been changed to not set `Connection: Close` if
    DisableKeepAlive is set and the request is a HTTP/1.1 protocol upgrade.

Updates golang#43073
@gopherbot
Copy link

Change https://golang.org/cl/276375 mentions this issue: net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade

@toothrot
Copy link
Contributor

@bradfitz @neild This is currently blocking beta1. Could you please take a look at the CL when you get a chance?

nhooyr added a commit to nhooyr/go that referenced this issue Dec 10, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
d32c154 
1. The test now checks the response status code.
2. The transport has been changed to not set `Connection: Close` if
    DisableKeepAlive is set and the request is a HTTP/1.1 protocol upgrade.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 10, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
4c05ef3 
1. The test now checks the response status code.
2. The transport has been changed to not set `Connection: Close` if
   DisableKeepAlive is set and the request is a HTTP/1.1 protocol
   upgrade.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 11, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
e58bd9a 
1. The test now checks the response status code.
2. The transport has been changed to not set "Connection: Close" if
   DisableKeepAlive is set and the request is a HTTP/1.1 protocol
   upgrade.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 11, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
fa61250 
1. The test now checks the response status code.
2. The transport has been changed to not set "Connection: Close" if
   DisableKeepAlive is set and the request is a HTTP/1.1 protocol
   upgrade.

Updates golang#43073
nhooyr added a commit to nhooyr/go that referenced this issue Dec 11, 2020
@nhooyr
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
f809ceb 
1. The test now checks the response status code.
2. The transport has been changed to not set "Connection: Close" if
   DisableKeepAlive is set and the request is a HTTP/1.1 protocol
   upgrade.

Updates golang#43073
gopherbot pushed a commit that referenced this issue Dec 14, 2020
@nhooyr @neild
net/http: attempt deadlock fix in TestDisableKeepAliveUpgrade
c81343c 
1. The test now checks the response status code.
2. The transport has been changed to not set "Connection: Close" if
   DisableKeepAlive is set and the request is a HTTP/1.1 protocol
   upgrade.

Updates #43073

Change-Id: I9977a18b33b8747ef847a8d11bb7b4f2d8053b8c
GitHub-Last-Rev: f809ceb
GitHub-Pull-Request: #43086
Reviewed-on: https://go-review.googlesource.com/c/go/+/276375
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor

dmitshur commented Dec 14, 2020 •
edited

Thanks for looking into this @nhooyr.

There haven't been instances of this failure since CL 276375 was submitted, although given it happened infrequently earlier, that's not a definitive signal.

Do you know if anything more needs to be done for this issue, or is it a matter of waiting for some more data before closing?

I'll add okay-after-beta1 since I'm not seeing something here that needs to block beta 1. Please comment if I missed something.

@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 Dec 14, 2020
@nhooyr
Copy link
Contributor

nhooyr commented Dec 15, 2020

Do you know if anything more needs to be done for this issue, or is it a matter of waiting for some more data before closing?

I didn't notice anything else that could cause it so yes I'd agree that it is a matter of waiting for some more data.

@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
@bcmills
Copy link
Contributor Author

bcmills commented Jan 7, 2021

Still a problem after CL 276375:

2021-01-04T12:06:08-b01fb2a/linux-386-longtest
2020-12-31T02:24:55-95ce805/linux-amd64-longtest
2020-12-26T19:08:56-1d78139/windows-amd64-longtest
2020-12-23T14:16:32-98a7303/windows-amd64-longtest
2020-12-17T16:03:19-2ff33f5/windows-amd64-longtest
2020-12-14T20:56:03-dea6d94/linux-amd64-longtest
2020-12-14T20:30:16-2f5b1a3/linux-386-longtest
2020-12-14T19:19:09-c81343c/linux-386-longtest

@bcmills bcmills changed the title net/http: apparent deadlocks in TestDisableKeepAliveUpgrade on windows-amd64-longtest net/http: apparent deadlocks in TestDisableKeepAliveUpgrade on longtest builders Jan 7, 2021
@nhooyr
Copy link
Contributor

nhooyr commented Jan 7, 2021

Damn, that's unfortunate. Hope I have some time this weekend to investigate but if not, I think we should just revert my changes & the test and I'll try and reproduce myself first.

@gopherbot
Copy link

Change https://golang.org/cl/285596 mentions this issue: net/http: fix flaky TestDisableKeepAliveUpgrade

@golang golang locked and limited conversation to collaborators Jan 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Milestone
Go1.16
Development

No branches or pull requests
5 participants
@toothrot @dmitshur @bcmills @gopherbot @nhooyr