server.go: handle a race between Close and Connect

server.go: handle a race between Close and Connect

If one routine (such as the pinger) starts a new connection at the same time a Close is called (say by test case tear down), it is possible for that connection to return and be marked alive even though all other connections are closed.

This checks if closed has happened since the connect was requested, and immediately closes the new connection.

See bug #1191487 for more details about how this was triggered in the juju-core test suite.

https://code.launchpad.net/~jameinel/mgo/bug-1191487-close-race/+merge/169999

(do not edit description out of merge proposal)

[jameinel, 2013-06-18 13:11:23]

Please take a look.

[jameinel, 2013-06-18 13:16:12]

Please take a look.

[rog, 2013-06-18 13:29:04]

LGTM with a few thoughts and suggestions regarding the test.

https://codereview.appspot.com/10392043/diff/1/server.go
File server.go (right):

https://codereview.appspot.com/10392043/diff/1/server.go#newcode109
server.go:109: socket.Close()
server.Unlock()
(probably before calling socket.Close, but worth investigating)

https://codereview.appspot.com/10392043/diff/1/server_test.go
File server_test.go (right):

https://codereview.appspot.com/10392043/diff/1/server_test.go#newcode19
server_test.go:19: if server != nil {
is the dialler guaranteed not to be called concurrently?

https://codereview.appspot.com/10392043/diff/1/server_test.go#newcode22
server_test.go:22: return net.DialTCP("tcp", nil, addr.(*net.TCPAddr))
return net.Dial("tcp", addr.String()) ?

https://codereview.appspot.com/10392043/diff/1/server_test.go#newcode41
server_test.go:41: // It is possible for newServer to ping the remote host, and
the
s/possible/not possible/
?

https://codereview.appspot.com/10392043/diff/1/server_test.go#newcode52
server_test.go:52: }
i'm not sure this is guaranteed to work unless you have a sleep in here (i think
you're relying on logf yielding, which doesn't seem quite right).

[niemeyer, 2013-06-18 13:59:07]

https://codereview.appspot.com/10392043/diff/1/server.go
File server.go (right):

https://codereview.appspot.com/10392043/diff/1/server.go#newcode109
server.go:109: socket.Close()
On 2013/06/18 13:29:04, rog wrote:
> server.Unlock()
> (probably before calling socket.Close, but worth investigating)

Yeah, definitely.

https://codereview.appspot.com/10392043/diff/1/server.go#newcode110
server.go:110: return nil, false, errServerClosed
If we have a closed error (which is a good idea, thanks), it should also be
exercised before the connection is attempted.

https://codereview.appspot.com/10392043/diff/1/server.go#newcode264
server.go:264: } else if err == errServerClosed {
There are now two different ways to check for server closed in this loop. Can we
have a single one now that we have errServerClosed?

https://codereview.appspot.com/10392043/diff/1/server_test.go
File server_test.go (right):

https://codereview.appspot.com/10392043/diff/1/server_test.go#newcode16
server_test.go:16: // The first call to this will be during newServer, and
I appreciate this test as a good reproducer for the issue you're fixing, but I'm
afraid it's quite fragile and extremely attached to implementation details. If
there's no good way to test it in a more reliable way, I'm fine without an
additional test (note that you arrived at the fix from breakage in the test
suite itself).

[jameinel, 2013-06-18 14:32:25]

Please take a look.

https://codereview.appspot.com/10392043/diff/1/server_test.go
File server_test.go (right):

https://codereview.appspot.com/10392043/diff/1/server_test.go#newcode41
server_test.go:41: // It is possible for newServer to ping the remote host, and
the
On 2013/06/18 13:29:04, rog wrote:
> s/possible/not possible/
> ?

It is possible for (newServer to ping the remote host and have the write fail).

[niemeyer, 2013-06-18 14:35:23]

LGTM, thanks for the fix.

[niemeyer, 2013-06-18 14:40:34]

*** Submitted:

Fix race in AcquireSocket

Fix AcquireSocket so it doesn't put sockets in the live list after
the server has been closed. Fix by John Meinel.

R=rog, niemeyer
CC=
https://codereview.appspot.com/10392043