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: UDPConn.ReadFrom failure on Windows #5834

Closed
gopherbot opened this issue Jul 3, 2013 · 23 comments

Comments

Projects
None yet
3 participants
@gopherbot
Copy link

commented Jul 3, 2013

by junshi04:

The bug description is below. The program to reproduce the bug is attached.

On Windows, net.UDPConn.ReadFrom may return an error of "WSARecvFrom
...: No service is operating at the destination network endpoint on
the remote system", when the same UDPConn is used to write a packet
that has no recipient.

To reproduce the error, set "-me" to the local IP:port, "-peer" to a
random IP:port, and run the attached file. The ReadFrom will fail
immediately with the "No service is operating ..." error.

If "-peer" is not set, the net.UDPConn is not used to write packets,
and the ReadFrom won't fail.

If "-peer" is set to an address with a recipient, the ReadFrom won't
fail either.

Looks like the error is actually for the write, writing to the peer
IP:port generates an ICMP error, but the error is returned to the
ReadFrom.

Attachments:

  1. readfrombug.go (916 bytes)
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jul 5, 2013

Comment 1:

Your program kind of works for me, but there are many unclear points. Perhaps it is best
that you change your program so that is simpler and does not require any parameters or
typing and outputs some data. Then you could tell us what your program outputs and what
you think its output should be instead. Just like the original issue template was:
What steps will reproduce the problem?
1.
2.
3.
What is the expected output? What do you see instead?
Please use labels and text to provide additional information.
Thank you.
Alex

Status changed to WaitingForReply.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Jul 6, 2013

Comment 2 by junshi04:

I simplified the program. It fails on 64-bit windows (windows 7,
windows server 2008). The ReadFrom returns an error "No service is
operating at the destination network endpoint on the remote system.".
I believe the error is actually for the WriteTo call, as there is no
recipient on the remote address. I think the error is triggered by an
ICMP message from the peer, but I did not capture the packet to
confirm it.
I have seen the same error on 32-bit windows machines occasionally,
but could not find a reliable way to trigger it.

Attachments:

  1. readfrombug2.go (353 bytes)
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jul 8, 2013

Comment 3:

I don't know much about UDP, but it looks to me that is how Windows handles this
situation.
http://www.gamedev.net/topic/540870-udp-iocp-and-error_port_unreachablewsaeconnreset/
https://groups.google.com/forum/#!topic/microsoft.public.win32.programmer.networks/tTOuZ0a4hDs
http://www.lenholgate.com/blog/2007/12/bug-in-overlapped-udp-port-unreachable-errors.html
What do you propose we should do?
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

commented Jul 8, 2013

Comment 4 by junshi04:

Thanks for these pointers! It is a Windows quirk.
http://support.microsoft.com/kb/263823 has the details. Though it is
still not clear to me why this shows up reliably only on 64-bit
Windows.
Right now, I have to ignore the error of ReadFrom and retry on
Windows. I cannot think of any other reasonable ways to handle this
kind of error.
But the problem of this ignore-and-retry approach is I have to
distinguish this error from other persistent errors such as reading
from closed descriptor by checking the actual error message. That's
ugly!
ReadFrom on Linux does not have this behavior. Does it make sense to
you to export SIO_UDP_CONNRESET in syscall, so that I can call
WSAIoctl to disable this behavior?
Another option is to make this error temporary, i.e. err.Temporary()
== true, so that I can ignore this error in a cheap and safe way.
Thanks,
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jul 8, 2013

Comment 5:

Like I said earlier, I am not an expert in net, so I hope others will help.
Alternatively, feel free to discuss your proposal on golang-nuts newsgroup. If that
won't help, just send a proposed change. I am happy to help with your change once
everyone agree on a direction.
Alex
@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 6:

Labels changed: added priority-later, go1.2, expertneeded, suggested, removed priority-triage.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2013

Comment 7:

It looks like the fix is to retry ReadFrom when this error comes back.
Does anyone have time to write a test to go with https://golang.org/cl/13703043?
@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2013

Comment 8:

I'd like to see that CL turn into a tested fix in some form (and perhaps the loop needs
to be lower down to handle other kinds of reads?), but we won't hold up Go 1.2 for it.

Labels changed: added go1.2maybe, removed go1.2.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Sep 20, 2013

Comment 9:

junshi04,
Sorry, but I am still not clear about what we are fixing here. I wrote this test:
diff --git a/src/pkg/net/udp_test.go b/src/pkg/net/udp_test.go
--- a/src/pkg/net/udp_test.go
+++ b/src/pkg/net/udp_test.go
@@ -9,6 +9,7 @@
    "reflect"
    "runtime"
    "testing"
+   "time"
 )
 
 type resolveUDPAddrTest struct {
@@ -280,3 +281,35 @@
        }
    }
 }
+
+func TestUDPReadFrom(t *testing.T) {
+   const raddr = "54.244.132.193:55555"
+   c, err := Dial("udp", raddr)
+   if err != nil {
+       t.Fatalf("Dial failed: %v", err)
+   }
+   defer c.Close()
+
+   ra, err := ResolveUDPAddr("udp", raddr)
+   if err != nil {
+       t.Fatalf("ResolveUDPAddr failed: %v", err)
+   }
+
+   _, err = c.(*UDPConn).WriteToUDP([]byte("a"), ra)
+   if err == nil {
+       t.Fatal("WriteToUDP should fail")
+   }
+   if err != nil && err.(*OpError).Err != ErrWriteToConnected {
+       t.Fatalf("WriteToUDP should fail as ErrWriteToConnected: %v", err)
+   }
+
+   err = c.SetDeadline(time.Now().Add(100 * time.Millisecond))
+   if err != nil {
+       t.Fatalf("SetDeadline failed: %v", err)
+   }
+   b := make([]byte, 1)
+   _, _, err = c.(*UDPConn).ReadFromUDP(b)
+   if err != nil && !isTimeout(err) {
+       t.Fatalf("ReadFromUDP failed: %v", err)
+   }
+}
and it PASSes on both linux and windows here. Please, change the test, so it PASSes on
one and FAILs on the other. This way we can narrow it down where the differences are and
decide what to do about them.
Thank you.
Alex

Status changed to WaitingForReply.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Sep 20, 2013

Comment 10 by junshi04:

You should be able to see the error, if you run the program attached
in #2, readfrombug2.go, on 64-bit windows. The program may not fail on
a 32-bit windows.
I used net.ListenUDP, not net.Dial. So the WriteToUDP won't fail in my
program. I don't think you should use WriteToUDP. The write would fail
locally with ErrWriteToConnected. But we want the UDP packet to be
sent to the remote host to trigger an ICMP reset packet. It is the
ICMP reset packet that causes the following ReadFromUDP to fail.
Thanks,
@gopherbot

This comment has been minimized.

Copy link
Author

commented Sep 21, 2013

Comment 11 by junshi04:

As to the fix, I think a better fix is to disable the
SIO_UDP_CONNRESET behavior, to avoid the bogus read errors.
Thanks,
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Sep 22, 2013

Comment 12:

I run my new TestUDPReadFrom on both windows/386 and windows/amd64. It PASSes on both.
If you want me to look further, please change my test as requested in
https://golang.org/issue/5834?c=9. I don't know enough about Go udp
code, but why cannot you do what my test does?
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

commented Sep 23, 2013

Comment 13 by junshi04:

I don't know how to run your test. It is part of the standard "net" package.
As I pointed out in my early message, your test should not use both
net.Dial and WriteToUDP. The WriteToUDP would fail locally, because
you cannot use WriteToUDP on a connected UDP socket. To reproduce the
fail I reported, the UDP packet should be sent to the remote host to
trigger an ICMP reset packet.
You need to make these changes to your test,
- replace Dial with ListenUDP
- check that WriteToUDP does not fail
- the raddr "54.244.132.193" is my EC2 instance. It is up at this
point, but you'd better replace it with a well-known host that
generates ICMP reset packets (www.google.com does not send ICMP reset
packets afaik).
Thanks,
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Sep 24, 2013

Comment 14:

Like I said earlier, I don't know enough about Go udp. So I need a test that I can use
to develop solution. Please, try again to modify my test, as I have requested.
Here is what I do to modify udp_test.go file to add new test and run it:
C:\>cd %GOROOT%\src\pkg\net
C:\go\root\src\pkg\net>hg st
C:\go\root\src\pkg\net>hg patch -f --no-commit u:\a.diff
applying u:\a.diff
C:\go\root\src\pkg\net>hg st
M src\pkg\net\udp_test.go
C:\go\root\src\pkg\net>go test -v -run=TestUDPReadFrom
=== RUN TestUDPReadFrom
--- PASS: TestUDPReadFrom (0.11 seconds)
PASS
ok      net     0.203s
C:\go\root\src\pkg\net>
a.diff file attached. Please modify the test to demonstrate the issue (it must PASS on
linux and FAIL on windows or vice versa), and post your changes here (output of "hg
diff" command should do).
Thank you.
Alex

Attachments:

  1. a.diff (1092 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2013

Comment 15:

Labels changed: added go1.3, removed go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 16:

Labels changed: added release-go1.3, removed go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 17:

Labels changed: added repo-main.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 9, 2014

Comment 18:

Labels changed: added release-none, removed release-go1.3.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Sep 29, 2014

Comment 19 by in60jp:

Alex,
This test will demonstrate the error:
diff --git a/src/pkg/net/udp_test.go b/src/pkg/net/udp_test.go
--- a/src/pkg/net/udp_test.go
+++ b/src/pkg/net/udp_test.go
@@ -9,6 +9,7 @@
    "runtime"
    "strings"
    "testing"
+   "time"
 )
 
 func TestResolveUDPAddr(t *testing.T) {
@@ -255,3 +256,39 @@
        }
    }
 }
+
+func TestUDPReadFrom(t *testing.T) {
+   const raddr = "127.0.0.1:55555"
+   const laddr = ":0"
+
+   ra, err := ResolveUDPAddr("udp", raddr)
+   if err != nil {
+       t.Fatalf("ResolveUDPAddr failed: %v", err)
+   }
+
+   la, err := ResolveUDPAddr("udp", laddr)
+   if err != nil {
+       t.Fatalf("ResolveUDPAddr failed: %v", err)
+   }
+
+   c, err := ListenUDP("udp", la)
+   if err != nil {
+       t.Fatalf("ListenUDP failed: %v", err)
+   }
+   defer c.Close()
+
+   _, err = c.WriteToUDP([]byte("a"), ra)
+   if err != nil {
+       t.Fatal("WriteToUDP failed: %v", err)
+   }
+
+   err = c.SetDeadline(time.Now().Add(100 * time.Millisecond))
+   if err != nil {
+       t.Fatalf("SetDeadline failed: %v", err)
+   }
+   b := make([]byte, 1)
+   _, _, err = c.ReadFromUDP(b)
+   if err != nil && !isTimeout(err) {
+       t.Fatalf("ReadFromUDP failed: %v", err)
+   }
+}
and I wrote a patch to fix this issue:
diff --git a/src/pkg/net/fd_windows.go b/src/pkg/net/fd_windows.go
--- a/src/pkg/net/fd_windows.go
+++ b/src/pkg/net/fd_windows.go
@@ -294,6 +294,18 @@
            fd.skipSyncNotif = true
        }
    }
+   // Disable the SIO_UDP_CONNRESET behavior.
+   // https://golang.org/issue/5834
+   switch fd.net {
+   case "udp", "udp4", "udp6":
+       ret := uint32(0)
+       flag := uint32(0)
+       size := uint32(unsafe.Sizeof(flag))
+       err := syscall.WSAIoctl(fd.sysfd, syscall.SIO_UDP_CONNRESET,
(*byte)(unsafe.Pointer(&flag)), size, nil, 0, &ret, nil, 0)
+       if err != nil {
+           return os.NewSyscallError("WSAIoctl", err)
+       }
+   }
    fd.rop.mode = 'r'
    fd.wop.mode = 'w'
    fd.rop.fd = fd
diff --git a/src/pkg/syscall/ztypes_windows.go b/src/pkg/syscall/ztypes_windows.go
--- a/src/pkg/syscall/ztypes_windows.go
+++ b/src/pkg/syscall/ztypes_windows.go
@@ -520,6 +520,7 @@
    IOC_WS2                            = 0x08000000
    SIO_GET_EXTENSION_FUNCTION_POINTER = IOC_INOUT | IOC_WS2 | 6
    SIO_KEEPALIVE_VALS                 = IOC_IN | IOC_VENDOR | 4
+   SIO_UDP_CONNRESET                  = IOC_IN | IOC_VENDOR | 12
 
    // cf. http://support.microsoft.com/default.aspx?scid=kb;en-us;257460
 
Thanks.
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Oct 3, 2014

Comment 20:

in60jp,
Sounds reasonable to me. But we cannot accept contributions as patches.
http://golang.org/doc/contribute.html shows how to contribute. Would you like to try?
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 3, 2014

Comment 21 by in60jp:

Alex,
OK. I'm going to read Contribution Guidelines.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 5, 2014

Comment 22:

CL https://golang.org/cl/149510043 mentions this issue.
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Oct 9, 2014

Comment 23:

This issue was closed by revision 3114bd6.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 24, 2016

wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018

net: disable SIO_UDP_CONNRESET behavior on windows.
Fixes golang#5834.

LGTM=alex.brainman
R=golang-codereviews, bradfitz, alex.brainman, mikioh.mikioh, in60jp, iant
CC=golang-codereviews
https://golang.org/cl/149510043

wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018

net: disable SIO_UDP_CONNRESET behavior on windows.
Fixes golang#5834.

LGTM=alex.brainman
R=golang-codereviews, bradfitz, alex.brainman, mikioh.mikioh, in60jp, iant
CC=golang-codereviews
https://golang.org/cl/149510043

wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018

net: disable SIO_UDP_CONNRESET behavior on windows.
Fixes golang#5834.

LGTM=alex.brainman
R=golang-codereviews, bradfitz, alex.brainman, mikioh.mikioh, in60jp, iant
CC=golang-codereviews
https://golang.org/cl/149510043

wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018

net: disable SIO_UDP_CONNRESET behavior on windows.
Fixes golang#5834.

LGTM=alex.brainman
R=golang-codereviews, bradfitz, alex.brainman, mikioh.mikioh, in60jp, iant
CC=golang-codereviews
https://golang.org/cl/149510043

This issue was closed.

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.