code review 7322089: log/syslog: fix race in test between channel close and ...

log/syslog: fix race in test between channel close and accept.

Fixes issue 4769.

[remyoudompheng, 2013-02-27 07:06:36]

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

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

[dave_cheney.net, 2013-02-27 09:32:09]

Thank you for your proposal. I am annoyed at this test for having to grow even
more lines for what is supposed to be simple. Maybe you can take some parts from
this counter proposal.

Counter proposal: https://codereview.appspot.com/7392052

[adg, 2013-02-27 21:47:44]

On 2013/02/27 09:32:09, dfc wrote:
> Thank you for your proposal. I am annoyed at this test for having to grow even
> more lines for what is supposed to be simple. Maybe you can take some parts
from
> this counter proposal.
> 
> Counter proposal: https://codereview.appspot.com/7392052

The counter proposal seems more fiddly and less of a known quantity than the
existing test. Remy's fixes seem to address the problem specifically. I don't
particularly like these tests, and this CL doesn't make them appreciably worse
to my eyes.

If you guys think it's worth the time to refine this a bit, then go ahead, but
otherwise this change LGTM.

[bradfitz, 2013-02-27 21:55:42]

LGTM

On Tue, Feb 26, 2013 at 11:06 PM, <remyoudompheng@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> log/syslog: fix race in test between channel close and accept.
>
> Fixes issue 4769.
>
> Please review this at
https://codereview.appspot.**com/7322089/<https://codereview.appspot.com/7322...
>
> Affected files:
>   M src/pkg/log/syslog/syslog_**test.go
>
>
> Index: src/pkg/log/syslog/syslog_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/log/syslog/syslog_**test.go
> +++ b/src/pkg/log/syslog/syslog_**test.go
> @@ -51,7 +51,6 @@
>                 var c net.Conn
>                 var err error
>                 if c, err = l.Accept(); err != nil {
> -                       fmt.Print(err)
>                         return
>                 }
>                 wg.Add(1)
> @@ -71,7 +70,7 @@
>         }
>  }
>
> -func startServer(n, la string, done chan<- string) (addr string, wg
> *sync.WaitGroup) {
> +func startServer(n, la string, done chan<- string) (addr string, sock
> io.Closer, wg *sync.WaitGroup) {
>         if n == "udp" || n == "tcp" {
>                 la = "127.0.0.1:0"
>         } else {
> @@ -95,6 +94,7 @@
>                         log.Fatalf("startServer failed: %v", e)
>                 }
>                 addr = l.LocalAddr().String()
> +               sock = l
>                 wg.Add(1)
>                 go func() {
>                         defer wg.Done()
> @@ -106,7 +106,12 @@
>                         log.Fatalf("startServer failed: %v", e)
>                 }
>                 addr = l.Addr().String()
> -               go runStreamSyslog(l, done, wg)
> +               sock = l
> +               wg.Add(1)
> +               go func() {
> +                       defer wg.Done()
> +                       runStreamSyslog(l, done, wg)
> +               }()
>         }
>         return
>  }
> @@ -117,7 +122,7 @@
>
>         for _, tr := range transport {
>                 done := make(chan string)
> -               addr, _ := startServer(tr, "", done)
> +               addr, _, _ := startServer(tr, "", done)
>                 if tr == "unix" || tr == "unixgram" {
>                         defer os.Remove(addr)
>                 }
> @@ -137,8 +142,9 @@
>  func TestFlap(t *testing.T) {
>         net := "unix"
>         done := make(chan string)
> -       addr, _ := startServer(net, "", done)
> +       addr, sock, _ := startServer(net, "", done)
>         defer os.Remove(addr)
> +       defer sock.Close()
>
>         s, err := Dial(net, addr, LOG_INFO|LOG_USER, "syslog_test")
>         if err != nil {
> @@ -152,7 +158,8 @@
>         check(t, msg, <-done)
>
>         // restart the server
> -       startServer(net, addr, done)
> +       _, sock2, _ := startServer(net, addr, done)
> +       defer sock2.Close()
>
>         // and try retransmitting
>         msg = "Moo 3"
> @@ -242,7 +249,8 @@
>         } else {
>                 for _, test := range tests {
>                         done := make(chan string)
> -                       addr, _ := startServer("udp", "", done)
> +                       addr, sock, _ := startServer("udp", "", done)
> +                       defer sock.Close()
>                         l, err := Dial("udp", addr, test.pri, test.pre)
>                         if err != nil {
>                                 t.Fatalf("syslog.Dial() failed: %v", err)
> @@ -263,7 +271,8 @@
>  }
>
>  func TestConcurrentWrite(t *testing.T) {
> -       addr, _ := startServer("udp", "", make(chan string))
> +       addr, sock, _ := startServer("udp", "", make(chan string))
> +       defer sock.Close()
>         w, err := Dial("udp", addr, LOG_USER|LOG_ERR, "how's it going?")
>         if err != nil {
>                 t.Fatalf("syslog.Dial() failed: %v", err)
> @@ -289,7 +298,7 @@
>
>         net := "unix"
>         done := make(chan string)
> -       addr, srvWG := startServer(net, "", done)
> +       addr, sock, srvWG := startServer(net, "", done)
>         defer os.Remove(addr)
>
>         // count all the messages arriving
> @@ -327,6 +336,7 @@
>                 }()
>         }
>         wg.Wait()
> +       sock.Close()
>         srvWG.Wait()
>         close(done)
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dave_cheney.net, 2013-02-28 00:55:37]

LGTM, passes stress testing for an hour.

+ cc: fullung

[remyoudompheng, 2013-02-28 06:48:42]

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

log/syslog: fix race in test between channel close and accept.

Fixes issue 4769.

R=golang-dev, dave, adg, bradfitz
CC=fullung, golang-dev
https://codereview.appspot.com/7322089