code review 5543062: net/http: fix data race in test

net/http: fix data race in test
Fixes issue 2712.

[dvyukov, 2012-01-16 09:02:28]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
mpimenov@google.com),

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

[dsymonds, 2012-01-16 10:34:36]

LGTM

[dvyukov, 2012-01-16 10:47:58]

*** Submitted as http://code.google.com/p/go/source/detail?r=221d98ac4a51 ***

net/http: fix data race in test
Fixes issue 2712.

R=golang-dev, dsymonds
CC=golang-dev, mpimenov
http://codereview.appspot.com/5543062

[bradfitz, 2012-01-16 17:11:46]

This "fix" is kinda awkward.

In all the other tests where the test HTTP server is supposed to vary its
behavior for a specific request, the parameter is put on the HTTP request.
 In this case we need to do two things to make a request: send the request,
and also write to that channel.  I'd rather just say /?override=1.

I say "fix" because I can't see how this would happen in reality:  between
setting override from false to true, I believe there are a few dozen memory
barriers inside of making an HTTP request.

Unlike some of the other fixes, this is just a theoretical clean-up, I
assume?


On Mon, Jan 16, 2012 at 1:02 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
> mpimenov@google.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: fix data race in test
> Fixes issue 2712.
>
> Please review this at
http://codereview.appspot.com/**5543062/<http://codereview.appspot.com/5543062/>
>
> Affected files:
>  M src/pkg/net/http/fs_test.go
>
>
> Index: src/pkg/net/http/fs_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/fs_test.go
> +++ b/src/pkg/net/http/fs_test.go
> @@ -224,9 +224,9 @@
>
>  func TestServeFileContentType(t *testing.T) {
>        const ctype = "icecream/chocolate"
> -       override := false
> +       override := make(chan bool, 1)
>        ts := httptest.NewServer(**HandlerFunc(func(w ResponseWriter, r
> *Request) {
> -               if override {
> +               if <-override {
>                        w.Header().Set("Content-Type", ctype)
>                }
>                ServeFile(w, r, "testdata/file")
> @@ -241,8 +241,9 @@
>                        t.Errorf("Content-Type mismatch: got %q, want %q",
> h, want)
>                }
>        }
> +       override <- false
>        get("text/plain; charset=utf-8")
> -       override = true
> +       override <- true
>        get(ctype)
>  }
>
>
>
>

[dvyukov, 2012-01-17 07:57:38]

On Mon, Jan 16, 2012 at 9:11 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> This "fix" is kinda awkward.
>
> In all the other tests where the test HTTP server is supposed to vary its
> behavior for a specific request, the parameter is put on the HTTP request.
>  In this case we need to do two things to make a request: send the request,
> and also write to that channel.  I'd rather just say /?override=1.
>
> I say "fix" because I can't see how this would happen in reality:  between
> setting override from false to true, I believe there are a few dozen memory
> barriers inside of making an HTTP request.
>
> Unlike some of the other fixes, this is just a theoretical clean-up, I
> assume?
>

First of all, yes, it is a clean-up CL. We want fix all races to catch
regressions (and potentially setup buildbots).

Passing the parameter with the request (/?override=1) would be better, but
current code is at least formally correct (while it still takes advantage
of knowledge that both client and server executes in a single process and
cooperate).

As for "few dozen memory barriers", well, maybe yes, maybe no. In either
case we can't (strongly don't want to) treat it as correct synchronization.
First, it encourages people to use that, second, it will lead to missed
races (where concurrent code synchronizes via sockets according to fact,
but is not intended to; moreover practically it's difficult to say whether
2 file descriptors relate to a single underlying file/socket/pipe).


On Mon, Jan 16, 2012 at 1:02 AM, <dvyukov@google.com> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
>> mpimenov@google.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> net/http: fix data race in test
>> Fixes issue 2712.
>>
>> Please review this at
http://codereview.appspot.com/**5543062/<http://codereview.appspot.com/5543062/>
>>
>> Affected files:
>>  M src/pkg/net/http/fs_test.go
>>
>>
>> Index: src/pkg/net/http/fs_test.go
>> ==============================**==============================**=======
>> --- a/src/pkg/net/http/fs_test.go
>> +++ b/src/pkg/net/http/fs_test.go
>> @@ -224,9 +224,9 @@
>>
>>  func TestServeFileContentType(t *testing.T) {
>>        const ctype = "icecream/chocolate"
>> -       override := false
>> +       override := make(chan bool, 1)
>>        ts := httptest.NewServer(**HandlerFunc(func(w ResponseWriter, r
>> *Request) {
>> -               if override {
>> +               if <-override {
>>                        w.Header().Set("Content-Type", ctype)
>>                }
>>                ServeFile(w, r, "testdata/file")
>> @@ -241,8 +241,9 @@
>>                        t.Errorf("Content-Type mismatch: got %q, want %q",
>> h, want)
>>                }
>>        }
>> +       override <- false
>>        get("text/plain; charset=utf-8")
>> -       override = true
>> +       override <- true
>>        get(ctype)
>>  }
>>
>>
>>
>>
>

[rsc, 2012-01-17 16:14:52]

I agree with Brad about this.  I did not object because this is a test,
but in general it is not the role of the race detection to dictate the
way we write Go code.

[dvyukov, 2012-01-19 12:16:17]

On Tue, Jan 17, 2012 at 8:14 PM, Russ Cox <rsc@golang.org> wrote:

> I agree with Brad about this.  I did not object because this is a test,
> but in general it is not the role of the race detection to dictate the
> way we write Go code.
>

The tool merely points to potential issues. The way to fix it was solely my
idea.