net/http: Client / Transport deadlock

[gopherbot]

by yoshiyuki.kanno@stoic.co.jp:

What steps will reproduce the problem?
1. Invoke massive http requests by http.(Get|Head|...) to a same domain(for using pconn).

2. When switching goroutine between "http.(*persistConn).expectingResponse()"
and "http.(*persistConn).close()" in http.readLoop, scheduled goroutine
invoking http requests  will get into dead locked in
"http.(*persistConn).roundTrip". because  "responseAndError" will
never get data.

What is the expected output?
http.(*Transport).RoundTrip should return with an appropriate error.

What do you see instead?
To prevent getting into this dead locked, the critical region of
"http.(*persistConn).expectingResponse()" should include
"http.(*persistConn).close()".


Which compiler are you using (5g, 6g, 8g, gccgo)?
6g

Which operating system are you using?
Linux ubuntu11 3.0.0-12-server x86_64
Linux CentOS6 2.6.18-238.12.1.el5 x86_64

Which revision are you using?  (hg identify)
c1702f36df03 (release-branch.r60) release/release.r60.3
8e23f5bdc859 tip

Please provide any additional information below.
attached a quick patch for r60.3

Attachments:

1. transport.patch (3956 bytes)

[gopherbot]

Comment 1 by nekotaroh:

this issue can't be fixed completely without fixing issue #2645. because can not
detect`pc.cc.Write(req)`'s error in (*persistConn).roundTrip.
For details, please refer to issue #2645

[gopherbot]

Comment 2 by nekotaroh:

published reproducible codes on github.
https://github.com/mocchira/digestw
and the following code is where Getting into dead locked.
https://github.com/mocchira/digestw/blob/master/common/http.go
After applied patches issue #2616(this) and issue #2645,
No dead locked happen for now.

[bradfitz]

Comment 3:

We don't submit patches over the bug tracker.
To submit code changes, see:
http://golang.org/doc/contribute.html
Notably,
1) you need to submuit a CLA (http://golang.org/doc/contribute.html#copyright)
2) you need to use the codereview site.
Alternatively, I could work on fixing this independently, but I'd prefer to take your
change if you want to do the steps above.

Owner changed to @bradfitz.

Status changed to Accepted.

[gopherbot]

Comment 4 by nekotaroh:

submitted patches.
http://golang.org/cl/5532057/

[bradfitz]

Comment 5:

You haven't submitted a CLA that I can find.
See: http://golang.org/doc/contribute.html#copyright

[gopherbot]

Comment 6 by nekotaroh:

hmm.. I had done it through online, but might be migging something else.
So I'll do it again.
Thanks

[gopherbot]

Comment 7 by nekotaroh:

submitted two CLAs with yoshiyuki.kanno@stoic.co.jp and nekotaroh@gmail.com

[bradfitz]

Comment 8:

Issue #2645 has been merged into this issue.

[robpike]

Comment 10:

Labels changed: added priority-go1.

[robpike]

Comment 11:

Owner changed to builder@golang.org.

[bradfitz]

Comment 12:

Fix for this is in review. Waiting on one more round.

Owner changed to @bradfitz.

Status changed to Started.

[bradfitz]

Comment 14:

The latest version of the CL doesn't pass its new test on my machine:
$ go test -v net/http
--- FAIL: TestClientTransportDeadlock (30.00 seconds)
        client_test.go:486: probably dead locked at [7830 7883 7886 7890 7935 7936 7912 7902]
... also doesn't fail with a great error message.

[bradfitz]

Comment 15:

This issue was closed by revision d645adc.

Status changed to Fixed.