code review 70950043: net/http/httptest: lock Server's WaitGroup earlier, at ...

net/http/httptest: lock Server's WaitGroup earlier, at Accept time

Fixes Issue 7264

[bradfitz, 2014-03-03 23:09:12]

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-03-04 03:38:27]

I don't see how it helps.
The client does not wait for the server to accept the connection before calling
close.

[dvyukov, 2014-03-04 07:19:04]

On 2014/03/04 03:38:27, dvyukov wrote:
> I don't see how it helps.
> The client does not wait for the server to accept the connection before
calling
> close.

if I correctly read the code, now it's equivalent to:

var wg sync.WaitGroup
httpReq := make(chan bool)

go func() { // acceptor
  for r := range httReq {
    wg.Add(1)
    go func() {  // handler
      // process r
      wg.Done()
    }()
  }
}()

httpReq <- true
time.Sleep(time.Millisecond)
wg.Wait()

there is still no guarantee that wg.Add is executed before wg.Wait

if you want to fix httptest to make such tests legal, you need to wg.Add before
*sending* a http request
then if you send a request before calling ts.Close, Close will wait for the
request

[bradfitz, 2014-04-14 19:35:48]

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

Please take another look.

[bradfitz, 2014-04-14 19:39:39]

Dmitry, PTAL.

Now that my machine reliably shows a race
on TestTransportResponseHeaderTimeout, I've verified that this CL fixes it.

This adds a WaitGroup.Add on the http.StateActive hook, which happens
before the goroutine for the handler is started, and after 1 or more bytes
are received from the network (so obviously the accept happened).

This lets client test code do synchronous HTTP requests in the normal style:

func TestFoo(t *testing.T)
     ts := httptest.NewServer(...)
     defer ts.Close()

     res, err := http.Get(ts.URL)
     ....
}


The http.Get will either fail, or it will return write its request and the
server will read the bytes and increment the waitgroup.



On Mon, Apr 14, 2014 at 12:35 PM, <bradfitz@golang.org> wrote:

> Hello dvyukov@google.com (cc: golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/70950043/
>

[dvyukov, 2014-04-15 06:23:11]

On 2014/04/14 19:39:39, bradfitz wrote:
> Dmitry, PTAL.
> 
> Now that my machine reliably shows a race
> on TestTransportResponseHeaderTimeout, I've verified that this CL fixes it.
> 
> This adds a WaitGroup.Add on the http.StateActive hook, which happens
> before the goroutine for the handler is started, and after 1 or more bytes
> are received from the network (so obviously the accept happened).
> 
> This lets client test code do synchronous HTTP requests in the normal style:
> 
> func TestFoo(t *testing.T)
>      ts := httptest.NewServer(...)
>      defer ts.Close()
> 
>      res, err := http.Get(ts.URL)
>      ....
> }
> 
> 
> The http.Get will either fail, or it will return write its request and the
> server will read the bytes and increment the waitgroup.

I do no see how it fixes the race.
The fact that connect and write on client has succeeded does not mean that
accept on server has returned with this connection. Even if accept has returned,
it does not mean that it had a chance to Add the WaitGroup.
Even if we ignore the above points, the original failing test is using timeout,
so http.Get can return with timeout error even before connect has succeeded.

[dvyukov, 2014-05-01 17:11:32]

Q=close