code review 8966046: net: do not call syscall.Bind twice on windows (issue 8966046)

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

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
net: do not call syscall.Bind twice on windows

Fixes issue 5355.

Please review this at https://codereview.appspot.com/8966046/

Affected files:
M src/pkg/net/dial_test.go
M src/pkg/net/fd_unix.go
M src/pkg/net/fd_windows.go
M src/pkg/net/sock_posix.go


Index: src/pkg/net/dial_test.go
===================================================================
--- a/src/pkg/net/dial_test.go
+++ b/src/pkg/net/dial_test.go
@@ -372,3 +372,31 @@
}
}
}
+
+func TestDialer(t *testing.T) {
+ l, err := Listen("tcp4", "127.0.0.1:0")
+ if err != nil {
+ t.Fatalf("Listen failed: %v", err)
+ }
+ defer l.Close()
+ go func() {
+ var err error
+ c, err := l.Accept()
+ if err != nil {
+ t.Fatalf("Accept failed: %v", err)
+ }
+ defer c.Close()
+ }()
+
+ laddr, err := ResolveTCPAddr("tcp4", "127.0.0.1:0")
+ if err != nil {
+ t.Fatalf("ResolveTCPAddr failed: %v", err)
+ }
+ d := &Dialer{LocalAddr: laddr}
+ c, err := d.Dial("tcp4", l.Addr().String())
+ if err != nil {
+ t.Fatalf("Dial failed: %v", err)
+ }
+ defer c.Close()
+ c.Read(make([]byte, 1))
+}
Index: src/pkg/net/fd_unix.go
===================================================================
--- a/src/pkg/net/fd_unix.go
+++ b/src/pkg/net/fd_unix.go
@@ -79,7 +79,7 @@
return fd.net + ":" + ls + "->" + rs
}

-func (fd *netFD) connect(ra syscall.Sockaddr) error {
+func (fd *netFD) connect(la, ra syscall.Sockaddr) error {
fd.wio.Lock()
defer fd.wio.Unlock()
if err := fd.pd.PrepareWrite(); err != nil {
Index: src/pkg/net/fd_windows.go
===================================================================
--- a/src/pkg/net/fd_windows.go
+++ b/src/pkg/net/fd_windows.go
@@ -364,22 +364,23 @@
return "ConnectEx"
}

-func (fd *netFD) connect(ra syscall.Sockaddr) error {
+func (fd *netFD) connect(la, ra syscall.Sockaddr) error {
if !canUseConnectEx(fd.net) {
return syscall.Connect(fd.sysfd, ra)
}
// ConnectEx windows API requires an unconnected, previously bound socket.
- var la syscall.Sockaddr
- switch ra.(type) {
- case *syscall.SockaddrInet4:
- la = &syscall.SockaddrInet4{}
- case *syscall.SockaddrInet6:
- la = &syscall.SockaddrInet6{}
- default:
- panic("unexpected type in connect")
- }
- if err := syscall.Bind(fd.sysfd, la); err != nil {
- return err
+ if la == nil {
+ switch ra.(type) {
+ case *syscall.SockaddrInet4:
+ la = &syscall.SockaddrInet4{}
+ case *syscall.SockaddrInet6:
+ la = &syscall.SockaddrInet6{}
+ default:
+ panic("unexpected type in connect")
+ }
+ if err := syscall.Bind(fd.sysfd, la); err != nil {
+ return err
+ }
}
// Call ConnectEx API.
var o connectOp
Index: src/pkg/net/sock_posix.go
===================================================================
--- a/src/pkg/net/sock_posix.go
+++ b/src/pkg/net/sock_posix.go
@@ -57,7 +57,7 @@
if !deadline.IsZero() {
setWriteDeadline(fd, deadline)
}
- if err = fd.connect(ursa); err != nil {
+ if err = fd.connect(ulsa, ursa); err != nil {
fd.Close()
return nil, err
}

[mikioh...@gmail.com]

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go
File src/pkg/net/dial_test.go (right):

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go#newcode376
src/pkg/net/dial_test.go:376: func TestDialer(t *testing.T) {
I'd suggest to use TestTCPConnSpecificMethods instead.
But also fine making a new Dialer test case, not a Dial or DialTCP case.
In that case, pls test other control knobs in Dialer such as Deadline,
Timeout.

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go#newcode377
src/pkg/net/dial_test.go:377: l, err := Listen("tcp4", "127.0.0.1:0")
s/l/ln/

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go#newcode386
src/pkg/net/dial_test.go:386: t.Fatalf("Accept failed: %v", err)
No t.Fatalf in goroutine pls.

https://codereview.appspot.com/8966046/

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

Hello golan...@googlegroups.com, mikioh...@gmail.com (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/8966046/

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

Thank you for review.

Alex

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go
File src/pkg/net/dial_test.go (right):

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go#newcode376
src/pkg/net/dial_test.go:376: func TestDialer(t *testing.T) {

On 2013/04/27 09:19:37, mikio wrote:
> I'd suggest to use TestTCPConnSpecificMethods instead.
> But also fine making a new Dialer test case, not a Dial or DialTCP
case.
> In that case, pls test other control knobs in Dialer such as Deadline,
> Timeout.

I am fixing issue 5355. New test does enough for that. I am not sure
what other knobs you want. If you think we need more, please send a diff
and I will include it in this CL. Perhaps, send a new CL instead. I also
do not know how to use TestTCPConnSpecificMethods instead. But I will be
happy to use your patch, if you send me one.

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go#newcode377
src/pkg/net/dial_test.go:377: l, err := Listen("tcp4", "127.0.0.1:0")

On 2013/04/27 09:19:37, mikio wrote:
> s/l/ln/

Done.

https://codereview.appspot.com/8966046/diff/5001/src/pkg/net/dial_test.go#newcode386
src/pkg/net/dial_test.go:386: t.Fatalf("Accept failed: %v", err)

On 2013/04/27 09:19:37, mikio wrote:
> No t.Fatalf in goroutine pls.

Done.

https://codereview.appspot.com/8966046/

[Mikio Hara]

On Sat, Apr 27, 2013 at 8:03 PM, <alex.b...@gmail.com> wrote:

> I am fixing issue 5355. New test does enough for that.

Just clarification. Does issue 5355 require Dialer for its repro?
Or enough with DialTCP?

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

On 2013/04/27 15:09:40, mikio wrote:

> ... Does issue 5355 require Dialer for its repro?
> Or enough with DialTCP?
I suspect, DialTCP could be used to demonstrate the issue too.

Alex


https://codereview.appspot.com/8966046/

[Brad Fitzpatrick]

I think this LGTM, but is this a regression from Go 1.0?

--

---You received this message because you are subscribed to the Google Groups "golang-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

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

On 2013/04/30 17:04:43, bradfitz wrote:
> I think this LGTM, but is this a regression from Go 1.0?

I think it is a regression from Go 1.0. The bug was introduced along
with windows ConnectEx. I think we should fix it for Go 1.1. I think
https://codereview.appspot.com/8687045/ should be submitted for Go 1.1
too.

Should I wait for r or adg now?

Alex

PS: Could you also please add Luke to CONTRIBUTORS please
(https://codereview.appspot.com/8751045/#msg7). Thank you.

https://codereview.appspot.com/8966046/

[Rob Pike]

there are a lot of outstanding comments

> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to golang-dev+...@googlegroups.com.

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

On 2013/05/01 00:15:57, r wrote:
> there are a lot of outstanding comments

I do not have anything new to add. Perhaps, I missed some comments,
please ask again. I am also happy to wait for mikio.

Alex

https://codereview.appspot.com/8966046/

[Rob Pike]

Codereview was showing unanswered comments last I looked. They are cleared now.

Convince me to check this in. We don't want to break things right now,
and every time I am talked into checking something in, it breaks
something.

First, please test the current code on Windows.

-rob

[r...@golang.org]

LGTM

https://codereview.appspot.com/8966046/

[Rob Pike]

i am being arm-twisted, but i will check it in and watch the flames.

-rob

[r...@golang.org]

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

net: do not call syscall.Bind twice on windows

Fixes issue 5355.

R=golang-dev, mikioh.mikioh, bradfitz, r
CC=golang-dev
https://codereview.appspot.com/8966046

Committer: Rob Pike <r...@golang.org>


https://codereview.appspot.com/8966046/

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

On 2013/05/01 00:47:34, r wrote:
> ... i will check it in and watch the flames.


In an adventurous mood today?
That is the spirit. :-)

Alex

https://codereview.appspot.com/8966046/