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: TestCloseIdleConnections_h2 didn't close connection #22413

Closed
alexbrainman opened this issue Oct 24, 2017 · 6 comments
Closed

net/http: TestCloseIdleConnections_h2 didn't close connection #22413

alexbrainman opened this issue Oct 24, 2017 · 6 comments
Labels
FrozenDueToAge release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@alexbrainman
Copy link
Member

Seen on linux-arm trybot that Ian run against CL 72592

https://storage.googleapis.com/go-build-log/af50a7e4/linux-arm_10535f18.log

2017/10/23 18:13:51 Dialing 127.0.0.1:33539
2017/10/23 18:13:51 Dialing 127.0.0.1:43143
2017/10/23 18:13:52 Unsolicited response received on idle HTTP channel starting with "0\r\n\r\n"; err=<nil>
--- FAIL: TestCloseIdleConnections_h2 (0.08s)
	clientserver_test.go:1304: didn't close connection
FAIL
FAIL	net/http	11.269s

Alex

CC @tombergan if you are interested

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Oct 25, 2017
@ianlancetaylor ianlancetaylor added release-blocker Testing An issue that has been verified to require only test changes, not just a test failure. labels Oct 25, 2017
@tombergan tombergan self-assigned this Oct 27, 2017
@tombergan
Copy link
Contributor

The test flakes every ~2 of 100000 runs. This started happening after https://golang.org/cl/70510 (or more correctly, https://golang.org/cl/71611). Before that CL, we did:

  1. Get HEADERS with END_STREAM
  2. Remove stream from ClientConn
  3. Generate a Response with Body = noBody

After that CL, we do:

  1. Get HEADERS with END_STREAM
  2. Generate a Response with Body = noBody
  3. Remove stream from ClientConn

TestCloseIdleConnections_h2 flakes when we call CloseIdleConnections between steps 2 and 3. Ideally, we should not remove a stream from the ClientConn until the user calls Response.Body.Close.

@mvdan
Copy link
Member

mvdan commented Nov 10, 2017

@bradfitz
Copy link
Contributor

@gopherbot
Copy link

Change https://golang.org/cl/80139 mentions this issue: http2: fix flake in net/http's TestCloseIdleConnections_h2

gopherbot pushed a commit to golang/net that referenced this issue Nov 28, 2017
That test makes a request with no body and receives a response with no
body. The client will receive a HEADERS frame with END_STREAM. The test
assumes that the stream is closed immediately on receipt of that HEADERS
frame, i.e., before RoundTrip returns.

This assumption was broken by https://golang.org/cl/70510, which made
stream closure asynchronous w.r.t. RoundTrip.

To fix TestCloseIdleConnections_h2 while preserving the intent of CL
70510, we break processHeaders into two cases:

1. The request has a body. In this case, END_STREAM puts the stream in a
   half-closed-remote state, which means the connection is not
   necessarily idle when RoundTrip returns (since the request body is
   still being uploaded). In this case, we preserve the behavior from CL
   70510.

2. The request does not have a body. In this case, END_STREAM puts the
   stream in a closed state and we must close the stream before
   returning from RoundTrip.

The following command passes when this CL is merged into net/http:
go test -count=100000 -run=TestCloseIdleConnections_h2 net/http

Updates golang/go#22413

Change-Id: Iff2a0685a636ad51bff380e86a42b0d0eea984e5
Reviewed-on: https://go-review.googlesource.com/80139
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/81276 mentions this issue: net/http: update bundled http2

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
That test makes a request with no body and receives a response with no
body. The client will receive a HEADERS frame with END_STREAM. The test
assumes that the stream is closed immediately on receipt of that HEADERS
frame, i.e., before RoundTrip returns.

This assumption was broken by https://golang.org/cl/70510, which made
stream closure asynchronous w.r.t. RoundTrip.

To fix TestCloseIdleConnections_h2 while preserving the intent of CL
70510, we break processHeaders into two cases:

1. The request has a body. In this case, END_STREAM puts the stream in a
   half-closed-remote state, which means the connection is not
   necessarily idle when RoundTrip returns (since the request body is
   still being uploaded). In this case, we preserve the behavior from CL
   70510.

2. The request does not have a body. In this case, END_STREAM puts the
   stream in a closed state and we must close the stream before
   returning from RoundTrip.

The following command passes when this CL is merged into net/http:
go test -count=100000 -run=TestCloseIdleConnections_h2 net/http

Updates golang/go#22413

Change-Id: Iff2a0685a636ad51bff380e86a42b0d0eea984e5
Reviewed-on: https://go-review.googlesource.com/80139
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

6 participants