Remove a lot of deferred functions in the request path. (#4733)

* Remove a lot of deferred functions in the request path.

There is an interesting benchmark at https://www.reddit.com/r/golang/comments/3h21nk/simple_micro_benchmark_to_measure_the_overhead_of/

It shows that defer actually adds quite a lot of overhead -- maybe 100ns
per call but we defer a *lot* of functions in the request path. So this
removes some of the ones in request handling, ha, barrier, router, and
physical cache.

One meta-note: nearly every metrics function is in a defer which means
every metrics call we add could add a non-trivial amount of time, e.g.
for every 10 extra metrics statements we add 1ms to a request. I don't
know how to solve this right now without doing what I did in some of
these cases and putting that call into a simple function call that then
goes before each return.

* Simplify barrier defer cleanup

physical/cache.go

@@ -119,12 +119,13 @@ func (c *Cache) Put(ctx context.Context, entry *Entry) error {
 
 	lock := locksutil.LockForKey(c.locks, entry.Key)
 	lock.Lock()
-	defer lock.Unlock()
 
 	err := c.backend.Put(ctx, entry)
 	if err == nil {
 		c.lru.Add(entry.Key, entry)
 	}
+
+	lock.Unlock()
 	return err
 }
 
@@ -135,19 +136,21 @@ func (c *Cache) Get(ctx context.Context, key string) (*Entry, error) {
 
 	lock := locksutil.LockForKey(c.locks, key)
 	lock.RLock()
-	defer lock.RUnlock()
 
 	// Check the LRU first
 	if raw, ok := c.lru.Get(key); ok {
 		if raw == nil {
+			lock.RUnlock()
 			return nil, nil
 		}
+		lock.RUnlock()
 		return raw.(*Entry), nil
 	}
 
 	// Read from the underlying backend
 	ent, err := c.backend.Get(ctx, key)
 	if err != nil {
+		lock.RUnlock()
 		return nil, err
 	}
 
@@ -156,6 +159,7 @@ func (c *Cache) Get(ctx context.Context, key string) (*Entry, error) {
 		c.lru.Add(key, ent)
 	}
 
+	lock.RUnlock()
 	return ent, nil
 }
 
@@ -166,12 +170,13 @@ func (c *Cache) Delete(ctx context.Context, key string) error {
 
 	lock := locksutil.LockForKey(c.locks, key)
 	lock.Lock()
-	defer lock.Unlock()
 
 	err := c.backend.Delete(ctx, key)
 	if err == nil {
 		c.lru.Remove(key)
 	}
+
+	lock.Unlock()
 	return err
 }
 
@@ -196,10 +201,16 @@ func (c *TransactionalCache) Transaction(ctx context.Context, txns []*TxnEntry)
 	// Lock the keys
 	for _, l := range locksutil.LocksForKeys(c.locks, keys) {
 		l.Lock()
-		defer l.Unlock()
+	}
+
+	unlockFunc := func() {
+		for _, l := range locksutil.LocksForKeys(c.locks, keys) {
+			l.Unlock()
+		}
 	}
 
 	if err := c.Transactional.Transaction(ctx, txns); err != nil {
+		unlockFunc()
 		return err
 	}
 
@@ -216,5 +227,6 @@ func (c *TransactionalCache) Transaction(ctx context.Context, txns []*TxnEntry)
 		}
 	}
 
+	unlockFunc()
 	return nil
 }

vault/barrier_aes_gcm.go

@@ -213,8 +213,9 @@ func (b *AESGCMBarrier) KeyLength() (int, int) {
 // is not expected to be able to perform any CRUD until it is unsealed.
 func (b *AESGCMBarrier) Sealed() (bool, error) {
 	b.l.RLock()
-	defer b.l.RUnlock()
+	sealed := b.sealed
-	return b.sealed, nil
+	b.l.RUnlock()
+	return sealed, nil
 }
 
 // VerifyMaster is used to check if the given key matches the master key
@@ -636,14 +637,16 @@ func (b *AESGCMBarrier) updateMasterKeyCommon(key []byte) (*Keyring, error) {
 func (b *AESGCMBarrier) Put(ctx context.Context, entry *Entry) error {
 	defer metrics.MeasureSince([]string{"barrier", "put"}, time.Now())
 	b.l.RLock()
-	defer b.l.RUnlock()
+
 	if b.sealed {
+		b.l.RUnlock()
 		return ErrBarrierSealed
 	}
 
 	term := b.keyring.ActiveTerm()
 	primary, err := b.aeadForTerm(term)
 	if err != nil {
+		b.l.RUnlock()
 		return err
 	}
 
@@ -652,29 +655,37 @@ func (b *AESGCMBarrier) Put(ctx context.Context, entry *Entry) error {
 		Value:    b.encrypt(entry.Key, term, primary, entry.Value),
 		SealWrap: entry.SealWrap,
 	}
-	return b.backend.Put(ctx, pe)
+
+	err = b.backend.Put(ctx, pe)
+
+	b.l.RUnlock()
+	return err
 }
 
 // Get is used to fetch an entry
 func (b *AESGCMBarrier) Get(ctx context.Context, key string) (*Entry, error) {
 	defer metrics.MeasureSince([]string{"barrier", "get"}, time.Now())
 	b.l.RLock()
-	defer b.l.RUnlock()
+
 	if b.sealed {
+		b.l.RUnlock()
 		return nil, ErrBarrierSealed
 	}
 
 	// Read the key from the backend
 	pe, err := b.backend.Get(ctx, key)
 	if err != nil {
+		b.l.RUnlock()
 		return nil, err
 	} else if pe == nil {
+		b.l.RUnlock()
 		return nil, nil
 	}
 
 	// Decrypt the ciphertext
 	plain, err := b.decryptKeyring(key, pe.Value)
 	if err != nil {
+		b.l.RUnlock()
 		return nil, errwrap.Wrapf("decryption failed: {{err}}", err)
 	}
 
@@ -684,6 +695,8 @@ func (b *AESGCMBarrier) Get(ctx context.Context, key string) (*Entry, error) {
 		Value:    plain,
 		SealWrap: pe.SealWrap,
 	}
+
+	b.l.RUnlock()
 	return entry, nil
 }
 
@@ -691,12 +704,16 @@ func (b *AESGCMBarrier) Get(ctx context.Context, key string) (*Entry, error) {
 func (b *AESGCMBarrier) Delete(ctx context.Context, key string) error {
 	defer metrics.MeasureSince([]string{"barrier", "delete"}, time.Now())
 	b.l.RLock()
-	defer b.l.RUnlock()
+
 	if b.sealed {
+		b.l.RUnlock()
 		return ErrBarrierSealed
 	}
 
-	return b.backend.Delete(ctx, key)
+	err := b.backend.Delete(ctx, key)
+
+	b.l.RUnlock()
+	return err
 }
 
 // List is used ot list all the keys under a given
@@ -704,12 +721,16 @@ func (b *AESGCMBarrier) Delete(ctx context.Context, key string) error {
 func (b *AESGCMBarrier) List(ctx context.Context, prefix string) ([]string, error) {
 	defer metrics.MeasureSince([]string{"barrier", "list"}, time.Now())
 	b.l.RLock()
-	defer b.l.RUnlock()
+
 	if b.sealed {
+		b.l.RUnlock()
 		return nil, ErrBarrierSealed
 	}
 
-	return b.backend.List(ctx, prefix)
+	keys, err := b.backend.List(ctx, prefix)
+
+	b.l.RUnlock()
+	return keys, err
 }
 
 // aeadForTerm returns the AES-GCM AEAD for the given term
@@ -852,34 +873,42 @@ func (b *AESGCMBarrier) decryptKeyring(path string, cipher []byte) ([]byte, erro
 // Encrypt is used to encrypt in-memory for the BarrierEncryptor interface
 func (b *AESGCMBarrier) Encrypt(ctx context.Context, key string, plaintext []byte) ([]byte, error) {
 	b.l.RLock()
-	defer b.l.RUnlock()
+
 	if b.sealed {
+		b.l.RUnlock()
 		return nil, ErrBarrierSealed
 	}
 
 	term := b.keyring.ActiveTerm()
 	primary, err := b.aeadForTerm(term)
 	if err != nil {
+		b.l.RUnlock()
 		return nil, err
 	}
 
 	ciphertext := b.encrypt(key, term, primary, plaintext)
+
+	b.l.RUnlock()
 	return ciphertext, nil
 }
 
 // Decrypt is used to decrypt in-memory for the BarrierEncryptor interface
 func (b *AESGCMBarrier) Decrypt(ctx context.Context, key string, ciphertext []byte) ([]byte, error) {
 	b.l.RLock()
-	defer b.l.RUnlock()
+
 	if b.sealed {
+		b.l.RUnlock()
 		return nil, ErrBarrierSealed
 	}
 
 	// Decrypt the ciphertext
 	plain, err := b.decryptKeyring(key, ciphertext)
 	if err != nil {
+		b.l.RUnlock()
 		return nil, errwrap.Wrapf("decryption failed: {{err}}", err)
 	}
+
+	b.l.RUnlock()
 	return plain, nil
 }
 

vault/ha.go

@@ -23,8 +23,9 @@ import (
 // Standby checks if the Vault is in standby mode
 func (c *Core) Standby() (bool, error) {
 	c.stateLock.RLock()
-	defer c.stateLock.RUnlock()
+	standby := c.standby
-	return c.standby, nil
+	c.stateLock.RUnlock()
+	return standby, nil
 }
 
 // Leader is used to get the current active leader
@@ -36,30 +37,34 @@ func (c *Core) Leader() (isLeader bool, leaderAddr, clusterAddr string, err erro
 	}
 
 	c.stateLock.RLock()
-	defer c.stateLock.RUnlock()
 
 	// Check if sealed
 	if c.sealed {
+		c.stateLock.RUnlock()
 		return false, "", "", consts.ErrSealed
 	}
 
 	// Check if we are the leader
 	if !c.standby {
+		c.stateLock.RUnlock()
 		return true, c.redirectAddr, c.clusterAddr, nil
 	}
 
 	// Initialize a lock
 	lock, err := c.ha.LockWith(coreLockPath, "read")
 	if err != nil {
+		c.stateLock.RUnlock()
 		return false, "", "", err
 	}
 
 	// Read the value
 	held, leaderUUID, err := lock.Value()
 	if err != nil {
+		c.stateLock.RUnlock()
 		return false, "", "", err
 	}
 	if !held {
+		c.stateLock.RUnlock()
 		return false, "", "", nil
 	}
 
@@ -72,11 +77,13 @@ func (c *Core) Leader() (isLeader bool, leaderAddr, clusterAddr string, err erro
 	// If the leader hasn't changed, return the cached value; nothing changes
 	// mid-leadership, and the barrier caches anyways
 	if leaderUUID == localLeaderUUID && localRedirAddr != "" {
+		c.stateLock.RUnlock()
 		return false, localRedirAddr, localClusterAddr, nil
 	}
 
 	c.logger.Trace("found new active node information, refreshing")
 
+	defer c.stateLock.RUnlock()
 	c.clusterLeaderParamsLock.Lock()
 	defer c.clusterLeaderParamsLock.Unlock()
 

vault/request_handling.go

@@ -252,16 +252,17 @@ func (c *Core) checkToken(ctx context.Context, req *logical.Request, unauth bool
 // HandleRequest is used to handle a new incoming request
 func (c *Core) HandleRequest(req *logical.Request) (resp *logical.Response, err error) {
 	c.stateLock.RLock()
-	defer c.stateLock.RUnlock()
+
 	if c.sealed {
+		c.stateLock.RUnlock()
 		return nil, consts.ErrSealed
 	}
 	if c.standby {
+		c.stateLock.RUnlock()
 		return nil, consts.ErrStandby
 	}
 
 	ctx, cancel := context.WithCancel(c.activeContext)
-	defer cancel()
 
 	// Allowing writing to a path ending in / makes it extremely difficult to
 	// understand user intent for the filesystem-like backends (kv,
@@ -272,6 +273,8 @@ func (c *Core) HandleRequest(req *logical.Request) (resp *logical.Response, err
 	if strings.HasSuffix(req.Path, "/") &&
 		(req.Operation == logical.UpdateOperation ||
 			req.Operation == logical.CreateOperation) {
+		cancel()
+		c.stateLock.RUnlock()
 		return logical.ErrorResponse("cannot write to a path ending in '/'"), nil
 	}
 
@@ -338,6 +341,8 @@ func (c *Core) HandleRequest(req *logical.Request) (resp *logical.Response, err
 			err := jsonutil.DecodeJSON(resp.Data[logical.HTTPRawBody].([]byte), httpResp)
 			if err != nil {
 				c.logger.Error("failed to unmarshal wrapped HTTP response for audit logging", "error", err)
+				cancel()
+				c.stateLock.RUnlock()
 				return nil, ErrInternalError
 			}
 
@@ -373,9 +378,13 @@ func (c *Core) HandleRequest(req *logical.Request) (resp *logical.Response, err
 	}
 	if auditErr := c.auditBroker.LogResponse(ctx, logInput, c.auditedHeaders); auditErr != nil {
 		c.logger.Error("failed to audit response", "request_path", req.Path, "error", auditErr)
+		cancel()
+		c.stateLock.RUnlock()
 		return nil, ErrInternalError
 	}
 
+	cancel()
+	c.stateLock.RUnlock()
 	return
 }
 

vault/router.go

@@ -209,9 +209,9 @@ func (r *Router) MatchingMountByUUID(mountID string) *MountEntry {
 	}
 
 	r.l.RLock()
-	defer r.l.RUnlock()
-
 	_, raw, ok := r.mountUUIDCache.LongestPrefix(mountID)
+	r.l.RUnlock()
+
 	if !ok {
 		return nil
 	}
@@ -226,9 +226,9 @@ func (r *Router) MatchingMountByAccessor(mountAccessor string) *MountEntry {
 	}
 
 	r.l.RLock()
-	defer r.l.RUnlock()
-
 	_, raw, ok := r.mountAccessorCache.LongestPrefix(mountAccessor)
+	r.l.RUnlock()
+
 	if !ok {
 		return nil
 	}
@@ -239,8 +239,8 @@ func (r *Router) MatchingMountByAccessor(mountAccessor string) *MountEntry {
 // MatchingMount returns the mount prefix that would be used for a path
 func (r *Router) MatchingMount(path string) string {
 	r.l.RLock()
-	defer r.l.RUnlock()
+	mount := r.matchingMountInternal(path)
-	var mount = r.matchingMountInternal(path)
+	r.l.RUnlock()
 	return mount
 }
 
@@ -406,10 +406,10 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 	// Grab a read lock on the route entry, this protects against the backend
 	// being reloaded during a request.
 	re.l.RLock()
-	defer re.l.RUnlock()
 
 	// Filtered mounts will have a nil backend
 	if re.backend == nil {
+		re.l.RUnlock()
 		return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath
 	}
 
@@ -419,6 +419,7 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 		switch req.Operation {
 		case logical.RevokeOperation, logical.RollbackOperation:
 		default:
+			re.l.RUnlock()
 			return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath
 		}
 	}
@@ -448,6 +449,7 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 		// salted ID, so we double-salt what's going to the cubbyhole backend
 		salt, err := r.tokenStoreSaltFunc(ctx)
 		if err != nil {
+			re.l.RUnlock()
 			return nil, false, false, err
 		}
 		req.ClientToken = re.SaltID(salt.SaltID(req.ClientToken))
@@ -489,7 +491,7 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 	req.SetTokenEntry(nil)
 
 	// Reset the request before returning
-	defer func() {
+	resetFunc := func() {
 		req.Path = originalPath
 		req.MountPoint = mount
 		req.MountType = re.mountEntry.Type
@@ -511,11 +513,13 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 		req.EntityID = originalEntityID
 
 		req.SetTokenEntry(reqTokenEntry)
-	}()
+	}
 
 	// Invoke the backend
 	if existenceCheck {
 		ok, exists, err := re.backend.HandleExistenceCheck(ctx, req)
+		resetFunc()
+		re.l.RUnlock()
 		return nil, ok, exists, err
 	} else {
 		resp, err := re.backend.HandleRequest(ctx, req)
@@ -540,6 +544,8 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 				alias.MountAccessor = re.mountEntry.Accessor
 			}
 		}
+		resetFunc()
+		re.l.RUnlock()
 		return resp, false, false, err
 	}
 }