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

net: do not call syscall.Bind twice on windows

Fixes issue 5355.

[brainman, 2013-04-27 06:57:49]

Hello golang-dev@googlegroups.com,

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

[mikio, 2013-04-27 09:19:37]

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#new...
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#new...
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#new...
src/pkg/net/dial_test.go:386: t.Fatalf("Accept failed: %v", err)
No t.Fatalf in goroutine pls.

[brainman, 2013-04-27 11:03:27]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-04-27 11:03:36]

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

[mikio, 2013-04-27 15:09:40]

On Sat, Apr 27, 2013 at 8:03 PM,  <alex.brainman@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?

[brainman, 2013-04-27 23:46:07]

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

[bradfitz, 2013-04-30 17:04:43]

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




On Sat, Apr 27, 2013 at 4:46 PM, <alex.brainman@gmail.com> wrote:

> 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/<https://codereview.appspot.com/8966...
>
> --
>
> ---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<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[brainman, 2013-05-01 00:12:59]

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.

[r, 2013-05-01 00:15:57]

there are a lot of outstanding comments

On Tue, Apr 30, 2013 at 5:12 PM,  <alex.brainman@gmail.com> wrote:
> 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/
>
> --
>
> ---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.
>
>

[brainman, 2013-05-01 00:20:46]

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

[r, 2013-05-01 00:41:47]

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, 2013-05-01 00:47:21]

LGTM

[r, 2013-05-01 00:47:34]

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

-rob


On Tue, Apr 30, 2013 at 5:41 PM, Rob Pike <r@golang.org> wrote:
> 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, 2013-05-01 00:47:41]

*** 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>

[brainman, 2013-05-01 00:52:14]

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