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

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

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

Comments

Assignees

@tombergan tombergan

Labels
FrozenDueToAge Testing release-blocker
Projects
None yet
Milestone
  Go1.10
6 participants
@alexbrainman @ianlancetaylor @tombergan @mvdan @bradfitz @gopherbot
@alexbrainman
Copy link
Member

commented Oct 24, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Oct 25, 2017

Another instance: https://storage.googleapis.com/go-build-log/882d593d/freebsd-amd64-110_86b5d076.log

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Oct 25, 2017

@ianlancetaylor ianlancetaylor added release-blocker Testing labels Oct 25, 2017

@tombergan tombergan referenced this issue Oct 27, 2017

Closed

x/net/http2: connection reused with DisableKeepAlives #22441

@tombergan tombergan self-assigned this Oct 27, 2017

@tombergan

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 2, 2017

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.

This was referenced Nov 2, 2017

Open
Closed
@mvdan

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 10, 2017

Another: https://storage.googleapis.com/go-build-log/fd2d9f3d/linux-amd64-race_9bf28e55.log
@bradfitz

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 27, 2017

Another: https://build.golang.org/log/9a2ea5a6a70fc22f4f6561abb2d3229afca0a312
@gopherbot

This comment has been minimized.

Sign in to view
Copy link

commented Nov 28, 2017

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

@tombergan
http2: fix flake in net/http's TestCloseIdleConnections_h2 
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>
894f8ed
@gopherbot

This comment has been minimized.

Sign in to view
Copy link

commented Nov 30, 2017

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

@gopherbot gopherbot closed this in 7e394a2 Dec 1, 2017

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

@tombergan
http2: fix flake in net/http's TestCloseIdleConnections_h2 
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>
a7d5f1a

@golang golang locked and limited conversation to collaborators Dec 1, 2018

@gopherbot gopherbot added the FrozenDueToAge label Dec 1, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.