code review 9159043: net: fix dial race on plan9 and windows

net: fix dial race on plan9 and windows

Fixes issue 5349.

[brainman, 2013-05-03 05:49:04]

Hello golang-dev@googlegroups.com,

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

[lucio, 2013-05-03 09:51:13]

Won't please you make sure the description contains "plan 9" rather
than just "plan"?

Lucio.


On 5/3/13, alex.brainman@gmail.com <alex.brainman@gmail.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net: fix dail race on plan and windows
>
> Fixes issue 5349.
>
> Please review this at https://codereview.appspot.com/9159043/
>
> Affected files:
>    M src/pkg/net/dial_gen.go
>    A src/pkg/net/dial_gen_test.go
>
>
> Index: src/pkg/net/dial_gen.go
> ===================================================================
> --- a/src/pkg/net/dial_gen.go
> +++ b/src/pkg/net/dial_gen.go
> @@ -10,14 +10,23 @@
>   	"time"
>   )
>
> +var testingIssue5349 bool // used during tests
> +
>   // resolveAndDialChannel is the simple pure-Go implementation of
>   // resolveAndDial, still used on operating systems where the deadline
>   // hasn't been pushed down into the pollserver. (Plan 9 and some old
>   // versions of Windows)
>   func resolveAndDialChannel(net, addr string, localAddr Addr, deadline
> time.Time) (Conn, error) {
> -	timeout := deadline.Sub(time.Now())
> -	if timeout < 0 {
> -		timeout = 0
> +	var timeout time.Duration
> +	if !deadline.IsZero() {
> +		timeout = deadline.Sub(time.Now())
> +	}
> +	if timeout <= 0 {
> +		ra, err := resolveAddr("dial", net, addr, noDeadline)
> +		if err != nil {
> +			return nil, err
> +		}
> +		return dial(net, addr, localAddr, ra, noDeadline)
>   	}
>   	t := time.NewTimer(timeout)
>   	defer t.Stop()
> @@ -28,6 +37,9 @@
>   	ch := make(chan pair, 1)
>   	resolvedAddr := make(chan Addr, 1)
>   	go func() {
> +		if testingIssue5349 {
> +			time.Sleep(time.Millisecond)
> +		}
>   		ra, err := resolveAddr("dial", net, addr, noDeadline)
>   		if err != nil {
>   			ch <- pair{nil, err}
> Index: src/pkg/net/dial_gen_test.go
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/src/pkg/net/dial_gen_test.go
> @@ -0,0 +1,11 @@
> +// Copyright 2013 The Go Authors.  All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +// +build windows plan9
> +
> +package net
> +
> +func init() {
> +	testingIssue5349 = true
> +}
>
>
> --
>
> ---
> 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-03 12:53:57]

On 2013/05/03 09:51:13, lucio wrote:
> Won't please you make sure the description contains "plan 9" rather
> than just "plan"?
> 

Fixed. Too much beer :-)

Alex

[dsymonds, 2013-05-07 18:04:03]

maybe fix the spelling of "dial" in the CL description too.

[bradfitz, 2013-05-07 18:26:53]

LGTM

[iant, 2013-05-07 20:17:59]

LGTM

[brainman, 2013-05-07 23:51:00]

On 2013/05/07 18:04:03, dsymonds wrote:
> maybe fix the spelling of "dial" in the CL description too.

Done. I will wait for adg or r now.

Alex

[bradfitz, 2013-05-08 00:00:30]

Please reupload.  Rietveld is confused:
https://codereview.appspot.com/9159043/diff/17001/src/pkg/net/dial_gen.go

I think adg & r want rc2 to be the final version, unless there are critical
problems.  This can go in if other things are going in.

Nobody spoke up about this bug when we asked what which outstanding bugs
were remaining.

How would you classify the severity of this bug? Is all UDP dialing broken
on Windows currently?  Or just sometimes?





On Tue, May 7, 2013 at 4:51 PM, <alex.brainman@gmail.com> wrote:

> On 2013/05/07 18:04:03, dsymonds wrote:
>
>> maybe fix the spelling of "dial" in the CL description too.
>>
>
> Done. I will wait for adg or r now.
>
> Alex
>
>
https://codereview.appspot.**com/9159043/<https://codereview.appspot.com/9159...
>

[adg, 2013-05-08 00:02:01]

On 7 May 2013 17:00, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> Nobody spoke up about this bug when we asked what which outstanding bugs
> were remaining.
>

Actually, Alex did speak up, but only after we had tagged rc2.

[brainman, 2013-05-08 00:10:51]

On 2013/05/08 00:00:30, bradfitz wrote:
> Please reupload.  Rietveld is confused:
> https://codereview.appspot.com/9159043/diff/17001/src/pkg/net/dial_gen.go

I just did "hg upload 9159043" before I replied to dsymonds. Do you want me to
run the command again?

> I think adg & r want rc2 to be the final version, unless there are critical
> problems.  This can go in if other things are going in.

Fair enough.

> How would you classify the severity of this bug? ...

I think it is serious. It is a race, so it makes it is so much harder to deal
with. Here it is and here it is gone.

> ... Is all UDP dialing broken
> on Windows currently? 

I think so. And some TCP systems too.

> ... Or just sometimes?

Sometimes, for sure. It works most of the time, but sometimes it breaks, because
it is a race.

On 2013/05/08 00:02:01, adg wrote:
> 
> > Nobody spoke up about this bug when we asked what which outstanding bugs
> > were remaining.
> >
> 
> Actually, Alex did speak up, but only after we had tagged rc2.

Yes, I am a slow one in my family :-)

Alex

[dave_cheney.net, 2013-05-08 00:13:57]

Yes, if you view the CL, the most current revision is broken because
of a chunk mismatch.

On Wed, May 8, 2013 at 10:10 AM,  <alex.brainman@gmail.com> wrote:
> On 2013/05/08 00:00:30, bradfitz wrote:
>>
>> Please reupload.  Rietveld is confused:
>
>
> https://codereview.appspot.com/9159043/diff/17001/src/pkg/net/dial_gen.go
>
> I just did "hg upload 9159043" before I replied to dsymonds. Do you want
> me to run the command again?
>
>
>> I think adg & r want rc2 to be the final version, unless there are
>
> critical
>>
>> problems.  This can go in if other things are going in.
>
>
> Fair enough.
>
>> How would you classify the severity of this bug? ...
>
>
> I think it is serious. It is a race, so it makes it is so much harder to
> deal with. Here it is and here it is gone.
>
>> ... Is all UDP dialing broken
>> on Windows currently?
>
>
> I think so. And some TCP systems too.
>
>> ... Or just sometimes?
>
>
> Sometimes, for sure. It works most of the time, but sometimes it breaks,
> because it is a race.
>
>
> On 2013/05/08 00:02:01, adg wrote:
>
>> > Nobody spoke up about this bug when we asked what which outstanding
>
> bugs
>>
>> > were remaining.
>> >
>
>
>> Actually, Alex did speak up, but only after we had tagged rc2.
>
>
> Yes, I am a slow one in my family :-)
>
> Alex
>
> https://codereview.appspot.com/9159043/
>
> --
>
> ---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-08 00:25:21]

Hello golang-dev@googlegroups.com, lucio.dere@gmail.com, dsymonds@golang.org,
bradfitz@golang.org, iant@golang.org, adg@golang.org, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-05-08 00:25:49]

On 2013/05/08 00:13:57, dfc wrote:
> Yes, if you view the CL, the most current revision is broken because
> of a chunk mismatch.
> 

I don't see what the problem is, but I have updated to the tip and did "hg mail
9159043". Hope it helps.

Alex

[r, 2013-05-08 06:06:49]

LGTM

[brainman, 2013-05-08 06:19:11]

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

net: fix dial race on plan9 and windows

Fixes issue 5349.

R=golang-dev, lucio.dere, dsymonds, bradfitz, iant, adg, dave, r
CC=golang-dev
https://codereview.appspot.com/9159043