code review 87970045: net/http: fix data race in TestTransportResponseHeaderT...

net/http: fix data race in TestTransportResponseHeaderTimeout

Fixes Issue 7264

[bradfitz, 2014-04-16 02:13:31]

Hello dvyukov@google.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dvyukov, 2014-04-16 05:49:37]

Now I see how *this* fixes the race
looks good, please also fix TestIssue7264 in httptest the same way
if we agree that this is the fix, let TestIssue7264 be the demonstration of the
problem and the solution

[bradfitz, 2014-04-16 15:14:25]

I disagree that your TestIssue7264 in httptest is a real race (either in
theory or practice: it doesn't trigger on my machine), and I disagree that
this CL with the channel is a fix for a case that doesn't involve timeouts.

Your TestIssue7264 has a synchronous http.Get with no timeout.  I think it
is a perfectly valid race-free test:

                        ts := NewServer(http.HandlerFunc(func(w
http.ResponseWriter, r *http.Request) {}))
                        defer ts.Close()
                        tr := &http.Transport{
ResponseHeaderTimeout: time.Nanosecond,
}
                        defer tr.CloseIdleConnections()
c := &http.Client{Transport: tr}
res, err := c.Get(ts.URL)
if err == nil {
res.Body.Close()
}

I only caved and gave you this CL because of the timeout, which is a valid
point.

Why does build.golang.org's *-race builders not report this
TestTransportResponseHeaderTimeout
race, if my machine does?  Do you have some manually disabled on that
machine?



On Tue, Apr 15, 2014 at 10:49 PM, <dvyukov@google.com> wrote:

> Now I see how *this* fixes the race
> looks good, please also fix TestIssue7264 in httptest the same way
> if we agree that this is the fix, let TestIssue7264 be the demonstration
> of the problem and the solution
>
>
> https://codereview.appspot.com/87970045/
>

[dvyukov, 2014-04-16 16:01:59]

On Wed, Apr 16, 2014 at 7:14 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I disagree that your TestIssue7264 in httptest is a real race (either in
> theory or practice: it doesn't trigger on my machine

Have you tried running with -cpu=4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4?

> ), and I disagree that
> this CL with the channel is a fix for a case that doesn't involve timeouts.
>
> Your TestIssue7264 has a synchronous http.Get with no timeout.  I think it
> is a perfectly valid race-free test:
>
>                         ts := NewServer(http.HandlerFunc(func(w
> http.ResponseWriter, r *http.Request) {}))
>                         defer ts.Close()
>                         tr := &http.Transport{
> ResponseHeaderTimeout: time.Nanosecond,

What does ResponseHeaderTimeout: time.Nanosecond mean? Isn't it a timeout?

> }
>                         defer tr.CloseIdleConnections()
> c := &http.Client{Transport: tr}
> res, err := c.Get(ts.URL)
> if err == nil {
> res.Body.Close()
> }
>
> I only caved and gave you this CL because of the timeout, which is a valid
> point.
>
> Why does build.golang.org's *-race builders not report this
> TestTransportResponseHeaderTimeout race, if my machine does?  Do you have
> some manually disabled on that machine?

Probably the same reason why my reproduce does not reproduce the race
on your machine?
Race builders do not disable any tests.

>
>
>
> On Tue, Apr 15, 2014 at 10:49 PM, <dvyukov@google.com> wrote:
>>
>> Now I see how *this* fixes the race
>> looks good, please also fix TestIssue7264 in httptest the same way
>> if we agree that this is the fix, let TestIssue7264 be the demonstration
>> of the problem and the solution
>>
>>
>> https://codereview.appspot.com/87970045/
>
>

[bradfitz, 2014-04-16 17:01:56]

On Wed, Apr 16, 2014 at 9:01 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Apr 16, 2014 at 7:14 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > I disagree that your TestIssue7264 in httptest is a real race (either in
> > theory or practice: it doesn't trigger on my machine
>
> Have you tried running with -cpu=4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4?


There we go. I see one here on Linux, at least. Can try Mac later.


>  > ), and I disagree that

> this CL with the channel is a fix for a case that doesn't involve
> timeouts.
> >
> > Your TestIssue7264 has a synchronous http.Get with no timeout.  I think
> it
> > is a perfectly valid race-free test:
> >
> >                         ts := NewServer(http.HandlerFunc(func(w
> > http.ResponseWriter, r *http.Request) {}))
> >                         defer ts.Close()
> >                         tr := &http.Transport{
> > ResponseHeaderTimeout: time.Nanosecond,
>
> What does ResponseHeaderTimeout: time.Nanosecond mean? Isn't it a timeout?
>

Wow, I totally missed that, sorry.

I'm either going blind or this experiment of giving up coffee the past
week+ is an utter disaster. Maybe both.

Okay, this is the same case.

I thought somewhere else you were objecting to no-timeout http.Gets
in-between the test server starting and the ts.Close, but maybe that was
also me being stupid.

[bradfitz, 2014-04-16 17:10:22]

Hello dvyukov@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[dvyukov, 2014-04-16 17:12:40]

On Wed, Apr 16, 2014 at 9:01 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
>
> On Wed, Apr 16, 2014 at 9:01 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> On Wed, Apr 16, 2014 at 7:14 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > I disagree that your TestIssue7264 in httptest is a real race (either in
>> > theory or practice: it doesn't trigger on my machine
>>
>> Have you tried running with -cpu=4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4?
>
>
> There we go. I see one here on Linux, at least. Can try Mac later.
>
>
>>  > ), and I disagree that
>
> > this CL with the channel is a fix for a case that doesn't involve
>> timeouts.
>> >
>> > Your TestIssue7264 has a synchronous http.Get with no timeout.  I think
>> it
>> > is a perfectly valid race-free test:
>> >
>> >                         ts := NewServer(http.HandlerFunc(func(w
>> > http.ResponseWriter, r *http.Request) {}))
>> >                         defer ts.Close()
>> >                         tr := &http.Transport{
>> > ResponseHeaderTimeout: time.Nanosecond,
>>
>> What does ResponseHeaderTimeout: time.Nanosecond mean? Isn't it a timeout?
>>
>
> Wow, I totally missed that, sorry.
>
> I'm either going blind or this experiment of giving up coffee the past
> week+
>

hey, we need you alive!


>  is an utter disaster. Maybe both.
>
> Okay, this is the same case.
>
> I thought somewhere else you were objecting to no-timeout http.Gets
> in-between the test server starting and the ts.Close, but maybe that was
> also me being stupid.
>

I don't remember saying something about the case w/o timeout.

[bradfitz, 2014-04-16 17:30:19]

LG now?



On Wed, Apr 16, 2014 at 10:12 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

>
> On Wed, Apr 16, 2014 at 9:01 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>>
>>
>>
>> On Wed, Apr 16, 2014 at 9:01 AM, Dmitry Vyukov <dvyukov@google.com>wrote:
>>
>>> On Wed, Apr 16, 2014 at 7:14 PM, Brad Fitzpatrick <bradfitz@golang.org>
>>> wrote:
>>> > I disagree that your TestIssue7264 in httptest is a real race (either
>>> in
>>> > theory or practice: it doesn't trigger on my machine
>>>
>>> Have you tried running with -cpu=4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4?
>>
>>
>> There we go. I see one here on Linux, at least. Can try Mac later.
>>
>>
>>>  > ), and I disagree that
>>
>> > this CL with the channel is a fix for a case that doesn't involve
>>> timeouts.
>>> >
>>> > Your TestIssue7264 has a synchronous http.Get with no timeout.  I
>>> think it
>>> > is a perfectly valid race-free test:
>>> >
>>> >                         ts := NewServer(http.HandlerFunc(func(w
>>> > http.ResponseWriter, r *http.Request) {}))
>>> >                         defer ts.Close()
>>> >                         tr := &http.Transport{
>>> > ResponseHeaderTimeout: time.Nanosecond,
>>>
>>> What does ResponseHeaderTimeout: time.Nanosecond mean? Isn't it a
>>> timeout?
>>>
>>
>> Wow, I totally missed that, sorry.
>>
>> I'm either going blind or this experiment of giving up coffee the past
>> week+
>>
>
> hey, we need you alive!
>
>
>>  is an utter disaster. Maybe both.
>>
>> Okay, this is the same case.
>>
>> I thought somewhere else you were objecting to no-timeout http.Gets
>> in-between the test server starting and the ts.Close, but maybe that was
>> also me being stupid.
>>
>
> I don't remember saying something about the case w/o timeout.
>
>
>

[dvyukov, 2014-04-16 17:46:55]

LGTM

[bradfitz, 2014-04-16 18:32:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=95ef4aef66c2 ***

net/http: fix data race in TestTransportResponseHeaderTimeout

Fixes Issue 7264

LGTM=dvyukov
R=dvyukov
CC=golang-codereviews
https://codereview.appspot.com/87970045

[gobot, 2014-04-16 19:54:27]

This CL appears to have broken the openbsd-386-rootbsd builder.
See http://build.golang.org/log/f6782210df598d926a178477088f7aed24fc44db