code review 12686045: runtime: fix network timers related crash

runtime: fix network timers related crash
Fixes issue 6103.

[dvyukov, 2013-08-12 08:21:03]

Hello golang-dev@googlegroups.com,

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

[brainman, 2013-08-12 23:16:37]

LGTM

for all I know about this stuff.

Alex

[dvyukov, 2013-08-13 08:56:35]

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

runtime: fix network timers related crash
Fixes issue 6103.

R=golang-dev, alex.brainman
CC=golang-dev
https://codereview.appspot.com/12686045

[bsiegert, 2013-08-13 13:55:09]

This change broke the build on Windows, OpenBSD, NetBSD and FreeBSD,
as far as I can see. The test times out after 2 minutes.

Example: http://build.golang.org/log/6bb93acb1bd32675761813840a85bb7f259a817c
(NetBSD/x64).

On Mon, Aug 12, 2013 at 10:21 AM,  <dvyukov@google.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: fix network timers related crash
> Fixes issue 6103.
>
> Please review this at https://codereview.appspot.com/12686045/
>
> Affected files:
>   M src/pkg/net/timeout_test.go
>   M src/pkg/runtime/netpoll.goc
>
>
> Index: src/pkg/net/timeout_test.go
> ===================================================================
> --- a/src/pkg/net/timeout_test.go
> +++ b/src/pkg/net/timeout_test.go
> @@ -703,3 +703,36 @@
>                 c.Write(buf[:])
>         }
>  }
> +
> +func TestDeadlineRace(t *testing.T) {
> +       switch runtime.GOOS {
> +       case "plan9":
> +               t.Skipf("skipping test on %q", runtime.GOOS)
> +       }
> +
> +       defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(4))
> +       ln := newLocalListener(t)
> +       defer ln.Close()
> +       c, err := Dial("tcp", ln.Addr().String())
> +       if err != nil {
> +               t.Fatalf("Dial: %v", err)
> +       }
> +       defer c.Close()
> +       done := make(chan bool)
> +       go func() {
> +               t := time.NewTicker(2 * time.Microsecond).C
> +               for {
> +                       if err := c.SetDeadline(time.Now().Add(2 *
> time.Microsecond)); err != nil {
> +                               break
> +                       }
> +                       <-t
> +               }
> +               done <- true
> +       }()
> +       var buf [1]byte
> +       for i := 0; i < 1024; i++ {
> +               c.Read(buf[:]) // ignore possible timeout errors
> +       }
> +       c.Close()
> +       <-done
> +}
> Index: src/pkg/runtime/netpoll.goc
> ===================================================================
> --- a/src/pkg/runtime/netpoll.goc
> +++ b/src/pkg/runtime/netpoll.goc
> @@ -113,10 +113,13 @@
>         runtime·lock(pd);
>         err = checkerr(pd, mode);
>         if(err == 0) {
> -               if(!netpollblock(pd, mode)) {
> +               while(!netpollblock(pd, mode)) {
>                         err = checkerr(pd, mode);
> -                       if(err == 0)
> -                               runtime·throw("runtime_pollWait: unblocked
> by ioready");
> +                       if(err != 0)
> +                               break;
> +                       // Can happen if timeout has fired and unblocked us,
> +                       // but before we had a chance to run, timeout has
> been reset.
> +                       // Pretend it has not happened and retry.
>                 }
>         }
>         runtime·unlock(pd);
>
>
> --
>
> ---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.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>



-- 
The first essential in chemistry is that you should perform practical
work and conduct experiments, for he who performs not practical work
nor makes experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[dvyukov, 2013-08-13 14:04:10]

On 2013/08/13 13:55:09, bsiegert wrote:
> This change broke the build on Windows, OpenBSD, NetBSD and FreeBSD,
> as far as I can see. The test times out after 2 minutes.

The test turned out to be that good that it uncovered 4 different bugs.
The fix is here:
https://codereview.appspot.com/12847043/
Once somebody LGTMs it, I will submit.
Sorry.

[brainman, 2013-08-14 04:32:47]

Dmitry,

Unfortunately, I think TestDeadlineRace is not going to work. There is a race
here in this test:

1) read blocks waiting for deadline to arrive;
2) net deadline is fired;
3) ticker channel is ready and deadline is extended;

2 and 3 are fired simultaneously. Where is guarantee that 2 happens before 3?
And once 3 completes 2 is no longer true. We could make ticker deadline longer,
so net deadline always gets fired, but that defeat the purpose of this test.

And secondly, even if everything works properly here, just waiting on the ticker
for 1024 times on my Windows XP takes more then 15s
(https://code.google.com/p/go/issues/detail?id=6142). And that is unacceptable
for -short test.

Alex

[dvyukov, 2013-08-14 09:51:27]

You are right.
I will address this today.


On Wed, Aug 14, 2013 at 8:32 AM, <alex.brainman@gmail.com> wrote:

> Dmitry,
>
> Unfortunately, I think TestDeadlineRace is not going to work. There is a
> race here in this test:
>
> 1) read blocks waiting for deadline to arrive;
> 2) net deadline is fired;
> 3) ticker channel is ready and deadline is extended;
>
> 2 and 3 are fired simultaneously. Where is guarantee that 2 happens
> before 3? And once 3 completes 2 is no longer true. We could make ticker
> deadline longer, so net deadline always gets fired, but that defeat the
> purpose of this test.
>
> And secondly, even if everything works properly here, just waiting on
> the ticker for 1024 times on my Windows XP takes more then 15s
>
(https://code.google.com/p/go/**issues/detail?id=6142<https://code.google.com/...).
> And that is
> unacceptable for -short test.
>
> Alex
>
>
https://codereview.appspot.**com/12686045/<https://codereview.appspot.com/126...
>

[dvyukov, 2013-08-14 17:11:19]

sent https://codereview.appspot.com/12937043


On Wed, Aug 14, 2013 at 1:51 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> You are right.
> I will address this today.
>
>
> On Wed, Aug 14, 2013 at 8:32 AM, <alex.brainman@gmail.com> wrote:
>
>> Dmitry,
>>
>> Unfortunately, I think TestDeadlineRace is not going to work. There is a
>> race here in this test:
>>
>> 1) read blocks waiting for deadline to arrive;
>> 2) net deadline is fired;
>> 3) ticker channel is ready and deadline is extended;
>>
>> 2 and 3 are fired simultaneously. Where is guarantee that 2 happens
>> before 3? And once 3 completes 2 is no longer true. We could make ticker
>> deadline longer, so net deadline always gets fired, but that defeat the
>> purpose of this test.
>>
>> And secondly, even if everything works properly here, just waiting on
>> the ticker for 1024 times on my Windows XP takes more then 15s
>>
(https://code.google.com/p/go/**issues/detail?id=6142<https://code.google.com/...).
>> And that is
>> unacceptable for -short test.
>>
>> Alex
>>
>>
https://codereview.appspot.**com/12686045/<https://codereview.appspot.com/126...
>>
>
>