code review 10285044: go.net/ipv6: fix overlooked execution reorder in test

go.net/ipv6: fix overlooked execution reorder in test

The test expects to run opt.clear just after opt.set.

Fixes issue 5696.

[mikio, 2013-06-15 04:40:30]

Hello dave@cheney.net, fullung@gmail.com (cc: golang-dev@googlegroups.com),

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

[dave_cheney.net, 2013-06-16 04:24:29]

On 2013/06/15 04:40:30, mikio wrote:
> Hello mailto:dave@cheney.net, mailto:fullung@gmail.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go.net

Why do these need to be run concurrently ? Something like this would be simpler

diff -r da2eb5dd9ea9 ipv6/control_test.go
--- a/ipv6/control_test.go      Sat Jun 15 18:34:13 2013 +0900
+++ b/ipv6/control_test.go      Sun Jun 16 14:24:14 2013 +1000
@@ -17,12 +17,13 @@
        var wg sync.WaitGroup
        for _, fn := range []ffn{opt.set, opt.clear, opt.clear} {
                wg.Add(1)
-               go func(fn ffn) {
+               f := func(fn ffn) {
                        defer wg.Done()
                        opt.Lock()
                        defer opt.Unlock()
                        fn(tf)
-               }(fn)
+               }
+               f(fn)
        }
        wg.Wait()


What am I missing ?

[mikio, 2013-06-16 04:46:14]

On Sun, Jun 16, 2013 at 1:24 PM,  <dave@cheney.net> wrote:

> Why do these need to be run concurrently

Because you requested to test the rawOpt's lock. ;)

[dave_cheney.net, 2013-06-16 04:55:39]

Fair enough :) I forgot that. In that case could you please add a short comment
to the loop to explain why it is how it is.



On 16/06/2013, at 14:46, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> On Sun, Jun 16, 2013 at 1:24 PM,  <dave@cheney.net> wrote:
> 
>> Why do these need to be run concurrently
> 
> Because you requested to test the rawOpt's lock. ;)

[mikio, 2013-06-16 05:21:23]

Done.

Albert?

On Sun, Jun 16, 2013 at 1:55 PM, Dave Cheney <dave@cheney.net> wrote:
> Fair enough :) I forgot that. In that case could you please add a short
comment to the loop to explain why it is how it is.
>
>
>
> On 16/06/2013, at 14:46, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
>
>> On Sun, Jun 16, 2013 at 1:24 PM,  <dave@cheney.net> wrote:
>>
>>> Why do these need to be run concurrently
>>
>> Because you requested to test the rawOpt's lock. ;)

[dave_cheney.net, 2013-06-17 00:55:23]

Hmm, i'm seeing livelocks running my stress test. Please hold, more information
to come tonight.

[mikio, 2013-06-28 07:27:50]

ping

[dave_cheney.net, 2013-06-29 03:50:24]

On 2013/06/28 07:27:50, mikio wrote:
> ping

I just tried again and this live locks under my stress test

#!/bin/bash

set -e
go test -c # -race
PKG=$(basename $(pwd))

while true ; do 
        export GOMAXPROCS=$[ 1 + $[ RANDOM % 128 ]]
        GOTRACEBACK=2 ./$PKG.test $@ 2>&1
done

cd $PKG; bash stress.bash

[dave_cheney.net, 2013-06-29 03:50:47]

https://codereview.appspot.com/10285044/diff/11001/ipv6/control_test.go
File ipv6/control_test.go (right):

https://codereview.appspot.com/10285044/diff/11001/ipv6/control_test.go#newco...
ipv6/control_test.go:16: // This loop runs methods of raw.Opt concurrentry for
testing
/s/concurrentry/concurrently/

[mikio, 2013-06-29 04:02:20]

On Sat, Jun 29, 2013 at 12:50 PM,  <dave@cheney.net> wrote:

> I just tried again and this live locks under my stress test

ah, I missed that. sure, it really just doesn't hand-off, thx.

[mikio, 2013-06-29 04:03:15]

PTAL

https://codereview.appspot.com/10285044/diff/11001/ipv6/control_test.go
File ipv6/control_test.go (right):

https://codereview.appspot.com/10285044/diff/11001/ipv6/control_test.go#newco...
ipv6/control_test.go:16: // This loop runs methods of raw.Opt concurrentry for
testing
On 2013/06/29 03:50:47, dfc wrote:
> /s/concurrentry/concurrently/

Done.

[dave_cheney.net, 2013-06-29 04:37:02]

LGTM. Thank you.

[mikio, 2013-06-29 05:04:31]

*** Submitted as
https://code.google.com/p/go/source/detail?r=0579515385a2&repo=net ***

go.net/ipv6: fix overlooked execution reorder in test

The test expects to run opt.clear just after opt.set.

Fixes issue 5696.

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