code review 70410044: net/http: proactively address potential race in Ser...

net/http: proactively address potential race in Server.ConnState

The addition of Server.ConnState provides all the necessary hooks to
stop a Server gracefully.

Users are very likely to reach for sync.WaitGroup before sync/atomic
to track active connections, especially considering the warning in the
sync/atomic package: "Except for special, low-level applications,
synchronization is better done with channels or the facilities of the
sync package."

The sync.WaitGroup documentation says, "Note that calls with positive
delta must happen before the call to Wait, or else Wait may wait for
too small a group. Typically this means the calls to Add should
execute before the statement creating the goroutine or other event to
be waited for."

So to allow safe use of a sync.WaitGroup in a Server.ConnState
callback, I believe we need to move the conn.setState for StateNew
into Server.newConn so it takes place before the goroutine is spawned.

https://github.com/rcrowley/go-tigertonic/blob/go1.3/server.go#L42-L43
has an example where I've moved the sync.WaitGroup from net.Conn and
net.Listener wrappers into Server.ConnState.

I brought this up in https://codereview.appspot.com/69260044/#msg13
but didn't explain it sufficiently.

I'm happy to write a test to accompany this change but I'm at a loss
for how to write a deterministic test, hence the appeal to
documentation and the principle of least surprise.

[rcrowley, 2014-03-01 23:11:14]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-03-02 00:42:25]

I still see exactly no difference between this and what's already there.

What event do you think this now happens before?  "a new goroutine" is
true, but that's not guarding any data or call to any other locking event.
 Having all the state callbacks in the same function right next to each
other is still my preference, in lieu of any compelling arguments about why
StateNew needs to be elsewhere.

You writing "I believe we need to move" and your difficulty imagining a
suitable test both hint that you're also not convinced.

I wouldn't use StateNew to do the WaitGroup operation. It's racy regardless
of where we move the callback... you need to increment the WaitGroup in
your net.Listener implementation BEFORE you return the result of the
Listener.Accept to the net/http package.

But if you don't want to do a custom net.Listener, I don't think you need
to--- just Close the Listener, SetKeepAlivesEnabled(false), look at all
Idle requests, and then sleep 5 (or whatever) seconds, and then close
anything that's still idle, and then trust your StateNew's WaitGroup.Add(1)
races have settled (it's been 5+ seconds), and use WaitGroup.Wait before
killing things forcefully.

Or convince me otherwise with a description of events or code.




On Sat, Mar 1, 2014 at 3:11 PM, <r@rcrowley.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
>     net/http: proactively address potential race in Server.ConnState
>
> The addition of Server.ConnState provides all the necessary hooks to
> stop a Server gracefully.
>
> Users are very likely to reach for sync.WaitGroup before sync/atomic
> to track active connections, especially considering the warning in the
> sync/atomic package: "Except for special, low-level applications,
> synchronization is better done with channels or the facilities of the
> sync package."
>
> The sync.WaitGroup documentation says, "Note that calls with positive
> delta must happen before the call to Wait, or else Wait may wait for
> too small a group. Typically this means the calls to Add should
> execute before the statement creating the goroutine or other event to
> be waited for."
>
> So to allow safe use of a sync.WaitGroup in a Server.ConnState
> callback, I believe we need to move the conn.setState for StateNew
> into Server.newConn so it takes place before the goroutine is spawned.
>
> https://github.com/rcrowley/go-tigertonic/blob/go1.3/server.go#L42-L43
> has an example where I've moved the sync.WaitGroup from net.Conn and
> net.Listener wrappers into Server.ConnState.
>
> I brought this up in https://codereview.appspot.com/69260044/#msg13
> but didn't explain it sufficiently.
>
> I'm happy to write a test to accompany this change but I'm at a loss
> for how to write a deterministic test, hence the appeal to
> documentation and the principle of least surprise.
>
> Please review this at https://codereview.appspot.com/70410044/
>
> Affected files (+1, -1 lines):
>   M src/pkg/net/http/server.go
>
>
> Index: src/pkg/net/http/server.go
> ===================================================================
> --- a/src/pkg/net/http/server.go
> +++ b/src/pkg/net/http/server.go
> @@ -428,6 +428,7 @@
>         if debugServerConnections {
>                 c.rwc = newLoggingConn("server", c.rwc)
>         }
> +       c.setState(c.rwc, StateNew)
>         c.sr = liveSwitchReader{r: c.rwc}
>         c.lr = io.LimitReader(&c.sr, noLimit).(*io.LimitedReader)
>         br := newBufioReader(c.lr)
> @@ -1090,7 +1091,6 @@
>  // Serve a new connection.
>  func (c *conn) serve() {
>         origConn := c.rwc // copy it before it's set nil on Close or Hijack
> -       c.setState(origConn, StateNew)
>         defer func() {
>                 if err := recover(); err != nil {
>                         const size = 64 << 10
>
>
>

[rcrowley, 2014-03-02 01:46:21]

> You writing "I believe we need to move" and your difficulty imagining a
> suitable test both hint that you're also not convinced.

I am convinced but it's very difficult, unless there are tools I'm unaware of,
to write a test that depends on the order in which goroutines run.

> I wouldn't use StateNew to do the WaitGroup operation. It's racy regardless
> of where we move the callback... you need to increment the WaitGroup in
> your net.Listener implementation BEFORE you return the result of the
> Listener.Accept to the net/http package.

I think incrementing the sync.WaitGroup in Listener.Accept is functionally
equivalent to incrementing it prior to

    go c.serve()

in Server.Serve.
 
> Or convince me otherwise with a description of events or code.

This isn't a Go test but it is a series of events I believe to be possible in a
sample program that, if you're unlucky at runtime, will not work as expected.

goroutine 1 go handleSignals()
goroutine 1 srv.Serve(l)
goroutine 1 rw, e := l.Accept() // e is nil
goroutine 1 c, err := srv.newConn(rw)
goroutine 1 go c.serve()
goroutine 2 l.Close()
goroutine 1 rw, e := l.Accept() // e is "use of closed network connection"
goroutine 1 wg.Wait()
goroutine 1 wg.Wait() // returns
goroutine 3 c.serve()
goroutine 3 c.setState(origConn, StateNew)
goroutine 3 wg.Add(1)

package main

import (
    "log"
    "net"
    "net/http"
    "os"
    "os/signal"
    "sync"
    "syscall"
)

func handleSignals(l net.Listener) {
    ch := make(chan os.Signal)
    signal.Notify(ch, syscall.SIGINT, syscall.SIGTERM)
    <-ch
    l.Close()
}

func main() { // call this "goroutine 1"
    l, err := net.Listen("tcp", "127.0.0.1:0")
    if err != nil {
        log.Fatalln(err)
    }
    log.Println("listening on", l.Addr())
    go handleSignals(l) // call this "goroutine 2"
    srv := http.Server{}
    var wg sync.WaitGroup
    srv.ConnState = func(c net.Conn, state http.ConnState) {
        switch state { // obviously incomplete
        case http.StateNew:
            wg.Add(1)
        case http.StateClosed:
            wg.Done()
        }
    }
    srv.Serve(l) // call the first connection accepted "goroutine 3"
    wg.Wait()
}

With this CL, the order-of-operations becomes

goroutine 1 go handleSignals()
goroutine 1 srv.Serve(l)
goroutine 1 rw, e := l.Accept() // e is nil
goroutine 1 c, err := srv.newConn(rw)
goroutine 1 c.setState(origConn, StateNew)
goroutine 1 wg.Add(1)
goroutine 1 go c.serve()
goroutine 2 l.Close()
goroutine 1 rw, e := l.Accept() // e is "use of closed network connection"
goroutine 1 wg.Wait()
goroutine 3 c.serve()
goroutine 1 wg.Wait() // returns

Does my expectation/understanding align with reality?

[bradfitz, 2014-03-02 02:38:50]

Sorry, sorry! Okay, I finally understand. Thanks for the explanation and
code.

You want the Serve method to not return before successfully-accepted Conns
call the StateNew callback.

Let's move the setState call to the Serve func, though, to make it more
clear that's what it's protecting, perhaps with a comment like:

func (srv *Server) Serve(l net.Listener) error {
        defer l.Close()
        var tempDelay time.Duration // how long to sleep on accept failure
        for {
                rw, e := l.Accept()
                if e != nil {
....
                c, err := srv.newConn(rw)
                if err != nil {
                        continue
                }
                c.setState(c.rwc, StateNew) // before Accept can fail &
cause this func to return
                go c.serve()
        }
}

Here's a test you can include in your CL that shows a data race under go
test -run=StateNew$ -race -cpu=2 , but works with your fix:

diff -r 891e16653547 src/pkg/net/http/serve_test.go
--- a/src/pkg/net/http/serve_test.go Sat Mar 01 11:13:29 2014 +1100
+++ b/src/pkg/net/http/serve_test.go Sat Mar 01 18:36:47 2014 -0800
@@ -2335,6 +2335,29 @@
  mu.Unlock()
 }

+// TestServerConnStateNew tests that the ConnState hook for StateNew is
called
+// synchronously before the next listener.Accept.
+// This test is designed to fail under the race detector.
+func TestServerConnStateNew(t *testing.T) {
+ sawNew := false // if the test is buggy, we'll race on this variable.
+
+ srv := &Server{
+ ConnState: func(c net.Conn, state ConnState) {
+ sawNew = true // testing that this write is safe
+ },
+ Handler: HandlerFunc(func(w ResponseWriter, r *Request) {}), // irrelevant
+ }
+ go srv.Serve(&oneConnListener{
+ conn: &rwTestConn{
+ Reader: strings.NewReader("GET / HTTP/1.1\r\nHost: foo\r\n\r\n"),
+ Writer: ioutil.Discard,
+ },
+ })
+ if !sawNew { // testing that this read is safe
+ t.Error("StateNew not seen")
+ }
+}
+
 func mustGet(t *testing.T, url string, headers ...string) {
  req, err := NewRequest("GET", url, nil)
  if err != nil {


The failure was:

=== RUN TestServerConnStateNew-2
--- FAIL: TestServerConnStateNew-2 (0.00 seconds)
serve_test.go:2357: StateNew not seen
==================
WARNING: DATA RACE
Write by goroutine 6:
  net/http_test.func·128()
      /Users/bradfitz/go/src/pkg/net/http/serve_test.go:2346 +0x4b
  net/http.(*conn).setState()
      /Users/bradfitz/go/src/pkg/net/http/server.go:1086 +0x8a
  net/http.(*conn).serve()
      /Users/bradfitz/go/src/pkg/net/http/server.go:1093 +0xbd

Previous read by goroutine 4:
  net/http_test.TestServerConnStateNew()
      /Users/bradfitz/go/src/pkg/net/http/serve_test.go:2356 +0x2e1
FAIL
  testing.tRunner()
      /Users/bradfitz/go/src/pkg/testing/testing.go:422 +0x121

Goroutine 6 (running) created at:
  net/http.(*Server).Serve()
      /Users/bradfitz/go/src/pkg/net/http/server.go:1725 +0x2fd

Goroutine 4 (finished) created at:
  testing.RunTests()
      /Users/bradfitz/go/src/pkg/testing/testing.go:503 +0xb14
exit status 1
FAIL net/http 0.038s



Thanks for your perseverance on this.



On Sat, Mar 1, 2014 at 5:46 PM, <r@rcrowley.org> wrote:

> You writing "I believe we need to move" and your difficulty imagining
>>
> a
>
>> suitable test both hint that you're also not convinced.
>>
>
> I am convinced but it's very difficult, unless there are tools I'm
> unaware of, to write a test that depends on the order in which
> goroutines run.
>
>
>  I wouldn't use StateNew to do the WaitGroup operation. It's racy
>>
> regardless
>
>> of where we move the callback... you need to increment the WaitGroup
>>
> in
>
>> your net.Listener implementation BEFORE you return the result of the
>> Listener.Accept to the net/http package.
>>
>
> I think incrementing the sync.WaitGroup in Listener.Accept is
> functionally equivalent to incrementing it prior to
>
>     go c.serve()
>
> in Server.Serve.
>
>
>  Or convince me otherwise with a description of events or code.
>>
>
> This isn't a Go test but it is a series of events I believe to be
> possible in a sample program that, if you're unlucky at runtime, will
> not work as expected.
>
> goroutine 1 go handleSignals()
> goroutine 1 srv.Serve(l)
> goroutine 1 rw, e := l.Accept() // e is nil
> goroutine 1 c, err := srv.newConn(rw)
> goroutine 1 go c.serve()
> goroutine 2 l.Close()
> goroutine 1 rw, e := l.Accept() // e is "use of closed network
> connection"
> goroutine 1 wg.Wait()
> goroutine 1 wg.Wait() // returns
> goroutine 3 c.serve()
> goroutine 3 c.setState(origConn, StateNew)
> goroutine 3 wg.Add(1)
>
> package main
>
> import (
>     "log"
>     "net"
>     "net/http"
>     "os"
>     "os/signal"
>     "sync"
>     "syscall"
> )
>
> func handleSignals(l net.Listener) {
>     ch := make(chan os.Signal)
>     signal.Notify(ch, syscall.SIGINT, syscall.SIGTERM)
>     <-ch
>     l.Close()
> }
>
> func main() { // call this "goroutine 1"
>     l, err := net.Listen("tcp", "127.0.0.1:0")
>     if err != nil {
>         log.Fatalln(err)
>     }
>     log.Println("listening on", l.Addr())
>     go handleSignals(l) // call this "goroutine 2"
>     srv := http.Server{}
>     var wg sync.WaitGroup
>     srv.ConnState = func(c net.Conn, state http.ConnState) {
>         switch state { // obviously incomplete
>         case http.StateNew:
>             wg.Add(1)
>         case http.StateClosed:
>             wg.Done()
>         }
>     }
>     srv.Serve(l) // call the first connection accepted "goroutine 3"
>     wg.Wait()
> }
>
> With this CL, the order-of-operations becomes
>
> goroutine 1 go handleSignals()
> goroutine 1 srv.Serve(l)
> goroutine 1 rw, e := l.Accept() // e is nil
> goroutine 1 c, err := srv.newConn(rw)
> goroutine 1 c.setState(origConn, StateNew)
> goroutine 1 wg.Add(1)
> goroutine 1 go c.serve()
> goroutine 2 l.Close()
> goroutine 1 rw, e := l.Accept() // e is "use of closed network
> connection"
> goroutine 1 wg.Wait()
> goroutine 3 c.serve()
> goroutine 1 wg.Wait() // returns
>
> Does my expectation/understanding align with reality?
>
> https://codereview.appspot.com/70410044/
>

[bradfitz, 2014-03-02 02:47:01]

Sorry, that test isn't sufficient.  (It's always racy... left the "go" in
there)

Try this:

diff -r 891e16653547 src/pkg/net/http/serve_test.go
--- a/src/pkg/net/http/serve_test.go Sat Mar 01 11:13:29 2014 +1100
+++ b/src/pkg/net/http/serve_test.go Sat Mar 01 18:46:37 2014 -0800
@@ -2335,6 +2335,30 @@
  mu.Unlock()
 }

+// TestServerConnStateNew tests that the ConnState hook for StateNew is
called
+// synchronously before the next listener.Accept.
+// This test is designed to fail under the race detector.
+func TestServerConnStateNew(t *testing.T) {
+ sawNew := false // if the test is buggy, we'll race on this variable.
+ srv := &Server{
+ ConnState: func(c net.Conn, state ConnState) {
+ if state == StateNew {
+ sawNew = true // testing that this write isn't racy
+ }
+ },
+ Handler: HandlerFunc(func(w ResponseWriter, r *Request) {}), // irrelevant
+ }
+ srv.Serve(&oneConnListener{
+ conn: &rwTestConn{
+ Reader: strings.NewReader("GET / HTTP/1.1\r\nHost: foo\r\n\r\n"),
+ Writer: ioutil.Discard,
+ },
+ })
+ if !sawNew { // testing that this read isn't racy
+ t.Error("StateNew not seen")
+ }
+}
+
 func mustGet(t *testing.T, url string, headers ...string) {
  req, err := NewRequest("GET", url, nil)
  if err != nil {






On Sat, Mar 1, 2014 at 6:38 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Sorry, sorry! Okay, I finally understand. Thanks for the explanation and
> code.
>
> You want the Serve method to not return before successfully-accepted Conns
> call the StateNew callback.
>
> Let's move the setState call to the Serve func, though, to make it more
> clear that's what it's protecting, perhaps with a comment like:
>
> func (srv *Server) Serve(l net.Listener) error {
>         defer l.Close()
>         var tempDelay time.Duration // how long to sleep on accept failure
>         for {
>                 rw, e := l.Accept()
>                 if e != nil {
> ....
>                 c, err := srv.newConn(rw)
>                 if err != nil {
>                         continue
>                 }
>                 c.setState(c.rwc, StateNew) // before Accept can fail &
> cause this func to return
>                 go c.serve()
>         }
> }
>
> Here's a test you can include in your CL that shows a data race under go
> test -run=StateNew$ -race -cpu=2 , but works with your fix:
>
> diff -r 891e16653547 src/pkg/net/http/serve_test.go
> --- a/src/pkg/net/http/serve_test.go Sat Mar 01 11:13:29 2014 +1100
> +++ b/src/pkg/net/http/serve_test.go Sat Mar 01 18:36:47 2014 -0800
> @@ -2335,6 +2335,29 @@
>   mu.Unlock()
>  }
>
> +// TestServerConnStateNew tests that the ConnState hook for StateNew is
> called
> +// synchronously before the next listener.Accept.
> +// This test is designed to fail under the race detector.
> +func TestServerConnStateNew(t *testing.T) {
> + sawNew := false // if the test is buggy, we'll race on this variable.
> +
> + srv := &Server{
> + ConnState: func(c net.Conn, state ConnState) {
> + sawNew = true // testing that this write is safe
> + },
> + Handler: HandlerFunc(func(w ResponseWriter, r *Request) {}), //
> irrelevant
> + }
> + go srv.Serve(&oneConnListener{
> + conn: &rwTestConn{
> + Reader: strings.NewReader("GET / HTTP/1.1\r\nHost: foo\r\n\r\n"),
> + Writer: ioutil.Discard,
> + },
> + })
> + if !sawNew { // testing that this read is safe
> + t.Error("StateNew not seen")
> + }
> +}
> +
>  func mustGet(t *testing.T, url string, headers ...string) {
>   req, err := NewRequest("GET", url, nil)
>   if err != nil {
>
>
> The failure was:
>
> === RUN TestServerConnStateNew-2
> --- FAIL: TestServerConnStateNew-2 (0.00 seconds)
> serve_test.go:2357: StateNew not seen
> ==================
> WARNING: DATA RACE
> Write by goroutine 6:
>   net/http_test.func·128()
>       /Users/bradfitz/go/src/pkg/net/http/serve_test.go:2346 +0x4b
>   net/http.(*conn).setState()
>       /Users/bradfitz/go/src/pkg/net/http/server.go:1086 +0x8a
>   net/http.(*conn).serve()
>       /Users/bradfitz/go/src/pkg/net/http/server.go:1093 +0xbd
>
> Previous read by goroutine 4:
>   net/http_test.TestServerConnStateNew()
>       /Users/bradfitz/go/src/pkg/net/http/serve_test.go:2356 +0x2e1
> FAIL
>   testing.tRunner()
>       /Users/bradfitz/go/src/pkg/testing/testing.go:422 +0x121
>
> Goroutine 6 (running) created at:
>   net/http.(*Server).Serve()
>       /Users/bradfitz/go/src/pkg/net/http/server.go:1725 +0x2fd
>
> Goroutine 4 (finished) created at:
>   testing.RunTests()
>       /Users/bradfitz/go/src/pkg/testing/testing.go:503 +0xb14
> exit status 1
> FAIL net/http 0.038s
>
>
>
> Thanks for your perseverance on this.
>
>
>
> On Sat, Mar 1, 2014 at 5:46 PM, <r@rcrowley.org> wrote:
>
>> You writing "I believe we need to move" and your difficulty imagining
>>>
>> a
>>
>>> suitable test both hint that you're also not convinced.
>>>
>>
>> I am convinced but it's very difficult, unless there are tools I'm
>> unaware of, to write a test that depends on the order in which
>> goroutines run.
>>
>>
>>  I wouldn't use StateNew to do the WaitGroup operation. It's racy
>>>
>> regardless
>>
>>> of where we move the callback... you need to increment the WaitGroup
>>>
>> in
>>
>>> your net.Listener implementation BEFORE you return the result of the
>>> Listener.Accept to the net/http package.
>>>
>>
>> I think incrementing the sync.WaitGroup in Listener.Accept is
>> functionally equivalent to incrementing it prior to
>>
>>     go c.serve()
>>
>> in Server.Serve.
>>
>>
>>  Or convince me otherwise with a description of events or code.
>>>
>>
>> This isn't a Go test but it is a series of events I believe to be
>> possible in a sample program that, if you're unlucky at runtime, will
>> not work as expected.
>>
>> goroutine 1 go handleSignals()
>> goroutine 1 srv.Serve(l)
>> goroutine 1 rw, e := l.Accept() // e is nil
>> goroutine 1 c, err := srv.newConn(rw)
>> goroutine 1 go c.serve()
>> goroutine 2 l.Close()
>> goroutine 1 rw, e := l.Accept() // e is "use of closed network
>> connection"
>> goroutine 1 wg.Wait()
>> goroutine 1 wg.Wait() // returns
>> goroutine 3 c.serve()
>> goroutine 3 c.setState(origConn, StateNew)
>> goroutine 3 wg.Add(1)
>>
>> package main
>>
>> import (
>>     "log"
>>     "net"
>>     "net/http"
>>     "os"
>>     "os/signal"
>>     "sync"
>>     "syscall"
>> )
>>
>> func handleSignals(l net.Listener) {
>>     ch := make(chan os.Signal)
>>     signal.Notify(ch, syscall.SIGINT, syscall.SIGTERM)
>>     <-ch
>>     l.Close()
>> }
>>
>> func main() { // call this "goroutine 1"
>>     l, err := net.Listen("tcp", "127.0.0.1:0")
>>     if err != nil {
>>         log.Fatalln(err)
>>     }
>>     log.Println("listening on", l.Addr())
>>     go handleSignals(l) // call this "goroutine 2"
>>     srv := http.Server{}
>>     var wg sync.WaitGroup
>>     srv.ConnState = func(c net.Conn, state http.ConnState) {
>>         switch state { // obviously incomplete
>>         case http.StateNew:
>>             wg.Add(1)
>>         case http.StateClosed:
>>             wg.Done()
>>         }
>>     }
>>     srv.Serve(l) // call the first connection accepted "goroutine 3"
>>     wg.Wait()
>> }
>>
>> With this CL, the order-of-operations becomes
>>
>> goroutine 1 go handleSignals()
>> goroutine 1 srv.Serve(l)
>> goroutine 1 rw, e := l.Accept() // e is nil
>> goroutine 1 c, err := srv.newConn(rw)
>> goroutine 1 c.setState(origConn, StateNew)
>> goroutine 1 wg.Add(1)
>> goroutine 1 go c.serve()
>> goroutine 2 l.Close()
>> goroutine 1 rw, e := l.Accept() // e is "use of closed network
>> connection"
>> goroutine 1 wg.Wait()
>> goroutine 3 c.serve()
>> goroutine 1 wg.Wait() // returns
>>
>> Does my expectation/understanding align with reality?
>>
>> https://codereview.appspot.com/70410044/
>>
>
>

[rcrowley, 2014-03-02 03:20:06]

> You want the Serve method to not return before successfully-accepted Conns
> call the StateNew callback.

Yes, that is a much shorter way to say it.

> 
> Let's move the setState call to the Serve func, though, to make it more
> clear that's what it's protecting, perhaps with a comment like:

Done.  

> Thanks for your perseverance on this.

Right back at you!

[bradfitz, 2014-03-02 03:25:24]

Please include that new test in this CL too.

[bradfitz, 2014-03-02 03:27:24]

https://codereview.appspot.com/70410044/diff/60001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/70410044/diff/60001/src/pkg/net/http/server.go...
src/pkg/net/http/server.go:1724: 
this is unnecessarily whitespacey compared to the rest of this function. looks
out of place.

just do a same-line comment:

c.setState(c.rwc, StateNew) // before Serve can return

[rcrowley, 2014-03-02 03:29:07]

On 2014/03/02 03:25:24, bradfitz wrote:
> Please include that new test in this CL too.

Done.  Was testing it separately and it works well.

I hadn't been thinking to try to trigger the race detector.  Thanks for that.

[rcrowley, 2014-03-02 03:30:56]

https://codereview.appspot.com/70410044/diff/60001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/70410044/diff/60001/src/pkg/net/http/server.go...
src/pkg/net/http/server.go:1724: 
On 2014/03/02 03:27:25, bradfitz wrote:
> this is unnecessarily whitespacey compared to the rest of this function. looks
> out of place.
> 
> just do a same-line comment:
> 
> c.setState(c.rwc, StateNew) // before Serve can return

Done.

[bradfitz, 2014-03-02 03:39:00]

I still don't see the test in this CL, so I've sent you
https://codereview.appspot.com/70460043



On Sat, Mar 1, 2014 at 7:30 PM, <r@rcrowley.org> wrote:

>
> https://codereview.appspot.com/70410044/diff/60001/src/
> pkg/net/http/server.go
> File src/pkg/net/http/server.go (right):
>
> https://codereview.appspot.com/70410044/diff/60001/src/
> pkg/net/http/server.go#newcode1724
> src/pkg/net/http/server.go:1724:
> On 2014/03/02 03:27:25, bradfitz wrote:
>
>> this is unnecessarily whitespacey compared to the rest of this
>>
> function. looks
>
>> out of place.
>>
>
>  just do a same-line comment:
>>
>
>  c.setState(c.rwc, StateNew) // before Serve can return
>>
>
> Done.
>
> https://codereview.appspot.com/70410044/
>

[rcrowley, 2014-03-02 03:47:54]

On 2014/03/02 03:39:00, bradfitz wrote:
> I still don't see the test in this CL, so I've sent you
> https://codereview.appspot.com/70460043

I missed the `hg file ...` command.  My bad.  Uploaded now, for real.

[bradfitz, 2014-03-02 04:31:57]

LGTM



On Sat, Mar 1, 2014 at 7:47 PM, <r@rcrowley.org> wrote:

> On 2014/03/02 03:39:00, bradfitz wrote:
>
>> I still don't see the test in this CL, so I've sent you
>> https://codereview.appspot.com/70460043
>>
>
> I missed the `hg file ...` command.  My bad.  Uploaded now, for real.
>
> https://codereview.appspot.com/70410044/
>

[bradfitz, 2014-03-02 04:32:53]

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

net/http: ensure ConnState for StateNew fires before Server.Serve returns

The addition of Server.ConnState provides all the necessary
hooks to stop a Server gracefully, but StateNew previously
could fire concurrently with Serve exiting (as it does when
its net.Listener is closed). This previously meant one
couldn't use a WaitGroup incremented in the StateNew hook
along with calling Wait after Serve. Now you can.

Update Issue 4674

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/70410044

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[gobot, 2014-03-02 05:14:13]

This CL appears to have broken the darwin-386-cheney builder.

[dvyukov, 2014-03-02 06:24:43]

On 2014/03/02 05:14:13, gobot wrote:
> This CL appears to have broken the darwin-386-cheney builder.

Brad, https://code.google.com/p/go/issues/detail?id=7264 is about a similar
situation in a test.