Drain session pool before creating new sessions

If a bad session was retrieved from the session pool, a new session would be immediately created instead of attempting to retrieve another session from the pool. This change attempts to uses pooled sessions before creating new ones. Signed-off-by: Matthew Sykes <[email protected]> Signed-off-by: Gari Singh <[email protected]>
hyperledger · Aug 22, 2020 · f47ac7d · f47ac7d
1 parent 4fd232e
commit f47ac7d
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 13 deletions.
diff --git a/bccsp/pkcs11/pkcs11.go b/bccsp/pkcs11/pkcs11.go
@@ -65,22 +65,22 @@ func loadLib(lib, pin, label string) (*pkcs11.Ctx, uint, *pkcs11.SessionHandle,
 }
 
 func (csp *impl) getSession() (session pkcs11.SessionHandle, err error) {
-	select {
-	case session = <-csp.sessions:
-		_, err = csp.ctx.GetSessionInfo(session)
-		if err != nil {
+	for {
+		select {
+		case session = <-csp.sessions:
+			if _, err = csp.ctx.GetSessionInfo(session); err == nil {
+				logger.Debugf("Reusing existing pkcs11 session %d on slot %d\n", session, csp.slot)
+				return session, nil
+			}
+
 			logger.Warningf("Get session info failed [%s], closing existing session and getting a new session\n", err)
 			csp.ctx.CloseSession(session)
-			session, err = createSession(csp.ctx, csp.slot, csp.pin)
-		} else {
-			logger.Debugf("Reusing existing pkcs11 session %+v on slot %d\n", session, csp.slot)
-		}
 
-	default:
-		// cache is empty (or completely in use), create a new session
-		session, err = createSession(csp.ctx, csp.slot, csp.pin)
+		default:
+			// cache is empty (or completely in use), create a new session
+			return createSession(csp.ctx, csp.slot, csp.pin)
+		}
 	}
-	return session, err
 }
 
 func createSession(ctx *pkcs11.Ctx, slot uint, pin string) (pkcs11.SessionHandle, error) {

diff --git a/bccsp/pkcs11/pkcs11_test.go b/bccsp/pkcs11/pkcs11_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/hyperledger/fabric/bccsp"
 	"github.com/miekg/pkcs11"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestKeyGenFailures(t *testing.T) {
@@ -103,11 +104,28 @@ func TestPKCS11GetSession(t *testing.T) {
 	_, err := currentBCCSP.(*impl).getSession()
 	assert.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID")
 
+	// Load cache with bad sessions
+	for i := 0; i < sessionCacheSize; i++ {
+		currentBCCSP.(*impl).returnSession(pkcs11.SessionHandle(^uint(0)))
+	}
+
+	// Fix OpenSession so non-cached sessions can be opened
+	currentBCCSP.(*impl).slot = oldSlot
+
+	// Request a session, return, and re-acquire. The pool should be emptied
+	// before creating a new session so when returned, it should be the only
+	// session in the cache.
+	sess, err := currentBCCSP.(*impl).getSession()
+	require.NoError(t, err)
+	currentBCCSP.(*impl).returnSession(sess)
+	sess2, err := currentBCCSP.(*impl).getSession()
+	require.NoError(t, err)
+	require.Equal(t, sess, sess2, "expected to get back the same session")
+
 	// Cleanup
 	for _, session := range sessions {
 		currentBCCSP.(*impl).returnSession(session)
 	}
-	currentBCCSP.(*impl).slot = oldSlot
 }
 
 func TestPKCS11ECKeySignVerify(t *testing.T) {