code review 6002053: net: fix race between Close and Read

net: fix race between Close and Read

Fixes issue 3507.

Applied the suggested fix from rsc. If the connection
is in closing state then errClosing will bubble up to
the caller.

The fix has been applied to udp, ip and unix as well as
their code path include nil'ing c.fd on close. Func
tests are available in the linked issue that verified
the bug existed there as well.

[dave_cheney.net, 2012-04-14 09:10:20]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[dave_cheney.net, 2012-04-14 09:11:39]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-04-14 09:12:25]

Removed the local err variable, just return c.fd.Close()

[albert.strasheim, 2012-04-14 09:27:04]

Is this only a TCP socket problem or could UNIX and UDP also be affected?

[dave_cheney.net, 2012-04-14 09:45:07]

Excellent point.

I had reviewed tcpsock_posix.go for potential side effects of this
change but neglected the other protocols (udp, ip, unix).

[dave_cheney.net, 2012-04-14 10:11:07]

Hello Devon, Alex and Mikio. 

Would you please review this change. Functional test cases are available in the
comments of issue 3507.

Cheers

Dave

[brainman, 2012-04-16 01:53:09]

I could not run your netstress.go from
http://code.google.com/p/go/issues/detail?id=3507#c2. It fails with
WSAEADDRINUSE:

dial tcp 127.0.0.1:9999: Only one usage of each socket address (protocol/network
address/port) is normally permitted.

If I ignore these, I get different error (ERROR_NETNAME_DELETED):

AcceptEx tcp 127.0.0.1:9999: The specified network name is no longer available.

If I ignore these, I get different error (ERROR_OPERATION_ABORTED):

AcceptEx tcp 127.0.0.1:9999: The I/O operation has been aborted because of
either a thread exit or an application request.

an so on. So your test program is very flaky on Windows. I can't check if your
fix makes any difference.

Your udpstress.go test runs fine before and after your CL applied. So no
difference here.

unixstress.go does not work, because Windows does not have unix sockets.

I didn't look into your issue enough to make a judgement. Leaving up to others
to decide on this change. Even if it will break some windows programs we could
investigate it then.

Alex

[dave_cheney.net, 2012-04-16 02:30:48]

Thanks Alex. I'll have another look at the TCP test this evening. 

On 16/04/2012, at 11:53, alex.brainman@gmail.com wrote:

> I could not run your netstress.go from
> http://code.google.com/p/go/issues/detail?id=3507#c2. It fails with
> WSAEADDRINUSE:
> 
> dial tcp 127.0.0.1:9999: Only one usage of each socket address
> (protocol/network address/port) is normally permitted.
> 
> If I ignore these, I get different error (ERROR_NETNAME_DELETED):
> 
> AcceptEx tcp 127.0.0.1:9999: The specified network name is no longer
> available.
> 
> If I ignore these, I get different error (ERROR_OPERATION_ABORTED):
> 
> AcceptEx tcp 127.0.0.1:9999: The I/O operation has been aborted because
> of either a thread exit or an application request.
> 
> an so on. So your test program is very flaky on Windows. I can't check
> if your fix makes any difference.
> 
> Your udpstress.go test runs fine before and after your CL applied. So no
> difference here.
> 
> unixstress.go does not work, because Windows does not have unix sockets.
> 
> I didn't look into your issue enough to make a judgement. Leaving up to
> others to decide on this change. Even if it will break some windows
> programs we could investigate it then.
> 
> Alex
> 
> http://codereview.appspot.com/6002053/

[mikio, 2012-04-17 01:25:08]

Sorry for late response.

http://codereview.appspot.com/6002053/diff/6002/src/pkg/net/iprawsock_posix.go
File src/pkg/net/iprawsock_posix.go (right):

http://codereview.appspot.com/6002053/diff/6002/src/pkg/net/iprawsock_posix.g...
src/pkg/net/iprawsock_posix.go:63: func (c *IPConn) ok() bool { return c != nil
&& c.fd != nil }
you may drop "c.fd != nil" and add other validation if you like.

[dave_cheney.net, 2012-04-17 09:17:38]

Thanks for your review Mikio.

http://codereview.appspot.com/6002053/diff/6002/src/pkg/net/iprawsock_posix.go
File src/pkg/net/iprawsock_posix.go (right):

http://codereview.appspot.com/6002053/diff/6002/src/pkg/net/iprawsock_posix.g...
src/pkg/net/iprawsock_posix.go:63: func (c *IPConn) ok() bool { return c != nil
&& c.fd != nil }
On 2012/04/17 01:25:09, mikioh wrote:
> you may drop "c.fd != nil" and add other validation if you like.

I'm not sure about removing the c.fd != nil check just yet. I'd like to think
about the cases where c.fd is assigned a bit more.

[dave_cheney.net, 2012-04-18 06:52:49]

@brainman, could you please try this version of the tcp test

http://code.google.com/p/go/issues/detail?id=3507#c9

I've bound the listening and dialing sockets to tcp4 which may (I hope) have
been the problem.

[brainman, 2012-04-18 23:40:05]

On 2012/04/18 06:52:49, dfc wrote:
> @brainman, could you please try this version of the tcp test
> 

Same as before:

2012/04/19 09:35:36 AcceptEx tcp4 127.0.0.1:9999: The specified network name is
no longer available.

Alex

[brainman, 2012-04-20 01:25:33]

On Fri, Apr 20, 2012 at 1:12 AM, Dave Cheney <dave@cheney.net> wrote:
>
> ... My best guess is
> this is a problem resolving the dns name inside net.Dial.
>
> Can you please try this version, setting GOMAXPROCS=100
>

Sure. Same error message:

2012/04/20 11:11:04 AcceptEx tcp4 127.0.0.1:2222: The specified network name is
no longer available.

I do not think this has anything to do with dns. I think new connection arrives
and gets disconnected so quick, that by the time Accept discover new connection
and tries accepting it, it is already gone. I think Accept is confused. It
reports new connection back to us, but since connection is already disconnected,
it signals error.

I am guessing here :-)

Dave, I suggest, you ignore windows. Proceed with your change. I am not
concerned about your sample program does not work on windows. No one should be
doing that anyway.

Alex

[dave_cheney.net, 2012-04-20 02:02:30]

Hi Alex,

Thank you for your diagnosis, your explanation sounds plausible. The
stress test does rely on some uncivilised socket usage to trigger the
original race.

Assuming no further discussion, I plan to submit this tomorrow morning
when I'll have time to debug any build breakage.

Cheers

Dave

On Fri, Apr 20, 2012 at 11:25 AM,  <alex.brainman@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 1:12 AM, Dave Cheney <dave@cheney.net> wrote:
>
>> ... My best guess is
>>
>> this is a problem resolving the dns name inside net.Dial.
>
>
>> Can you please try this version, setting GOMAXPROCS=100
>
>
>
> Sure. Same error message:
>
> 2012/04/20 11:11:04 AcceptEx tcp4 127.0.0.1:2222: The specified network
>
> name is no longer available.
>
> I do not think this has anything to do with dns. I think new connection
> arrives and gets disconnected so quick, that by the time Accept discover
> new connection and tries accepting it, it is already gone. I think
> Accept is confused. It reports new connection back to us, but since
> connection is already disconnected, it signals error.
>
> I am guessing here :-)
>
> Dave, I suggest, you ignore windows. Proceed with your change. I am not
> concerned about your sample program does not work on windows. No one
> should be doing that anyway.
>
> Alex
>
> http://codereview.appspot.com/6002053/

[dave_cheney.net, 2012-04-21 00:01:51]

*** Submitted as http://code.google.com/p/go/source/detail?r=5f24ff99b5f1 ***

net: fix race between Close and Read

Fixes issue 3507.

Applied the suggested fix from rsc. If the connection
is in closing state then errClosing will bubble up to
the caller.

The fix has been applied to udp, ip and unix as well as
their code path include nil'ing c.fd on close. Func
tests are available in the linked issue that verified
the bug existed there as well.

R=rsc, fullung, alex.brainman, mikioh.mikioh
CC=golang-dev
http://codereview.appspot.com/6002053

[niemeyer, 2012-04-25 18:01:53]

Thanks for the fix Dave. Just got a report from someone
affected by this bug as well.