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

[mikioh...@gmail.com]

Reviewers: golang-dev1, brainman,

Message:
Hello golan...@googlegroups.com, alex.b...@gmail.com (cc:
golan...@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)
+ }
+ }
+}

[Mikio Hara]

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

[alex.b...@gmail.com]

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

Alex

https://codereview.appspot.com/8687045/

[brad...@golang.org]

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#newcode708
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#newcode708
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#newcode710
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#newcode717
src/pkg/net/timeout_test.go:717: case <-time.After(2 * time.Second):
make this 5 seconds.

https://codereview.appspot.com/8687045/

[alex.b...@gmail.com]

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#newcode18
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.

https://codereview.appspot.com/8687045/