code review 6684054: net: return error from pollster rather than panicing

net: return error from pollster rather than panicing

Fixes issue 3590.

[dave_cheney.net, 2012-10-15 11:08:47]

Hello bradfitz@golang.org, mikioh.mikioh@gmail.com (cc:
golang-dev@googlegroups.com),

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

[bradfitz, 2012-10-15 12:08:12]

I believe this is Ian's code? He'd be a better reviewer than me.

In general, just send reviews to golang-dev when unsure.
 On Oct 15, 2012 4:08 AM, <dave@cheney.net> wrote:

> Reviewers: bradfitz, mikio,
>
> Message:
> Hello bradfitz@golang.org, mikioh.mikioh@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: return error from pollster rather than panicing
>
> Fixes issue 3590.
>
> Please review this at
http://codereview.appspot.com/**6684054/<http://codereview.appspot.com/6684054/>
>
> Affected files:
>   M src/pkg/net/fd_unix.go
>   A src/pkg/net/fd_unix_test.go
>
>
> Index: src/pkg/net/fd_unix.go
> ==============================**==============================**=======
> --- a/src/pkg/net/fd_unix.go
> +++ b/src/pkg/net/fd_unix.go
> @@ -97,15 +97,11 @@
>         }
>
>         wake, err := s.poll.AddFD(intfd, mode, false)
> +       s.Unlock()
>         if err != nil {
> -               panic("pollServer AddFD " + err.Error())
> +               return &OpError{"addfd", fd.net, fd.laddr, err}
>         }
> -       if wake {
> -               doWakeup = true
> -       }
> -       s.Unlock()
> -
> -       if doWakeup {
> +       if wake || doWakeup {
>                 s.Wakeup()
>         }
>         return nil
> Index: src/pkg/net/fd_unix_test.go
> ==============================**==============================**=======
> new file mode 100644
> --- /dev/null
> +++ b/src/pkg/net/fd_unix_test.go
> @@ -0,0 +1,56 @@
> +// Copyright 2012 The Go Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +// +build darwin freebsd linux netbsd openbsd
> +
> +package net
> +
> +import (
> +       "io"
> +       "testing"
> +)
> +
> +// Issue 3590. netFd.AddFD should return an error
> +// from the underlying pollster rather than panicing.
> +func TestAddFDReturnsError(t *testing.T) {
> +       l, err := Listen("tcp", "127.0.0.1:0")
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       defer l.Close()
> +
> +       go func() {
> +               for {
> +                       if _, err := l.Accept(); err != nil {
> +                               if err != io.EOF {
> +                                       t.Error(err)
> +                               }
> +                               return
> +                       }
> +               }
> +       }()
> +
> +       c, err := Dial("tcp", l.Addr().String())
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       defer c.Close()
> +
> +       // replace c's pollServer with a closed version.
> +       ps, err := newPollServer()
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       ps.poll.Close()
> +       c.(*TCPConn).conn.fd.**pollServer = ps
> +
> +       var b [1]byte
> +       _, err = c.Read(b[:])
> +       if err, ok := err.(*OpError); ok {
> +               if err.Op == "addfd" {
> +                       return
> +               }
> +       }
> +       t.Error(err)
> +}
>
>
>

[iant, 2012-10-15 16:55:10]

LGTM

[dave_cheney.net, 2012-10-15 22:30:16]

Thanks Ian. Could I ask someone from then *BSD camps to check this CL
passes on their systems please ?

On Tue, Oct 16, 2012 at 3:55 AM,  <iant@golang.org> wrote:
> LGTM
>
> https://codereview.appspot.com/6684054/

[mikio, 2012-10-16 03:17:38]

LTGM
a few nits, even if the kernel will clean the table when go test process
disappears.

https://codereview.appspot.com/6684054/diff/15001/src/pkg/net/fd_unix_test.go
File src/pkg/net/fd_unix_test.go (right):

https://codereview.appspot.com/6684054/diff/15001/src/pkg/net/fd_unix_test.go...
src/pkg/net/fd_unix_test.go:19: }
defer l.Close() to prevent fd leak?

https://codereview.appspot.com/6684054/diff/15001/src/pkg/net/fd_unix_test.go...
src/pkg/net/fd_unix_test.go:22: defer l.Close()
how to close underlying listener fd when Dial or newPollServer fails?

[mikio, 2012-10-16 03:23:12]

LGTM

> LTGM

[dave_cheney.net, 2012-10-16 05:05:24]

Hello bradfitz@golang.org, mikioh.mikioh@gmail.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-10-16 05:05:35]

Thank you for your comments. PTAL.

https://codereview.appspot.com/6684054/diff/15001/src/pkg/net/fd_unix_test.go
File src/pkg/net/fd_unix_test.go (right):

https://codereview.appspot.com/6684054/diff/15001/src/pkg/net/fd_unix_test.go...
src/pkg/net/fd_unix_test.go:19: }
On 2012/10/16 03:17:38, mikio wrote:
> defer l.Close() to prevent fd leak?

Done.

https://codereview.appspot.com/6684054/diff/15001/src/pkg/net/fd_unix_test.go...
src/pkg/net/fd_unix_test.go:22: defer l.Close()
On 2012/10/16 03:17:38, mikio wrote:
> how to close underlying listener fd when Dial or newPollServer fails?

yes, good point, fixed.

[dave_cheney.net, 2012-10-16 10:28:39]

Thank you for your comments. I am waiting on confirmation that this passes on
FreeBSD and NetBSD then I will submit.

[mikio, 2012-10-16 10:32:51]

forgot to say; it passes on freebsd/amd64,386.

[dave_cheney.net, 2012-10-16 10:35:14]

I've asked bsiegert if he can check NetBSD.

On Tue, Oct 16, 2012 at 9:32 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> forgot to say; it passes on freebsd/amd64,386.

[bsiegert, 2012-10-16 13:01:48]

On Tue, Oct 16, 2012 at 12:35 PM, Dave Cheney <dave@cheney.net> wrote:
> I've asked bsiegert if he can check NetBSD.

Passes on netbsd both on amd64 and 386.

However, when run with "go test -v", there is a warning:

=== RUN TestAddFDReturnsError
pollServer WaitFD: kevent: bad file descriptor
--- PASS: TestAddFDReturnsError (0.00 seconds)

Is this harmless?

--Benny.

-- 
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)

[dave_cheney.net, 2012-10-16 13:04:17]

Yes, this is the same as Linux.

As I am calling Read on a pollster that has been deliberately closed, some
complaining is expected, but harmless.

On 17 Oct 2012 00:01, "Benny Siegert" <bsiegert@gmail.com> wrote:
>
> On Tue, Oct 16, 2012 at 12:35 PM, Dave Cheney <dave@cheney.net> wrote:
> > I've asked bsiegert if he can check NetBSD.
>
> Passes on netbsd both on amd64 and 386.
>
> However, when run with "go test -v", there is a warning:
>
> === RUN TestAddFDReturnsError
> pollServer WaitFD: kevent: bad file descriptor
> --- PASS: TestAddFDReturnsError (0.00 seconds)
>
> Is this harmless?
>
> --Benny.
>
> --
> 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)

[bsiegert, 2012-10-16 13:04:51]

LGTM then.

[dave_cheney.net, 2012-10-16 22:41:25]

*** Submitted as http://code.google.com/p/go/source/detail?r=7fe634e0f93e ***

net: return error from pollster rather than panicing

Fixes issue 3590.

R=bradfitz, mikioh.mikioh, iant, bsiegert
CC=golang-dev
http://codereview.appspot.com/6684054