[FAB-9143] fix data race in TestOrderingService...

...StreamFailure

In the tests, client.Start() and client.Close() are called from different go routines
without serialization. Both methods modify a connection counter.

Looking closer, the connection counter is unnecessary as all references
to it are in assertions that follow connWG.Wait() calls that already
ensure outstanding connections have closed.

Fixing by removing the unnecessary connNumber state.

Change-Id: I112dcc59ff99ec7aacb29671ea6547748b03c2bf
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>

Author: sykesm

core/deliverservice/client_test.go

@@ -27,24 +27,20 @@ import (
 )
 
 var (
-	connNumber = 0
-	connWG     sync.WaitGroup
+	connWG sync.WaitGroup
 )
 
 func newConnection() *grpc.ClientConn {
 	// The balancer is in order to check connection leaks.
 	// When grpc.ClientConn.Close() is called, it calls the balancer's Close()
-	// method which decrements the connNumber
 	cc, _ := grpc.Dial("", grpc.WithInsecure(), grpc.WithBalancer(&balancer{}))
 	return cc
 }
 
-type balancer struct {
-}
+type balancer struct{}
 
 func (*balancer) Start(target string, config grpc.BalancerConfig) error {
 	connWG.Add(1)
-	connNumber++
 	return nil
 }
 
@@ -61,7 +57,6 @@ func (*balancer) Notify() <-chan []grpc.Address {
 }
 
 func (*balancer) Close() error {
-	connNumber--
 	connWG.Done()
 	return nil
 }
@@ -160,7 +155,6 @@ func TestOrderingServiceConnFailure(t *testing.T) {
 	testOrderingServiceConnFailure(t, blockDelivererConsumerWithRecv)
 	testOrderingServiceConnFailure(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testOrderingServiceConnFailure(t *testing.T, bdc blocksDelivererConsumer) {
@@ -198,7 +192,6 @@ func TestOrderingServiceStreamFailure(t *testing.T) {
 	testOrderingServiceStreamFailure(t, blockDelivererConsumerWithRecv)
 	testOrderingServiceStreamFailure(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testOrderingServiceStreamFailure(t *testing.T, bdc blocksDelivererConsumer) {
@@ -235,7 +228,6 @@ func TestOrderingServiceSetupFailure(t *testing.T) {
 	testOrderingServiceSetupFailure(t, blockDelivererConsumerWithRecv)
 	testOrderingServiceSetupFailure(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testOrderingServiceSetupFailure(t *testing.T, bdc blocksDelivererConsumer) {
@@ -270,7 +262,6 @@ func TestOrderingServiceFirstOperationFailure(t *testing.T) {
 	testOrderingServiceFirstOperationFailure(t, blockDelivererConsumerWithRecv)
 	testOrderingServiceFirstOperationFailure(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testOrderingServiceFirstOperationFailure(t *testing.T, bdc blocksDelivererConsumer) {
@@ -308,7 +299,6 @@ func TestOrderingServiceCrashAndRecover(t *testing.T) {
 	testOrderingServiceCrashAndRecover(t, blockDelivererConsumerWithRecv)
 	testOrderingServiceCrashAndRecover(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testOrderingServiceCrashAndRecover(t *testing.T, bdc blocksDelivererConsumer) {
@@ -350,7 +340,6 @@ func TestOrderingServicePermanentCrash(t *testing.T) {
 	testOrderingServicePermanentCrash(t, blockDelivererConsumerWithRecv)
 	testOrderingServicePermanentCrash(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testOrderingServicePermanentCrash(t *testing.T, bdc blocksDelivererConsumer) {
@@ -389,7 +378,6 @@ func TestLimitedConnAttempts(t *testing.T) {
 	testLimitedConnAttempts(t, blockDelivererConsumerWithRecv)
 	testLimitedConnAttempts(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testLimitedConnAttempts(t *testing.T, bdc blocksDelivererConsumer) {
@@ -418,13 +406,11 @@ func testLimitedConnAttempts(t *testing.T, bdc blocksDelivererConsumer) {
 func TestLimitedTotalConnTimeRcv(t *testing.T) {
 	testLimitedTotalConnTime(t, blockDelivererConsumerWithRecv)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func TestLimitedTotalConnTimeSnd(t *testing.T) {
 	testLimitedTotalConnTime(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testLimitedTotalConnTime(t *testing.T, bdc blocksDelivererConsumer) {
@@ -456,7 +442,6 @@ func TestGreenPath(t *testing.T) {
 	testGreenPath(t, blockDelivererConsumerWithRecv)
 	testGreenPath(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testGreenPath(t *testing.T, bdc blocksDelivererConsumer) {
@@ -523,7 +508,6 @@ func TestCloseWhileSleep(t *testing.T) {
 	testCloseWhileSleep(t, blockDelivererConsumerWithRecv)
 	testCloseWhileSleep(t, blockDelivererConsumerWithSend)
 	connWG.Wait()
-	assert.Equal(t, 0, connNumber)
 }
 
 func testCloseWhileSleep(t *testing.T, bdc blocksDelivererConsumer) {

core/deliverservice/deliveryclient_test.go

@@ -124,7 +124,8 @@ func TestNewDeliverService(t *testing.T) {
 	// Make sure to stop all blocks providers
 	service.Stop()
 	time.Sleep(time.Duration(500) * time.Millisecond)
-	assert.Equal(t, 0, connNumber)
+	connWG.Wait()
+
 	assertBlockDissemination(0, gossipServiceAdapter.GossipBlockDisseminations, t)
 	assert.Equal(t, atomic.LoadInt32(&blocksDeliverer.RecvCnt), atomic.LoadInt32(&gossipServiceAdapter.AddPayloadsCnt))
 	assert.Error(t, service.StartDeliverForChannel("TEST_CHAINID", &mocks.MockLedgerInfo{0}, func() {}), "Delivery service is stopping")