[FAB-6186] MSP cache should not use RWMutex

The MSP cache has RW locks and uses RLock when doing cache lookup,
and WLock when doing cache updates.

This may result in the an error if done concurrently while the cache
has more than 1 item and only cache lookups are performed,
because the cache lookup operation shifts and moves items in the
internal list of the cache (that specifies eviction order,
since its an LRU cache).

The test was changed and would fail wit a null pointer panic
if the cache.go file would be reverted back.

Change-Id: I97f8b8cce0ee21090da0b036814da52acbf3be60
Signed-off-by: yacovm <[email protected]>

msp/cache/cache.go

@@ -8,7 +8,6 @@ package cache
 
 import (
 	"fmt"
-
 	"sync"
 
 	"github.com/golang/groupcache/lru"
@@ -45,24 +44,24 @@ type cachedMSP struct {
 	// cache for DeserializeIdentity.
 	deserializeIdentityCache *lru.Cache
 
-	dicMutex sync.RWMutex // synchronize access to cache
+	dicMutex sync.Mutex // synchronize access to cache
 
 	// cache for validateIdentity
 	validateIdentityCache *lru.Cache
 
-	vicMutex sync.RWMutex // synchronize access to cache
+	vicMutex sync.Mutex // synchronize access to cache
 
 	// basically a map of principals=>identities=>stringified to booleans
 	// specifying whether this identity satisfies this principal
 	satisfiesPrincipalCache *lru.Cache
 
-	spcMutex sync.RWMutex // synchronize access to cache
+	spcMutex sync.Mutex // synchronize access to cache
 }
 
 func (c *cachedMSP) DeserializeIdentity(serializedIdentity []byte) (msp.Identity, error) {
-	c.dicMutex.RLock()
+	c.dicMutex.Lock()
 	cached, ok := c.deserializeIdentityCache.Get(string(serializedIdentity))
-	c.dicMutex.RUnlock()
+	c.dicMutex.Unlock()
 	if ok {
 		return cached.(msp.Identity), nil
 	}
@@ -86,9 +85,9 @@ func (c *cachedMSP) Validate(id msp.Identity) error {
 	identifier := id.GetIdentifier()
 	key := string(identifier.Mspid + ":" + identifier.Id)
 
-	c.vicMutex.RLock()
+	c.vicMutex.Lock()
 	_, ok := c.validateIdentityCache.Get(key)
-	c.vicMutex.RUnlock()
+	c.vicMutex.Unlock()
 	if ok {
 		// cache only stores if the identity is valid.
 		return nil
@@ -110,9 +109,9 @@ func (c *cachedMSP) SatisfiesPrincipal(id msp.Identity, principal *pmsp.MSPPrinc
 	principalKey := string(principal.PrincipalClassification) + string(principal.Principal)
 	key := identityKey + principalKey
 
-	c.spcMutex.RLock()
+	c.spcMutex.Lock()
 	v, ok := c.satisfiesPrincipalCache.Get(key)
-	c.spcMutex.RUnlock()
+	c.spcMutex.Unlock()
 	if ok {
 		if v == nil {
 			return nil

msp/cache/cache_test.go

@@ -7,6 +7,7 @@ SPDX-License-Identifier: Apache-2.0
 package cache
 
 import (
+	"sync"
 	"testing"
 
 	"github.com/hyperledger/fabric/msp"
@@ -115,23 +116,47 @@ func TestGetTLSIntermediateCerts(t *testing.T) {
 
 func TestDeserializeIdentity(t *testing.T) {
 	mockMSP := &mocks.MockMSP{}
-	i, err := New(mockMSP)
+	wrappedMSP, err := New(mockMSP)
 	assert.NoError(t, err)
 
 	// Check id is cached
 	mockIdentity := &mocks.MockIdentity{ID: "Alice"}
+	mockIdentity2 := &mocks.MockIdentity{ID: "Bob"}
 	serializedIdentity := []byte{1, 2, 3}
+	serializedIdentity2 := []byte{4, 5, 6}
 	mockMSP.On("DeserializeIdentity", serializedIdentity).Return(mockIdentity, nil)
-	id, err := i.DeserializeIdentity(serializedIdentity)
-	assert.NoError(t, err)
-	assert.Equal(t, mockIdentity, id)
+	mockMSP.On("DeserializeIdentity", serializedIdentity2).Return(mockIdentity2, nil)
+	// Prime the cache
+	wrappedMSP.DeserializeIdentity(serializedIdentity)
+	wrappedMSP.DeserializeIdentity(serializedIdentity2)
+
+	// Stress the cache and ensure concurrent operations
+	// do not result in a failure
+	var wg sync.WaitGroup
+	wg.Add(10000)
+	for i := 0; i < 10000; i++ {
+		go func(m msp.MSP) {
+			sIdentity := serializedIdentity
+			expectedIdentity := mockIdentity
+			defer wg.Done()
+			if i%2 == 0 {
+				sIdentity = serializedIdentity2
+				expectedIdentity = mockIdentity2
+			}
+			id, err := wrappedMSP.DeserializeIdentity(sIdentity)
+			assert.NoError(t, err)
+			assert.Equal(t, expectedIdentity, id)
+		}(wrappedMSP)
+	}
+	wg.Wait()
+
 	mockMSP.AssertExpectations(t)
 	// Check the cache
-	_, ok := i.(*cachedMSP).deserializeIdentityCache.Get(string(serializedIdentity))
+	_, ok := wrappedMSP.(*cachedMSP).deserializeIdentityCache.Get(string(serializedIdentity))
 	assert.True(t, ok)
 
 	// Check the same object is returned
-	id, err = i.DeserializeIdentity(serializedIdentity)
+	id, err := wrappedMSP.DeserializeIdentity(serializedIdentity)
 	assert.NoError(t, err)
 	assert.True(t, mockIdentity == id)
 	mockMSP.AssertExpectations(t)
@@ -140,12 +165,12 @@ func TestDeserializeIdentity(t *testing.T) {
 	mockIdentity = &mocks.MockIdentity{ID: "Bob"}
 	serializedIdentity = []byte{1, 2, 3, 4}
 	mockMSP.On("DeserializeIdentity", serializedIdentity).Return(mockIdentity, errors.New("Invalid identity"))
-	id, err = i.DeserializeIdentity(serializedIdentity)
+	id, err = wrappedMSP.DeserializeIdentity(serializedIdentity)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Invalid identity")
 	mockMSP.AssertExpectations(t)
 
-	_, ok = i.(*cachedMSP).deserializeIdentityCache.Get(string(serializedIdentity))
+	_, ok = wrappedMSP.(*cachedMSP).deserializeIdentityCache.Get(string(serializedIdentity))
 	assert.False(t, ok)
 }