Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4)

Issue 6002053: code review 6002053: net: fix race between Close and Read (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 1 month ago by dfc
Modified:
3 years, 1 month ago
Reviewers:
niemeyer
CC:
rsc, albert.strasheim, brainman, mikio, golang-dev
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 762426ee0cca https://code.google.com/p/go #

Patch Set 3 : diff -r 762426ee0cca https://code.google.com/p/go #

Patch Set 4 : diff -r 762426ee0cca https://code.google.com/p/go #

Patch Set 5 : diff -r f6e1ee2d8cd1 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 6 : diff -r b25d41fa3e5f https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -12 lines) Patch
M src/pkg/net/iprawsock_posix.go View 1 2 3 4 1 chunk +1 line, -3 lines 0 comments Download
M src/pkg/net/tcpsock_posix.go View 1 2 3 1 chunk +1 line, -3 lines 0 comments Download
M src/pkg/net/udpsock_posix.go View 1 2 3 4 1 chunk +1 line, -3 lines 0 comments Download
M src/pkg/net/unixsock_posix.go View 1 2 3 4 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 16
dfc
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
3 years, 1 month ago (2012-04-14 09:10:20 UTC) #1
dfc
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
3 years, 1 month ago (2012-04-14 09:11:39 UTC) #2
dfc
Removed the local err variable, just return c.fd.Close()
3 years, 1 month ago (2012-04-14 09:12:25 UTC) #3
albert.strasheim
Is this only a TCP socket problem or could UNIX and UDP also be affected?
3 years, 1 month ago (2012-04-14 09:27:04 UTC) #4
dfc
Excellent point. I had reviewed tcpsock_posix.go for potential side effects of this change but neglected ...
3 years, 1 month ago (2012-04-14 09:45:07 UTC) #5
dfc
Hello Devon, Alex and Mikio. Would you please review this change. Functional test cases are ...
3 years, 1 month ago (2012-04-14 10:11:07 UTC) #6
brainman
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: ...
3 years, 1 month ago (2012-04-16 01:53:09 UTC) #7
dfc
Thanks Alex. I'll have another look at the TCP test this evening. On 16/04/2012, at ...
3 years, 1 month ago (2012-04-16 02:30:48 UTC) #8
mikio
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.go#newcode63 src/pkg/net/iprawsock_posix.go:63: func (c *IPConn) ok() bool ...
3 years, 1 month ago (2012-04-17 01:25:08 UTC) #9
dfc
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.go#newcode63 src/pkg/net/iprawsock_posix.go:63: func (c *IPConn) ok() ...
3 years, 1 month ago (2012-04-17 09:17:38 UTC) #10
dfc
@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 ...
3 years, 1 month ago (2012-04-18 06:52:49 UTC) #11
brainman
On 2012/04/18 06:52:49, dfc wrote: > @brainman, could you please try this version of the ...
3 years, 1 month ago (2012-04-18 23:40:05 UTC) #12
brainman
On Fri, Apr 20, 2012 at 1:12 AM, Dave Cheney <dave@cheney.net> wrote: > > ...
3 years, 1 month ago (2012-04-20 01:25:33 UTC) #13
dfc
Hi Alex, Thank you for your diagnosis, your explanation sounds plausible. The stress test does ...
3 years, 1 month ago (2012-04-20 02:02:30 UTC) #14
dfc
*** Submitted as http://code.google.com/p/go/source/detail?r=5f24ff99b5f1 *** net: fix race between Close and Read Fixes issue 3507. ...
3 years, 1 month ago (2012-04-21 00:01:51 UTC) #15
niemeyer
3 years, 1 month ago (2012-04-25 18:01:53 UTC) #16
Thanks for the fix Dave. Just got a report from someone
affected by this bug as well.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld e6eadc5