mgo

Code review comment for lp:~jameinel/mgo/bug-1191487-close-race

Revision history for this message
Roger Peppe (rogpeppe) wrote :

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).

https://codereview.appspot.com/10392043/

« Back to merge proposal