Remove context lock from API VM interface (#2165)

Author: StephenButtolph

api/server/middleware_handler.go

@@ -5,13 +5,11 @@ package server
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net"
 	"net/http"
 	"net/url"
 	"path"
-	"sync"
 	"time"
 
 	"github.com/NYTimes/gziphandler"
@@ -39,15 +37,13 @@ const (
 )
 
 var (
-	errUnknownLockOption = errors.New("invalid lock options")
-
 	_ PathAdder = readPathAdder{}
 	_ Server    = (*server)(nil)
 )
 
 type PathAdder interface {
 	// AddRoute registers a route to a handler.
-	AddRoute(handler *common.HTTPHandler, lock *sync.RWMutex, base, endpoint string) error
+	AddRoute(handler http.Handler, base, endpoint string) error
 
 	// AddAliases registers aliases to the server
 	AddAliases(endpoint string, aliases ...string) error
@@ -56,7 +52,7 @@ type PathAdder interface {
 type PathAdderWithReadLock interface {
 	// AddRouteWithReadLock registers a route to a handler assuming the http
 	// read lock is currently held.
-	AddRouteWithReadLock(handler *common.HTTPHandler, lock *sync.RWMutex, base, endpoint string) error
+	AddRouteWithReadLock(handler http.Handler, base, endpoint string) error
 
 	// AddAliasesWithReadLock registers aliases to the server assuming the http read
 	// lock is currently held.
@@ -182,13 +178,8 @@ func (s *server) Dispatch() error {
 }
 
 func (s *server) RegisterChain(chainName string, ctx *snow.ConsensusContext, vm common.VM) {
-	var (
-		handlers map[string]*common.HTTPHandler
-		err      error
-	)
-
 	ctx.Lock.Lock()
-	handlers, err = vm.CreateHandlers(context.TODO())
+	handlers, err := vm.CreateHandlers(context.TODO())
 	ctx.Lock.Unlock()
 	if err != nil {
 		s.log.Error("failed to create handlers",
@@ -224,112 +215,44 @@ func (s *server) RegisterChain(chainName string, ctx *snow.ConsensusContext, vm
 	}
 }
 
-func (s *server) addChainRoute(chainName string, handler *common.HTTPHandler, ctx *snow.ConsensusContext, base, endpoint string) error {
+func (s *server) addChainRoute(chainName string, handler http.Handler, ctx *snow.ConsensusContext, base, endpoint string) error {
 	url := fmt.Sprintf("%s/%s", baseURL, base)
 	s.log.Info("adding route",
 		zap.String("url", url),
 		zap.String("endpoint", endpoint),
 	)
 	if s.tracingEnabled {
-		handler = &common.HTTPHandler{
-			LockOptions: handler.LockOptions,
-			Handler:     api.TraceHandler(handler.Handler, chainName, s.tracer),
-		}
-	}
-	// Apply middleware to grab/release chain's lock before/after calling API method
-	h, err := lockMiddleware(
-		handler.Handler,
-		handler.LockOptions,
-		s.tracingEnabled,
-		s.tracer,
-		&ctx.Lock,
-	)
-	if err != nil {
-		return err
+		handler = api.TraceHandler(handler, chainName, s.tracer)
 	}
 	// Apply middleware to reject calls to the handler before the chain finishes bootstrapping
-	h = rejectMiddleware(h, ctx)
-	h = s.metrics.wrapHandler(chainName, h)
-	return s.router.AddRouter(url, endpoint, h)
+	handler = rejectMiddleware(handler, ctx)
+	handler = s.metrics.wrapHandler(chainName, handler)
+	return s.router.AddRouter(url, endpoint, handler)
 }
 
-func (s *server) AddRoute(handler *common.HTTPHandler, lock *sync.RWMutex, base, endpoint string) error {
-	return s.addRoute(handler, lock, base, endpoint)
+func (s *server) AddRoute(handler http.Handler, base, endpoint string) error {
+	return s.addRoute(handler, base, endpoint)
 }
 
-func (s *server) AddRouteWithReadLock(handler *common.HTTPHandler, lock *sync.RWMutex, base, endpoint string) error {
+func (s *server) AddRouteWithReadLock(handler http.Handler, base, endpoint string) error {
 	s.router.lock.RUnlock()
 	defer s.router.lock.RLock()
-	return s.addRoute(handler, lock, base, endpoint)
+	return s.addRoute(handler, base, endpoint)
 }
 
-func (s *server) addRoute(handler *common.HTTPHandler, lock *sync.RWMutex, base, endpoint string) error {
+func (s *server) addRoute(handler http.Handler, base, endpoint string) error {
 	url := fmt.Sprintf("%s/%s", baseURL, base)
 	s.log.Info("adding route",
 		zap.String("url", url),
 		zap.String("endpoint", endpoint),
 	)
 
 	if s.tracingEnabled {
-		handler = &common.HTTPHandler{
-			LockOptions: handler.LockOptions,
-			Handler:     api.TraceHandler(handler.Handler, url, s.tracer),
-		}
-	}
-
-	// Apply middleware to grab/release chain's lock before/after calling API method
-	h, err := lockMiddleware(
-		handler.Handler,
-		handler.LockOptions,
-		s.tracingEnabled,
-		s.tracer,
-		lock,
-	)
-	if err != nil {
-		return err
-	}
-	h = s.metrics.wrapHandler(base, h)
-	return s.router.AddRouter(url, endpoint, h)
-}
-
-// Wraps a handler by grabbing and releasing a lock before calling the handler.
-func lockMiddleware(
-	handler http.Handler,
-	lockOption common.LockOption,
-	tracingEnabled bool,
-	tracer trace.Tracer,
-	lock *sync.RWMutex,
-) (http.Handler, error) {
-	var (
-		name          string
-		lockedHandler http.Handler
-	)
-	switch lockOption {
-	case common.WriteLock:
-		name = "writeLock"
-		lockedHandler = middlewareHandler{
-			before:  lock.Lock,
-			after:   lock.Unlock,
-			handler: handler,
-		}
-	case common.ReadLock:
-		name = "readLock"
-		lockedHandler = middlewareHandler{
-			before:  lock.RLock,
-			after:   lock.RUnlock,
-			handler: handler,
-		}
-	case common.NoLock:
-		return handler, nil
-	default:
-		return nil, errUnknownLockOption
-	}
-
-	if !tracingEnabled {
-		return lockedHandler, nil
+		handler = api.TraceHandler(handler, url, s.tracer)
 	}
 
-	return api.TraceHandler(lockedHandler, name, tracer), nil
+	handler = s.metrics.wrapHandler(base, handler)
+	return s.router.AddRouter(url, endpoint, handler)
 }
 
 // Reject middleware wraps a handler. If the chain that the context describes is
@@ -383,8 +306,8 @@ func PathWriterFromWithReadLock(pather PathAdderWithReadLock) PathAdder {
 	}
 }
 
-func (a readPathAdder) AddRoute(handler *common.HTTPHandler, lock *sync.RWMutex, base, endpoint string) error {
-	return a.pather.AddRouteWithReadLock(handler, lock, base, endpoint)
+func (a readPathAdder) AddRoute(handler http.Handler, base, endpoint string) error {
+	return a.pather.AddRouteWithReadLock(handler, base, endpoint)
 }
 
 func (a readPathAdder) AddAliases(endpoint string, aliases ...string) error {

api/server/mock_server.go

@@ -11,7 +11,7 @@ require (
 	github.com/DataDog/zstd v1.5.2
 	github.com/Microsoft/go-winio v0.5.2
 	github.com/NYTimes/gziphandler v1.1.1
-	github.com/ava-labs/coreth v0.12.5-rc.6
+	github.com/ava-labs/coreth v0.12.6-rc.0
 	github.com/ava-labs/ledger-avalanche/go v0.0.0-20230105152938-00a24d05a8c7
 	github.com/btcsuite/btcd/btcutil v1.1.3
 	github.com/decred/dcrd/dcrec/secp256k1/v4 v4.1.0

api/server/server.go

@@ -61,8 +61,8 @@ github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156 h1:eMwmnE/GDgah
 github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156/go.mod h1:Cb/ax3seSYIx7SuZdm2G2xzfwmv3TPSk2ucNfQESPXM=
 github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kdvxnR2qWY=
 github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
-github.com/ava-labs/coreth v0.12.5-rc.6 h1:OajGUyKkO5Q82XSuMa8T5UD6QywtCHUiZ4Tv3RFmRBU=
-github.com/ava-labs/coreth v0.12.5-rc.6/go.mod h1:s5wVyy+5UCCk2m0Tq3jVmy0UqOpKBDYqRE13gInCJVs=
+github.com/ava-labs/coreth v0.12.6-rc.0 h1:EaARpccHwa7+P60EXQ/dYUCetZmJjw9RO5L7ebutqXk=
+github.com/ava-labs/coreth v0.12.6-rc.0/go.mod h1:sNbwitXv4AhLvWpSqy6V8yzkhGFeWBQFD31/xiRDJ5M=
 github.com/ava-labs/ledger-avalanche/go v0.0.0-20230105152938-00a24d05a8c7 h1:EdxD90j5sClfL5Ngpz2TlnbnkNYdFPDXa0jDOjam65c=
 github.com/ava-labs/ledger-avalanche/go v0.0.0-20230105152938-00a24d05a8c7/go.mod h1:XhiXSrh90sHUbkERzaxEftCmUz53eCijshDLZ4fByVM=
 github.com/aymerick/raymond v2.0.3-0.20180322193309-b565731e1464+incompatible/go.mod h1:osfaiScAUVup+UC9Nfq76eWqDhXlp+4UYaA8uhTBO6g=

go.mod

@@ -357,8 +357,7 @@ func (i *indexer) registerChainHelper(
 		_ = index.Close()
 		return nil, err
 	}
-	handler := &common.HTTPHandler{LockOptions: common.NoLock, Handler: apiServer}
-	if err := i.pathAdder.AddRoute(handler, &sync.RWMutex{}, "index/"+name, "/"+endpoint); err != nil {
+	if err := i.pathAdder.AddRoute(apiServer, "index/"+name, "/"+endpoint); err != nil {
 		_ = index.Close()
 		return nil, err
 	}

go.sum

@@ -5,6 +5,7 @@ package indexer
 
 import (
 	"errors"
+	"net/http"
 	"sync"
 	"testing"
 	"time"
@@ -19,7 +20,6 @@ import (
 	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/snow"
 	"github.com/ava-labs/avalanchego/snow/engine/avalanche/vertex"
-	"github.com/ava-labs/avalanchego/snow/engine/common"
 	"github.com/ava-labs/avalanchego/snow/engine/snowman/block/mocks"
 	"github.com/ava-labs/avalanchego/utils"
 	"github.com/ava-labs/avalanchego/utils/logging"
@@ -37,7 +37,7 @@ type apiServerMock struct {
 	endpoints   []string
 }
 
-func (a *apiServerMock) AddRoute(_ *common.HTTPHandler, _ *sync.RWMutex, base, endpoint string) error {
+func (a *apiServerMock) AddRoute(_ http.Handler, base, endpoint string) error {
 	a.timesCalled++
 	a.bases = append(a.bases, base)
 	a.endpoints = append(a.endpoints, endpoint)