code review 149510043: net: disable SIO_UDP_CONNRESET behavior on windows.

net: disable SIO_UDP_CONNRESET behavior on windows.

Fixes issue 5834.

[h2so5, 2014-10-05 06:42:18]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

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

[bradfitz, 2014-10-05 21:26:07]

R=brainman

Btw, you can't just modify ztypes_windows.go by hand. It's auto-generated.

Also, the syscall package is frozen.


On Sat, Oct 4, 2014 at 11:42 PM, <mail@h2so5.net> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc:
> golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: disable SIO_UDP_CONNRESET behavior on windows.
>
> Fixes issue 5834.
>
> Please review this at https://codereview.appspot.com/149510043/
>
> Affected files (+50, -0 lines):
>   M src/net/fd_windows.go
>   M src/net/udp_test.go
>   M src/syscall/ztypes_windows.go
>
>
> Index: src/net/fd_windows.go
> ===================================================================
> --- a/src/net/fd_windows.go
> +++ b/src/net/fd_windows.go
> @@ -294,6 +294,18 @@
>                         fd.skipSyncNotif = true
>                 }
>         }
> +       // Disable SIO_UDP_CONNRESET behavior.
> +       // http://support.microsoft.com/kb/263823
> +       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
> Index: src/net/udp_test.go
> ===================================================================
> --- a/src/net/udp_test.go
> +++ b/src/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)
> +       }
> +}
> Index: src/syscall/ztypes_windows.go
> ===================================================================
> --- a/src/syscall/ztypes_windows.go
> +++ b/src/syscall/ztypes_windows.go
> @@ -547,6 +547,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
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[brainman, 2014-10-05 22:24:08]

On 2014/10/05 21:26:07, bradfitz wrote:
> 
> ... you can't just modify ztypes_windows.go by hand. It's auto-generated.

It is not true. We do not auto-generate ztypes_windows.go file - only
zsyscall_windows.go file is auto-generated on windows.

> Also, the syscall package is frozen.

That is true:
https://groups.google.com/d/topic/golang-dev/gyOoSwxBkCA/discussion

But sometimes Rob makes exceptions. Perhaps this is the case. It is one const
change. And const relates to others around it:

> > @@ -547,6 +547,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
> >

Rob, will you allow?

R=r

If not, please, create a new internal/syscall/windows package and put new const
there.

I will review your code properly when I have some time. Thank you.

Alex

[brainman, 2014-10-07 00:02:52]

LGTM

Thank you very much for taking care of this.

R=mikio

It would be nice if someone who understand network can review too, but I will
submit it anyway after a day or so.

I cannot see your name in our CONTRIBUTORS file. Before I submit, you will have
to sign one of our license agreements here
http://golang.org/doc/contribute.html#copyright

Thank you.

Alex

[mikio, 2014-10-07 22:23:52]

i'm not keen on breaking syscall freeze.

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go
File src/net/udp_test.go (right):

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:260: func TestUDPReadFrom(t *testing.T) {
move this func just before TestWriteToUDP, also rename to TestReadFromUDP?

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:261: const raddr = "127.0.0.1:55555"
can't we use discard or echo service port?

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:262: const laddr = ":0"
":0" annoys darwin firewall stuff; change to "127.0.0.1:0"

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:266: t.Fatalf("ResolveUDPAddr failed: %v", err)
just t.Fatal(err) is enough

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:271: t.Fatalf("ResolveUDPAddr failed: %v", err)
ditto

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:276: t.Fatalf("ListenUDP failed: %v", err)
ditto

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:282: t.Fatal("WriteToUDP failed: %v", err)
ditto

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:287: t.Fatalf("SetDeadline failed: %v", err)
ditto

https://codereview.appspot.com/149510043/diff/40001/src/net/udp_test.go#newco...
src/net/udp_test.go:292: t.Fatalf("ReadFromUDP failed: %v", err)
ditto

[brainman, 2014-10-07 22:43:51]

On 2014/10/07 22:23:52, mikio wrote:
> i'm not keen on breaking syscall freeze.

Lets leave that line in syscall as is. I will move that const myself in later CL
if there are more objections.

Alex

[h2so5, 2014-10-07 23:50:46]

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

Please take another look.

[brainman, 2014-10-08 11:04:36]

h2so5,

Did you sign license agreements?

Alex

[in60jp_gmail.com, 2014-10-08 11:12:59]

On Wednesday, October 8, 2014 8:04:37 PM UTC+9, brainman wrote:
> h2so5,
> 
> 
> 
> Did you sign license agreements?
> 
> 
> 
> Alex
> 
> 
> 
> https://codereview.appspot.com/149510043/

Alex,

I signed it electronically yesterday, but I have not received any notifications.

[brainman, 2014-10-08 11:16:36]

On 2014/10/08 11:12:59, in60jp_gmail.com wrote:
> I signed it electronically yesterday, 

Thank you.

> but I have not received any notifications.

That won't be any notification for you. One of Go team member will look for your
submission and modify CONTRIBUTORS file.

Brad?

R=bradfitz

Alex

[h2so5, 2014-10-08 11:16:40]

On 2014/10/08 11:04:36, brainman wrote:
> h2so5,
> 
> Did you sign license agreements?
> 
> Alex

Alex,

I signed it electronically yesterday, but I have not received any notifications.

[brainman, 2014-10-08 11:37:09]

On 2014/10/08 11:16:40, h2so5 wrote:
> I signed it electronically yesterday,

Thank you.

> but I have not received any notifications.

You won't receive any. One of Go team members will find your submission and
change CONTTRIBUTORS file. Brad?

Alex

[mikio, 2014-10-08 12:39:19]

https://codereview.appspot.com/149510043/diff/60001/src/net/udp_test.go
File src/net/udp_test.go (right):

https://codereview.appspot.com/149510043/diff/60001/src/net/udp_test.go#newco...
src/net/udp_test.go:42: ra, err := ResolveUDPAddr("udp", raddr)
you don't need to use a constant; can just write a literal ip address.

https://codereview.appspot.com/149510043/diff/60001/src/net/udp_test.go#newco...
src/net/udp_test.go:69: if err != nil && !isTimeout(err) {
not sure whether nil is the expected correct return value. ReadFromUDP should
fail here?

[h2so5, 2014-10-08 12:54:56]

https://codereview.appspot.com/149510043/diff/60001/src/net/udp_test.go
File src/net/udp_test.go (right):

https://codereview.appspot.com/149510043/diff/60001/src/net/udp_test.go#newco...
src/net/udp_test.go:69: if err != nil && !isTimeout(err) {
On 2014/10/08 12:39:19, mikio wrote:
> not sure whether nil is the expected correct return value. ReadFromUDP should
> fail here?

Yes. ReadFromUDP should be timed out here.
With SIO_UDP_CONNRESET behavior, ReadFromUDP returns error immediately.

[h2so5, 2014-10-08 13:03:36]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org,
alex.brainman@gmail.com, mikioh.mikioh@gmail.com, in60jp@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-10-08 14:10:17]

On Wed, Oct 8, 2014 at 4:16 AM,  <alex.brainman@gmail.com> wrote:
> On 2014/10/08 11:12:59, in60jp_gmail.com wrote:
>>
>> I signed it electronically yesterday,
>
>
> Thank you.
>
>> but I have not received any notifications.
>
>
> That won't be any notification for you. One of Go team member will look
> for your submission and modify CONTRIBUTORS file.

I took care of this.

Thanks.

Ian

[mikio, 2014-10-08 21:50:19]

On Wed, Oct 8, 2014 at 9:54 PM,  <mail@h2so5.net> wrote:

>> not sure whether nil is the expected correct return value. ReadFromUDP should
fail here?
>
> Yes. ReadFromUDP should be timed out here.
> With SIO_UDP_CONNRESET behavior, ReadFromUDP returns error immediately.

then your test "if err != nil && !isTimeout(err)" doesn't look correct.

[h2so5, 2014-10-09 02:06:20]

On 2014/10/08 21:50:19, mikio wrote:
> On Wed, Oct 8, 2014 at 9:54 PM,  <mailto:mail@h2so5.net> wrote:
> 
> >> not sure whether nil is the expected correct return value. ReadFromUDP
should
> fail here?
> >
> > Yes. ReadFromUDP should be timed out here.
> > With SIO_UDP_CONNRESET behavior, ReadFromUDP returns error immediately.
> 
> then your test "if err != nil && !isTimeout(err)" doesn't look correct.

I understand. I'll fix it.
Thank you.

[h2so5, 2014-10-09 04:35:05]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org,
alex.brainman@gmail.com, mikioh.mikioh@gmail.com, in60jp@gmail.com,
iant@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[brainman, 2014-10-09 22:21:13]

On 2014/10/08 14:10:17, iant wrote:
> 
> I took care of this.
> 

Thank you.

On 2014/10/09 02:06:20, h2so5 wrote:
> > 
> > then your test "if err != nil && !isTimeout(err)" doesn't look correct.
> 
> I understand. I'll fix it.

Don't worry. I missed that too. That is why we have code reviews. Submitting
now.

Alex

[brainman, 2014-10-09 22:21:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=c7511b8a22c4 ***

net: disable SIO_UDP_CONNRESET behavior on windows.

Fixes issue 5834.

LGTM=alex.brainman
R=golang-codereviews, bradfitz, alex.brainman, mikioh.mikioh, in60jp, iant
CC=golang-codereviews
https://codereview.appspot.com/149510043

Committer: Alex Brainman <alex.brainman@gmail.com>

[gobot, 2014-10-09 22:23:40]

This CL appears to have broken the plan9-amd64-aram builder.
See http://build.golang.org/log/26e95ec7041eca12a30036316d3a765ac8465c90

[brainman, 2014-10-09 22:30:03]

On 2014/10/09 22:23:40, gobot wrote:
> This CL appears to have broken the plan9-amd64-aram builder.
> See http://build.golang.org/log/26e95ec7041eca12a30036316d3a765ac8465c90

I will disable new test on plan9.

Alex

[brainman, 2014-10-09 22:39:29]

On 2014/10/09 22:30:03, brainman wrote:
> 
> I will disable new test on plan9.
> 

The CL 157820043.

Alex