Code review - Issue 6845077: code review 6845077: net/http/httptest: fix possible race on historyListener...https://codereview.appspot.com/2012-11-24T04:50:49+00:00rietveld
Message from unknown
2012-11-22T06:13:28+00:00dfcurn:md5:346d7cbd239894369e5acbfd33f0b535
Message from unknown
2012-11-22T06:13:33+00:00dfcurn:md5:18072869aa2c4140f76996d23d2f173e
Message from unknown
2012-11-22T06:16:02+00:00dfcurn:md5:824ef52e944266368e669896e28a5c05
Message from dave@cheney.net
2012-11-22T06:16:09+00:00dfcurn:md5:3b4953e15eda5d44d64bbfb6b8904754
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go
Message from bradfitz@golang.org
2012-11-22T13:20:00+00:00bradfitzurn:md5:b3e95717c2ae63ff92d5b28fedb3e724
LGTM
On Wed, Nov 21, 2012 at 10:16 PM, <dave@cheney.net> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net/http/httptest: fix possible race on historyListener.history
>
> Please review this at http://codereview.appspot.com/**6845077/<http://codereview.appspot.com/6845077/>
>
> Affected files:
> M src/pkg/net/http/httptest/**server.go
>
>
> Index: src/pkg/net/http/httptest/**server.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/httptest/**server.go
> +++ b/src/pkg/net/http/httptest/**server.go
> @@ -36,13 +36,16 @@
> // accepted.
> type historyListener struct {
> net.Listener
> - history []net.Conn
> + sync.Mutex // protects history
> + history []net.Conn
> }
>
> func (hs *historyListener) Accept() (c net.Conn, err error) {
> c, err = hs.Listener.Accept()
> if err == nil {
> + hs.Lock()
> hs.history = append(hs.history, c)
> + hs.Unlock()
> }
> return
> }
> @@ -96,7 +99,7 @@
> if s.URL != "" {
> panic("Server already started")
> }
> - s.Listener = &historyListener{s.Listener, make([]net.Conn, 0)}
> + s.Listener = &historyListener{Listener: s.Listener}
> s.URL = "http://" + s.Listener.Addr().String()
> s.wrapHandler()
> go s.Config.Serve(s.Listener)
> @@ -122,7 +125,7 @@
> }
> tlsListener := tls.NewListener(s.Listener, s.TLS)
>
> - s.Listener = &historyListener{tlsListener, make([]net.Conn, 0)}
> + s.Listener = &historyListener{Listener: tlsListener}
> s.URL = "https://" + s.Listener.Addr().String()
> s.wrapHandler()
> go s.Config.Serve(s.Listener)
> @@ -161,9 +164,11 @@
> if !ok {
> return
> }
> + hl.Lock()
> for _, conn := range hl.history {
> conn.Close()
> }
> + hl.Unlock()
> }
>
> // waitGroupHandler wraps a handler, incrementing and decrementing a
>
>
>
Message from unknown
2012-11-24T04:50:31+00:00dfcurn:md5:ae17696b0d6f43f02bccc3d17f8473e6
Message from dave@cheney.net
2012-11-24T04:50:49+00:00dfcurn:md5:360543aaab2546bc5550f99e2df71aad
*** Submitted as http://code.google.com/p/go/source/detail?r=ce46d1978338 ***
net/http/httptest: fix possible race on historyListener.history
R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6845077