code review 12847043: runtime: fix handling of network deadlines

runtime: fix handling of network deadlines
Ensure that deadlines affect already issued IO.

[dvyukov, 2013-08-13 09:50:50]

Hello golang-dev@googlegroups.com,

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

[mikio, 2013-08-13 10:29:34]

Dmitry,

Looks like TestDeadlineRace in timeout_test.go is still broken.
Now, and probably in Go 1.2, we will keep four network pollsters like
the following:

a) runtime-integrated network pollster on top of epoll, Linux
b) runtime-integrated network pollster on top of IO completion mech., Windows
c) runtime-integrated network pollster on top of kqueue, FreeBSD,
OpenBSD (I'm working on, not yet completed)
d) package net own pollster on top of kqueue (one-shot IO bounce),
FreeBSD/ARM, NetBSD

Unfortunately, setDeadline of (d) will never return non-nil value, thanks. ;)


On Tue, Aug 13, 2013 at 6:50 PM,  <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 handling of network deadlines
> Ensure that deadlines affect already issued IO.
>
> Please review this at https://codereview.appspot.com/12847043/
>
> 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
> @@ -718,6 +718,10 @@
>                 t.Fatalf("Dial: %v", err)
>         }
>         defer c.Close()
> +       // TODO(dvyukov): the old poller can't change deadline for an
> already issued IO.
> +       // So ensure that we have some initial deadline.
> +       // Remove this once all OSes switch to the runtime poller.
> +       c.SetDeadline(time.Now().Add(2 * time.Microsecond))
>         done := make(chan bool)
>         go func() {
>                 t := time.NewTicker(2 * time.Microsecond).C
> Index: src/pkg/runtime/netpoll.goc
> ===================================================================
> --- a/src/pkg/runtime/netpoll.goc
> +++ b/src/pkg/runtime/netpoll.goc
> @@ -134,9 +134,13 @@
>  }
>
>  func runtime_pollSetDeadline(pd *PollDesc, d int64, mode int) {
> +       G *rg, *wg;
> +
>         runtime·lock(pd);
> -       if(pd->closing)
> -               goto ret;
> +       if(pd->closing) {
> +               runtime·unlock(pd);
> +               return;
> +       }
>         pd->seq++;  // invalidate current timers
>         // Reset current timers.
>         if(pd->rt.fv) {
> @@ -148,9 +152,8 @@
>                 pd->wt.fv = nil;
>         }
>         // Setup new timers.
> -       if(d != 0 && d <= runtime·nanotime()) {
> +       if(d != 0 && d <= runtime·nanotime())
>                 d = -1;
> -       }
>         if(mode == 'r' || mode == 'r'+'w')
>                 pd->rd = d;
>         if(mode == 'w' || mode == 'r'+'w')
> @@ -180,8 +183,17 @@
>                         runtime·addtimer(&pd->wt);
>                 }
>         }
> -ret:
> +       // If we set the new deadline in the past, unblock currently pending
> IO if any.
> +       rg = wg = nil;
> +       if(pd->rd < 0)
> +               rg = netpollunblock(pd, 'r', false);
> +       if(pd->wd < 0)
> +               wg = netpollunblock(pd, 'w', false);
>         runtime·unlock(pd);
> +       if(rg)
> +               runtime·ready(rg);
> +       if(wg)
> +               runtime·ready(wg);
>  }
>
>  func runtime_pollUnblock(pd *PollDesc) {
>
>
> --
>
> ---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.

[dvyukov, 2013-08-13 10:44:22]

On Tue, Aug 13, 2013 at 2:29 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> Dmitry,
>
> Looks like TestDeadlineRace in timeout_test.go is still broken.
> Now, and probably in Go 1.2, we will keep four network pollsters like
> the following:
>
> a) runtime-integrated network pollster on top of epoll, Linux
> b) runtime-integrated network pollster on top of IO completion mech.,
> Windows
> c) runtime-integrated network pollster on top of kqueue, FreeBSD,
> OpenBSD (I'm working on, not yet completed)
> d) package net own pollster on top of kqueue (one-shot IO bounce),
> FreeBSD/ARM, NetBSD
>
> Unfortunately, setDeadline of (d) will never return non-nil value, thanks.
> ;)
>
>

Do you mean that the goroutine that sets deadlines will never exit?
Looks like the fourth bug unveiled by the test :)

I would expect all operations on closed Conn to return errClosing.

What is we change fd_poll_unix.go from:

func setReadDeadline(fd *netFD, t time.Time) error {
    fd.pd.rdeadline.setTime(t)
    return nil
}

to something like:

func setReadDeadline(fd *netFD, t time.Time) error {
    if err := fd.incref(); err != nil {
        return err
    }
    defer fd.decref()
    fd.pd.rdeadline.setTime(t)
    fd.pd.Wakeup()
    return nil
}

?

It should solve both errClosing and updating dealines for already issued IO.

[mikio, 2013-08-13 10:58:19]

On Tue, Aug 13, 2013 at 7:44 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> Do you mean that the goroutine that sets deadlines will never exit?
> Looks like the fourth bug unveiled by the test :)

Sure, Hooray!

> I would expect all operations on closed Conn to return errClosing.
>
> What is we change fd_poll_unix.go from:
>
> func setReadDeadline(fd *netFD, t time.Time) error {
>     fd.pd.rdeadline.setTime(t)
>     return nil
> }
>
> to something like:
>
> func setReadDeadline(fd *netFD, t time.Time) error {
>     if err := fd.incref(); err != nil {
>         return err
>     }
>     defer fd.decref()
>     fd.pd.rdeadline.setTime(t)
>     fd.pd.Wakeup()
>     return nil
> }
>
> ?
>
> It should solve both errClosing and updating dealines for already issued IO.

LGTM

[dvyukov, 2013-08-13 12:07:36]

On 2013/08/13 10:58:19, mikio wrote:
> On Tue, Aug 13, 2013 at 7:44 PM, Dmitry Vyukov <mailto:dvyukov@google.com>
wrote:
> 
> > Do you mean that the goroutine that sets deadlines will never exit?
> > Looks like the fourth bug unveiled by the test :)
> 
> Sure, Hooray!
> 
> > I would expect all operations on closed Conn to return errClosing.
> >
> > What is we change fd_poll_unix.go from:
> >
> > func setReadDeadline(fd *netFD, t time.Time) error {
> >     fd.pd.rdeadline.setTime(t)
> >     return nil
> > }
> >
> > to something like:
> >
> > func setReadDeadline(fd *netFD, t time.Time) error {
> >     if err := fd.incref(); err != nil {
> >         return err
> >     }
> >     defer fd.decref()
> >     fd.pd.rdeadline.setTime(t)
> >     fd.pd.Wakeup()
> >     return nil
> > }
> >
> > ?
> >
> > It should solve both errClosing and updating dealines for already issued IO.
> 
> LGTM

Done
PTAL

[mikio, 2013-08-13 12:24:58]

thanks, test passes with below modifications.

https://codereview.appspot.com/12847043/diff/16001/src/pkg/net/fd_poll_unix.go
File src/pkg/net/fd_poll_unix.go (right):

https://codereview.appspot.com/12847043/diff/16001/src/pkg/net/fd_poll_unix.g...
src/pkg/net/fd_poll_unix.go:366: func (fd *netFD) setDeadlineImpl(fd *netFD, t
time.Time, read, write bool) error {
drop receiver

https://codereview.appspot.com/12847043/diff/16001/src/pkg/net/fd_poll_unix.g...
src/pkg/net/fd_poll_unix.go:377: fd.pd.Wakeup()
return nil

[dvyukov, 2013-08-13 13:18:28]

On 2013/08/13 12:24:58, mikio wrote:
> thanks, test passes with below modifications.
> 
> https://codereview.appspot.com/12847043/diff/16001/src/pkg/net/fd_poll_unix.go
> File src/pkg/net/fd_poll_unix.go (right):
> 
>
https://codereview.appspot.com/12847043/diff/16001/src/pkg/net/fd_poll_unix.g...
> src/pkg/net/fd_poll_unix.go:366: func (fd *netFD) setDeadlineImpl(fd *netFD, t
> time.Time, read, write bool) error {
> drop receiver
> 
>
https://codereview.appspot.com/12847043/diff/16001/src/pkg/net/fd_poll_unix.g...
> src/pkg/net/fd_poll_unix.go:377: fd.pd.Wakeup()
> return nil

Done, thanks!
PTAL

[bradfitz, 2013-08-13 15:02:33]

LGTM

https://codereview.appspot.com/12847043/diff/21001/src/pkg/net/fd_poll_unix.go
File src/pkg/net/fd_poll_unix.go (right):

https://codereview.appspot.com/12847043/diff/21001/src/pkg/net/fd_poll_unix.g...
src/pkg/net/fd_poll_unix.go:366: func setDeadlineImpl(fd *netFD, t time.Time,
read, write bool) error {
Impl?  How about just setDeadline?  If we have both func setDeadline and func
setDeadlineImpl, I'm curious what setDeadline is doing.

https://codereview.appspot.com/12847043/diff/21001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/12847043/diff/21001/src/pkg/runtime/netpoll.go...
src/pkg/runtime/netpoll.goc:187: rg = wg = nil;
is this style okay?  I don't remember seeing this elsewhere in the codebase.

[dvyukov, 2013-08-13 15:10:31]

https://codereview.appspot.com/12847043/diff/21001/src/pkg/net/fd_poll_unix.go
File src/pkg/net/fd_poll_unix.go (right):

https://codereview.appspot.com/12847043/diff/21001/src/pkg/net/fd_poll_unix.g...
src/pkg/net/fd_poll_unix.go:366: func setDeadlineImpl(fd *netFD, t time.Time,
read, write bool) error {
On 2013/08/13 15:02:34, bradfitz wrote:
> Impl?  How about just setDeadline?  If we have both func setDeadline and func
> setDeadlineImpl, I'm curious what setDeadline is doing.

setDeadline/setReadDeadline/setWriteDeadline are interface functions of netFD,
they are called from net.go.
setDeadlineImpl is netFD implementation detail.
we have the same structure in fd_poll_runtime.go

https://codereview.appspot.com/12847043/diff/21001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/12847043/diff/21001/src/pkg/runtime/netpoll.go...
src/pkg/runtime/netpoll.goc:187: rg = wg = nil;
On 2013/08/13 15:02:34, bradfitz wrote:
> is this style okay?  I don't remember seeing this elsewhere in the codebase.

I think it's used in several places, but yeah, it's more of an exception.
Done

[dvyukov, 2013-08-13 15:12:07]

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

runtime: fix handling of network deadlines
Ensure that deadlines affect already issued IO.

R=golang-dev, mikioh.mikioh, bradfitz
CC=golang-dev
https://codereview.appspot.com/12847043