Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: TestDialTimeout failing on Plan 9 #15333

Closed
0intro opened this issue Apr 16, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@0intro
Copy link
Member

commented Apr 16, 2016

Since CL 22101, TestDialTimeout is failing on Plan 9.

--- FAIL: TestDialTimeout (0.00s)
    timeout_test.go:99: #5: dial tcp 127.0.0.1:0: write /net/tcp/clone: connection refused
FAIL
FAIL    net 1.007s

See http://build.golang.org/log/eb55b9009e42158295e02337ffa9b5ba1882a40b

The three last tests, which set a positive Dialer.Timeout or Dialer.Deadline, are no longer returning errTimeout.

=== RUN   TestDialTimeout
#0: dial tcp 127.0.0.1:0: i/o timeout
#1: dial tcp 127.0.0.1:0: i/o timeout
#2: dial tcp 127.0.0.1:0: i/o timeout
#3: dial tcp 127.0.0.1:0: i/o timeout
#4: dial tcp 127.0.0.1:0: i/o timeout
#5: dial tcp 127.0.0.1:0: write /net/tcp/clone: connection refused
#6: dial tcp 127.0.0.1:0: write /net/tcp/clone: connection refused
#7: dial tcp 127.0.0.1:0: write /net/tcp/clone: connection refused

@0intro 0intro added the OS-Plan9 label Apr 16, 2016

@0intro 0intro added this to the Go1.7 milestone Apr 16, 2016

@0intro

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

The expected timeout comes from dialChannel.
The dialChannel function should probably be modified to use context.Context instead of a deadline.

@0intro

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

CC: @bradfitz

@0intro

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

The problem is that the os-dependent dial function isn't called anymore from dialSerial.

@0intro

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

I propose CL 22143 to fix this issue, but I'm not really sure about the interaction with context.Context.
Please take a look.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 16, 2016

CL https://golang.org/cl/22143 mentions this issue.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 17, 2016

I don't see this failure on my Windows XP pc.

Alex

@gopherbot

This comment has been minimized.

Copy link

commented Apr 17, 2016

CL https://golang.org/cl/22144 mentions this issue.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 17, 2016

I think Windows XP (any windows) is not currently broken, because plan9 uses different code.

Alex

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 17, 2016

@alexbrainman, according to https://msdn.microsoft.com/en-us/library/windows/desktop/ms737606(v=vs.85).aspx ConnectEx isn't available on Windows XP, yet my https://golang.org/cl/22101 deleted 1 of the 2 paths that checked whether ConnectEx was available. It seemed likely to break XP, or at least make XP go into the wrong code path. I think it just disabled the backup racer dial on XP, meaning timeouts wouldn't be reliable?

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 17, 2016

ConnectEx does work on my Windows XP:

C:\dev\go\src\net>git rev-parse HEAD
04535459caf349bc5a731133f5e9eef5e1ab9af9

C:\dev\go\src\net>git diff
diff --git a/src/net/fd_windows.go b/src/net/fd_windows.go
index d1d91a6..c776dfd 100644
--- a/src/net/fd_windows.go
+++ b/src/net/fd_windows.go
@@ -70,7 +70,10 @@ func sysInit() {
    }
 }

-func canUseConnectEx(net string) bool {
+func canUseConnectEx(net string) (ret bool) {
+   defer func() {
+       println("canUseConnectEx: net=", net, " returns=", ret)
+   }()
    switch net {
    case "udp", "udp4", "udp6", "ip", "ip4", "ip6":
        // ConnectEx windows API does not support connectionless sockets.

C:\dev\go\src\net>go test -v -run=TestDialTimeout$
=== RUN   TestDialTimeout
canUseConnectEx: net= tcp  returns= true
canUseConnectEx: net= tcp  returns= true
canUseConnectEx: net= tcp  returns= true
--- PASS: TestDialTimeout (0.33s)
PASS
Running goroutines:
net.(*ioSrv).ProcessRemoteIO(0x10d70428)
        c:/dev/go/src/net/fd_windows.go:143 +0xcc
created by net.startServer
        c:/dev/go/src/net/fd_windows.go:243 +0xc9
net.(*ioSrv).ProcessRemoteIO(0x10d70430)
        c:/dev/go/src/net/fd_windows.go:143 +0xcc
created by net.startServer
        c:/dev/go/src/net/fd_windows.go:245 +0x11e

Socket statistical information:
(inet4, stream, default): opened=3 connected=0 listened=0 accepted=0 closed=3 openfailed=0 connectfailed=0 listenfailed=0 acceptfailed=0 closefailed=0

ok      net     0.922s

C:\dev\go\src\net>

Alex

@0intro

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

@alexbrainman, according to https://msdn.microsoft.com/en-us/library/windows/desktop/ms737606(v=vs.85).aspx ConnectEx isn't available on Windows XP

I think there is an error in this documentation. As far I understand it, ConnectEx is available since Windows XP. This specific case in Go was probably written for Windows 2000.

Amusingly, the German version of same page have the accurate version (appeared in Windows XP):

https://msdn.microsoft.com/de-de/library/windows/desktop/ms737606(v=vs.85).aspx

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

I agree with what @0intro said. I think non-ConnectEx code is actually for w2k. We don't support w2k, but Go runs on w2k just fine. So I think we should just let that code be as is.

Alex

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

If we don't support Go on Windows 2000, don't say we support it on the downloads page, don't have a builder for it, and don't have any developer with a Windows 2000 machine that regularly tests and reports its status, I am going to delete the non-ConnectEx code path from the net package and simplify things.

Sorry, that's way too old.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

I am going to delete the non-ConnectEx code path from the net package and simplify things.

Sure. If you think this is worth breaking someone using (even unsupported) version of Go.

Alex

PS: I think you should be able to remove oldLookupPort and oldLookupIP too. Would you like me to send the change and test it here on my Windows XP?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

PS: I think you should be able to remove oldLookupPort and oldLookupIP too. Would you like me to send the change and test it here on my Windows XP?

Yes, please. I have an XP install CD and license but I have not yet installed it, and my XP install CD is currently in a different city.

@gopherbot gopherbot closed this in f60fcca Apr 18, 2016

@golang golang locked and limited conversation to collaborators Apr 18, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.