Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1082)

Issue 6851061: code review 6851061: net/http: fix Transport races & deadlocks (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by bradfitz
Modified:
11 years, 4 months ago
Reviewers:
remyoudompheng
CC:
dfc, bradfitz, Dustin, rsc, albert.strasheim, golang-dev
Visibility:
Public.

Description

net/http: fix Transport races & deadlocks Thanks to Dustin Sallings for exposing the most frustrating bug ever, and for providing repro cases (which formed the basis of the new tests in this CL), and to Dave Cheney and Dmitry Vyukov for help debugging and fixing. This CL depends on submited pollster CLs ffd1e075c260 (Unix) and 14b544194509 (Windows), as well as unsubmitted 6852085. Some operating systems (OpenBSD, NetBSD, ?) may still require more pollster work, fixing races (Issue 4434 and http://goo.gl/JXB6W). Tested on linux-amd64 and darwin-amd64, both with GOMAXPROCS 1 and 4 (all combinations of which previously failed differently) Fixes Issue 4191 Update Issue 4434 (related fallout from this bug)

Patch Set 1 #

Patch Set 2 : diff -r 13d81e48dc90 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 2524ef4a77b4 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r b96491ad23fd https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r b96491ad23fd https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r b96491ad23fd https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 7 : diff -r b96491ad23fd https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 8 : diff -r ffd1e075c260 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 9 : diff -r cda840e2befc https://go.googlecode.com/hg/ #

Total comments: 9

Patch Set 10 : diff -r 03a6b8c9c396 https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r 81b14ef2226a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+216 lines, -41 lines) Patch
M src/pkg/net/http/export_test.go View 1 2 3 1 chunk +8 lines, -1 line 0 comments Download
M src/pkg/net/http/server.go View 1 2 3 4 5 6 7 3 chunks +52 lines, -3 lines 0 comments Download
M src/pkg/net/http/transport.go View 1 2 3 4 5 6 7 8 4 chunks +51 lines, -37 lines 0 comments Download
M src/pkg/net/http/transport_test.go View 1 2 3 4 5 6 7 8 9 1 chunk +105 lines, -0 lines 0 comments Download

Messages

Total messages: 31
dfc
I'm not sure if I fixed the problem, or just fixed the test, but with ...
11 years, 4 months ago (2012-11-22 12:46:20 UTC) #1
bradfitz
You shouldn't have to modify the test. I believe the tests are all valid Go ...
11 years, 4 months ago (2012-11-22 13:21:16 UTC) #2
dfc
The documentation says you always need to close the response body, I think not doing ...
11 years, 4 months ago (2012-11-22 13:24:17 UTC) #3
bradfitz
The docs say that as a short way to say "get to the end". A ...
11 years, 4 months ago (2012-11-22 14:03:48 UTC) #4
bradfitz
Hello dave@cheney.net, bradfitz@golang.org, dsallings@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 4 months ago (2012-11-22 15:15:30 UTC) #5
bradfitz
Whoops, I thought this would only mail Dave & Dustin. But this more of the ...
11 years, 4 months ago (2012-11-22 15:19:13 UTC) #6
dfc
https://codereview.appspot.com/6851061/diff/6014/src/pkg/net/http/transport_test.go File src/pkg/net/http/transport_test.go (right): https://codereview.appspot.com/6851061/diff/6014/src/pkg/net/http/transport_test.go#newcode977 src/pkg/net/http/transport_test.go:977: } Hi Brad, Just to humor me, can you ...
11 years, 4 months ago (2012-11-22 22:36:13 UTC) #7
bradfitz
Hello dave@cheney.net, bradfitz@golang.org, dsallings@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-24 07:18:32 UTC) #8
dfc
https://codereview.appspot.com/6851061/diff/8005/src/pkg/net/http/transport_test.go File src/pkg/net/http/transport_test.go (right): https://codereview.appspot.com/6851061/diff/8005/src/pkg/net/http/transport_test.go#newcode971 src/pkg/net/http/transport_test.go:971: defer ts.Close() There is a deadlock here, ts.Close calls ...
11 years, 4 months ago (2012-11-24 14:27:50 UTC) #9
dfc
goroutine 16 [semacquire]: sync.runtime_Semacquire(0xc2000bb3f8, 0xc2000bb3f8) /home/dfc/go/src/pkg/runtime/zsema_linux_amd64.c:165 +0x2e sync.(*WaitGroup).Wait(0xc20010d0d0, 0x0) /home/dfc/go/src/pkg/sync/waitgroup.go:102 +0xf2 net/http/httptest.(*Server).Close(0xc20010d0a0, 0x6cc8a0) /home/dfc/go/src/pkg/net/http/httptest/server.go:157 +0x41 ...
11 years, 4 months ago (2012-11-24 14:29:24 UTC) #10
bradfitz
Hello dave@cheney.net, bradfitz@golang.org, dsallings@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-25 17:54:42 UTC) #11
bradfitz
This is now ready for review. It just can't be submitted until Dave's -1 fix ...
11 years, 4 months ago (2012-11-25 18:15:23 UTC) #12
dfc
=== RUN TestIssue4191_InfiniteGetTimeout-18 --- PASS: TestIssue4191_InfiniteGetTimeout-18 (0.50 seconds) === RUN TestIssue4191_InfiniteGetToPutTimeout-18 jams (also -15 on ...
11 years, 4 months ago (2012-11-26 00:08:30 UTC) #13
bradfitz
Hello dave@cheney.net, bradfitz@golang.org, dsallings@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-26 00:18:55 UTC) #14
bradfitz
PTAL I can't reproduce any hang with any number of GOMAXPROCS (1, 4, 20), but ...
11 years, 4 months ago (2012-11-26 00:19:42 UTC) #15
dfc
> Done, even though I do like defers. So do I, but in this case ...
11 years, 4 months ago (2012-11-26 00:25:28 UTC) #16
bradfitz
On Sun, Nov 25, 2012 at 4:25 PM, <dave@cheney.net> wrote: > Done, even though I ...
11 years, 4 months ago (2012-11-26 00:33:42 UTC) #17
dfc
LGTM. This is my pkg stress testing script % more ~/stress.bash #!/bin/bash set -e go ...
11 years, 4 months ago (2012-11-26 00:36:58 UTC) #18
dfc
NOT LGTM. Sorry, even simple tests in net/http are failing lucky(~/go/src) % go test net/http ...
11 years, 4 months ago (2012-11-26 00:41:23 UTC) #19
bradfitz
On Sun, Nov 25, 2012 at 4:41 PM, <dave@cheney.net> wrote: > NOT LGTM. > > ...
11 years, 4 months ago (2012-11-26 00:44:50 UTC) #20
dfc
LGTM. Very sorry, I had some leftover debugging in net/http/httptest. PASS ok net/http 8.499s On ...
11 years, 4 months ago (2012-11-26 00:46:33 UTC) #21
bradfitz
I'll wait on submitting this for a bit. I'm still seeing the same GetToPut hang ...
11 years, 4 months ago (2012-11-26 01:15:41 UTC) #22
rsc
LGTM as long as dfc is happy
11 years, 4 months ago (2012-11-26 20:32:18 UTC) #23
albert.strasheim
Hello On Monday, November 26, 2012 2:33:44 AM UTC+2, Brad Fitzpatrick wrote: > > I ...
11 years, 4 months ago (2012-11-26 21:09:32 UTC) #24
bradfitz
Okay, I'll submit this now, since Albert is waiting on reporting high GOMAXPROCS errors, dfc ...
11 years, 4 months ago (2012-11-26 21:24:04 UTC) #25
dfc
Yup, worse case the -test.timeout=120 will recover the builder. On Tue, Nov 27, 2012 at ...
11 years, 4 months ago (2012-11-26 21:27:48 UTC) #26
bradfitz
*** Submitted as http://code.google.com/p/go/source/detail?r=be4b8f195c96 *** net/http: fix Transport races & deadlocks Thanks to Dustin Sallings ...
11 years, 4 months ago (2012-11-26 21:31:13 UTC) #27
remyoudompheng
I got the following failure during all.bash on linux/arm but could not reproduce by running ...
11 years, 4 months ago (2012-11-26 22:34:58 UTC) #28
bradfitz
What value of GOMAXPROCS? goroutine 1102 in the stacks below is the sign of the ...
11 years, 4 months ago (2012-11-26 22:44:21 UTC) #29
remyoudompheng
On 2012/11/26 Brad Fitzpatrick <bradfitz@golang.org> wrote: > What value of GOMAXPROCS? goroutine 1102 in the ...
11 years, 4 months ago (2012-11-26 22:47:00 UTC) #30
bradfitz
11 years, 4 months ago (2012-11-26 22:50:16 UTC) #31
On Mon, Nov 26, 2012 at 2:47 PM, Rémy Oudompheng
<remyoudompheng@gmail.com>wrote:

> On 2012/11/26 Brad Fitzpatrick <bradfitz@golang.org> wrote:
> > What value of GOMAXPROCS? goroutine 1102 in the stacks below is the sign
> of
> > the expected failure for GOMAXPROCS > 1 until Issue 4434 is fixed.
>
> I think it was GOMAXPROCS=1 since it was under all.bash and I don't
> have particular environment variables.
>

It's likely the bug still exists with GOMAXPROCS=1 but is just hard to
trigger on relatively fast machines, but your linux/arm triggers it more
easily.

FWIW, I couldn't reproduce that hang running the http tests in a loop on my
linux-amd64 laptop with GOMAXPROCS=1, but it would happen after awhile on
with GOMAXPROCS >1.

I can disable the test if it's not fixed in the next day or so and is
getting annoying.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b