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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by dfc
Modified:
1 year, 11 months 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
2 years ago #1
dfc
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
2 years ago #2
dfc
Removed the local err variable, just return c.fd.Close()
2 years ago #3
albert.strasheim
Is this only a TCP socket problem or could UNIX and UDP also be affected?
2 years ago #4
dfc
Excellent point. I had reviewed tcpsock_posix.go for potential side effects of this change but neglected ...
2 years ago #5
dfc
Hello Devon, Alex and Mikio. Would you please review this change. Functional test cases are ...
2 years ago #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: ...
2 years ago #7
dfc
Thanks Alex. I'll have another look at the TCP test this evening. On 16/04/2012, at ...
2 years ago #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 ...
1 year, 12 months ago #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() ...
1 year, 12 months ago #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 ...
1 year, 12 months ago #11
brainman
On 2012/04/18 06:52:49, dfc wrote: > @brainman, could you please try this version of the ...
1 year, 12 months ago #12
brainman
On Fri, Apr 20, 2012 at 1:12 AM, Dave Cheney <dave@cheney.net> wrote: > > ...
1 year, 12 months ago #13
dfc
Hi Alex, Thank you for your diagnosis, your explanation sounds plausible. The stress test does ...
1 year, 12 months ago #14
dfc
*** Submitted as http://code.google.com/p/go/source/detail?r=5f24ff99b5f1 *** net: fix race between Close and Read Fixes issue 3507. ...
1 year, 12 months ago #15
niemeyer
1 year, 11 months ago #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 1274:9b9295c7fd64