code review 8687045: net: fix dial datagram on windows, plan 9

net: fix dial datagram on windows, plan 9

Fixes issue 5349.

[mikio, 2013-04-25 05:49:42]

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

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

[mikio, 2013-04-25 05:59:51]

Sorry I have no windows stuff, I mean, not tested on windows.

On Thu, Apr 25, 2013 at 2:49 PM,  <mikioh.mikioh@gmail.com> wrote:
> Reviewers: golang-dev1, brainman,
>
> Message:
> Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: fix dial datagram on windows, plan 9
>
> Fixes issue 5349.
>
> Please review this at https://codereview.appspot.com/8687045/
>
> Affected files:
>   M src/pkg/net/dial_gen.go
>   M src/pkg/net/timeout_test.go
>
>
> Index: src/pkg/net/dial_gen.go
> ===================================================================
> --- a/src/pkg/net/dial_gen.go
> +++ b/src/pkg/net/dial_gen.go
> @@ -15,6 +15,13 @@
>  // hasn't been pushed down into the pollserver. (Plan 9 and some old
>  // versions of Windows)
>  func resolveAndDialChannel(net, addr string, localAddr Addr, deadline
> time.Time) (Conn, error) {
> +       if deadline.IsZero() {
> +               ra, err := resolveAddr("dial", net, addr, noDeadline)
> +               if err != nil {
> +                       return nil, err
> +               }
> +               return dial(net, addr, localAddr, ra, noDeadline)
> +       }
>         timeout := deadline.Sub(time.Now())
>         if timeout < 0 {
>                 timeout = 0
> Index: src/pkg/net/timeout_test.go
> ===================================================================
> --- a/src/pkg/net/timeout_test.go
> +++ b/src/pkg/net/timeout_test.go
> @@ -703,3 +703,22 @@
>                 c.Write(buf[:])
>         }
>  }
> +
> +func TestDialDatagramWithNoDeadline(t *testing.T) {
> +       ch := make(chan copyRes)
> +       go func() {
> +               c, err := Dial("udp4", "127.0.0.1:514")
> +               if err == nil {
> +                       c.Close()
> +               }
> +               ch <- copyRes{err: err}
> +       }()
> +       select {
> +       case <-time.After(2 * time.Second):
> +               t.Fatal("too slow")
> +       case res := <-ch:
> +               if res.err != nil {
> +                       t.Fatalf("Dial failed: %v", res.err)
> +               }
> +       }
> +}
>
>

[brainman, 2013-04-25 06:39:28]

Thank you for fixing this. Let me think about it for a day or two.

Alex

[bradfitz, 2013-04-25 06:42:23]

Thanks for fixing this.

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:708: ch := make(chan copyRes)
make this buffered with size 1.  it's just a test, but a good habit to get into
when the sender goroutine could otherwise leak.

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:708: ch := make(chan copyRes)
why not just a chan error?

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:710: c, err := Dial("udp4", "127.0.0.1:514")
why 514?

will this be reliable?

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:717: case <-time.After(2 * time.Second):
make this 5 seconds.

[brainman, 2013-04-26 01:44:13]

mikio,

I do not think we need new test. We have plenty of UDP tests. They just do not
trigger that error. But adding small sleep in dial_gen.go, makes them break
quickly:

C:\go\root\src\pkg\net>hg diff
diff -r 45c12efb4635 src/pkg/net/dial_gen.go
--- a/src/pkg/net/dial_gen.go   Thu Apr 25 14:45:56 2013 -0700
+++ b/src/pkg/net/dial_gen.go   Fri Apr 26 11:33:20 2013 +1000
@@ -28,6 +28,7 @@
        ch := make(chan pair, 1)
        resolvedAddr := make(chan Addr, 1)
        go func() {
+               time.Sleep(time.Millisecond)
                ra, err := resolveAddr("dial", net, addr, noDeadline)
                if err != nil {
                        ch <- pair{nil, err}

C:\go\root\src\pkg\net>
C:\go\root\src\pkg\net>go test -short
--- FAIL: TestConnAndPacketConn (0.00 seconds)
        packetconn_test.go:175: Dial failed: dial udp 127.0.0.1:2170: i/o
timeout
--- FAIL: TestTimeoutUDP (0.02 seconds)
        timeout_test.go:193: Dial("udp", "127.0.0.1:2187") failed: dial udp
127.0.0.1:2187: i/o timeout
        timeout_test.go:193: Dial("udp", "127.0.0.1:2187") failed: dial udp
127.0.0.1:2187: i/o timeout
--- FAIL: TestWriteToUDP (0.00 seconds)
        udp_test.go:78: Dial failed: dial udp 127.0.0.1:2237: i/o timeout
FAIL
exit status 1
FAIL    net     2.094s

C:\go\root\src\pkg\net>

It is not good, that we cannot trigger the error, but, I think, your new test is
no better in that regard. Perhaps, you can introduce that time.Sleep
artificially when you run the tests.

Alex

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/dial_gen.go
File src/pkg/net/dial_gen.go (right):

https://codereview.appspot.com/8687045/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:18: if deadline.IsZero() {
If you are creating this code path, that does not start new goroutine if
deadline.IsZero(), shouldn't you add following case of expired deadline too. No
big deal, but it looks more consistent to me.

[nkatsaros, 2013-04-26 15:18:41]

Thanks for fixing this mikio.

The description is slightly inaccurate though. I think dial is broken for 
all network types on plan9, not just datagrams.

[brainman, 2013-05-03 05:51:58]

I think it is silly to leave this open for go1.1, so I copied mikio fix into my
own: https://codereview.appspot.com/9159043/. Lets submit either one.

Alex

[mikio, 2013-05-08 10:24:42]

*** Abandoned ***