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

Open
bcmills opened this issue Dec 8, 2020 · 11 comments
Open

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 8, 2020

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
Copy link
Member Author

@bcmills bcmills commented Dec 8, 2020

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

@nhooyr
Copy link
Contributor

@nhooyr 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 nhooyr commented Dec 9, 2020

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

@nhooyr
Copy link
Contributor

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

@nhooyr nhooyr commented Dec 9, 2020

Opened #43086

nhooyr added a commit to nhooyr/go that referenced this issue Dec 9, 2020
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
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

@gopherbot gopherbot commented Dec 9, 2020

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

@toothrot
Copy link
Contributor

@toothrot toothrot commented Dec 10, 2020

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

@dmitshur dmitshur commented Dec 14, 2020

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.

@nhooyr
Copy link
Contributor

@nhooyr 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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.