Merge pull request #177 from 0xProject/marcin/dont-panic

Stop use panic()

internal/proxy/healthcheckmanager.go

@@ -26,7 +26,7 @@ type HealthCheckManager struct {
 	metricRPCProviderGasLimit    *prometheus.GaugeVec
 }
 
-func NewHealthCheckManager(config HealthCheckManagerConfig) *HealthCheckManager {
+func NewHealthCheckManager(config HealthCheckManagerConfig) (*HealthCheckManager, error) {
 	hcm := &HealthCheckManager{
 		logger: config.Logger,
 		metricRPCProviderInfo: promauto.NewGaugeVec(
@@ -72,15 +72,14 @@ func NewHealthCheckManager(config HealthCheckManagerConfig) *HealthCheckManager
 				FailureThreshold: config.Config.FailureThreshold,
 				SuccessThreshold: config.Config.SuccessThreshold,
 			})
-
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
 
 		hcm.hcs = append(hcm.hcs, hc)
 	}
 
-	return hcm
+	return hcm, nil
 }
 
 func (h *HealthCheckManager) runLoop(c context.Context) error {

internal/proxy/proxy.go

@@ -20,7 +20,7 @@ type Proxy struct {
 	metricRequestErrors   *prometheus.CounterVec
 }
 
-func NewProxy(config Config) *Proxy {
+func NewProxy(config Config) (*Proxy, error) {
 	proxy := &Proxy{
 		hcm:     config.HealthcheckManager,
 		timeout: config.Proxy.UpstreamTimeout,
@@ -61,16 +61,13 @@ func NewProxy(config Config) *Proxy {
 	for _, target := range config.Targets {
 		p, err := NewNodeProvider(target)
 		if err != nil {
-			// TODO
-			// Remove a call to panic()
-			//
-			panic(err)
+			return nil, err
 		}
 
 		proxy.targets = append(proxy.targets, p)
 	}
 
-	return proxy
+	return proxy, nil
 }
 
 func (p *Proxy) HasNodeProviderFailed(statusCode int) bool {

internal/proxy/proxy_test.go

@@ -66,17 +66,22 @@ func TestHttpFailoverProxyRerouteRequests(t *testing.T) {
 			},
 		},
 	}
-	healthcheckManager := NewHealthCheckManager(HealthCheckManagerConfig{
+	healthcheckManager, err := NewHealthCheckManager(HealthCheckManagerConfig{
 		Targets: rpcGatewayConfig.Targets,
 		Config:  rpcGatewayConfig.HealthChecks,
 		Logger:  slog.New(slog.NewTextHandler(os.Stderr, nil)),
 	})
+	assert.NoError(t, err)
+	assert.NotNil(t, healthcheckManager)
+
 	rpcGatewayConfig.HealthcheckManager = healthcheckManager
 
 	// Setup HttpFailoverProxy but not starting the HealthCheckManager
 	// so the no target will be tainted or marked as unhealthy by the HealthCheckManager
 	// the failoverProxy should automatically reroute the request to the second RPC Server by itself
-	httpFailoverProxy := NewProxy(rpcGatewayConfig)
+	httpFailoverProxy, err := NewProxy(rpcGatewayConfig)
+	assert.NoError(t, err)
+	assert.NotNil(t, httpFailoverProxy)
 
 	requestBody := bytes.NewBufferString(`{"this_is": "body"}`)
 	req, err := http.NewRequest(http.MethodPost, "/", requestBody)
@@ -119,20 +124,25 @@ func TestHttpFailoverProxyDecompressRequest(t *testing.T) {
 		},
 	}
 
-	healthcheckManager := NewHealthCheckManager(HealthCheckManagerConfig{
+	healthcheckManager, err := NewHealthCheckManager(HealthCheckManagerConfig{
 		Targets: rpcGatewayConfig.Targets,
 		Config:  rpcGatewayConfig.HealthChecks,
 		Logger:  slog.New(slog.NewTextHandler(os.Stderr, nil)),
 	})
+	assert.NotNil(t, healthcheckManager)
+	assert.NoError(t, err)
+
 	rpcGatewayConfig.HealthcheckManager = healthcheckManager
 	// Setup HttpFailoverProxy but not starting the HealthCheckManager
 	// so the no target will be tainted or marked as unhealthy by the HealthCheckManager
-	httpFailoverProxy := NewProxy(rpcGatewayConfig)
+	httpFailoverProxy, err := NewProxy(rpcGatewayConfig)
+	assert.NotNil(t, httpFailoverProxy)
+	assert.NoError(t, err)
 
 	var buf bytes.Buffer
 	g := gzip.NewWriter(&buf)
 
-	_, err := g.Write([]byte(`{"body": "content"}`))
+	_, err = g.Write([]byte(`{"body": "content"}`))
 	assert.NoError(t, err)
 	assert.NoError(t, g.Close())
 
@@ -175,20 +185,25 @@ func TestHttpFailoverProxyWithCompressionSupportedTarget(t *testing.T) {
 		},
 	}
 
-	healthcheckManager := NewHealthCheckManager(HealthCheckManagerConfig{
+	healthcheckManager, err := NewHealthCheckManager(HealthCheckManagerConfig{
 		Targets: rpcGatewayConfig.Targets,
 		Config:  rpcGatewayConfig.HealthChecks,
 		Logger:  slog.New(slog.NewTextHandler(os.Stderr, nil)),
 	})
+	assert.NotNil(t, healthcheckManager)
+	assert.NoError(t, err)
+
 	rpcGatewayConfig.HealthcheckManager = healthcheckManager
 	// Setup HttpFailoverProxy but not starting the HealthCheckManager
 	// so the no target will be tainted or marked as unhealthy by the HealthCheckManager
-	httpFailoverProxy := NewProxy(rpcGatewayConfig)
+	httpFailoverProxy, err := NewProxy(rpcGatewayConfig)
+	assert.NotNil(t, httpFailoverProxy)
+	assert.NoError(t, err)
 
 	var buf bytes.Buffer
 	g := gzip.NewWriter(&buf)
 
-	_, err := g.Write([]byte(`{"body": "content"}`))
+	_, err = g.Write([]byte(`{"body": "content"}`))
 	assert.NoError(t, err)
 	assert.NoError(t, g.Close())
 
@@ -241,18 +256,23 @@ func TestHTTPFailoverProxyWhenCannotConnectToPrimaryProvider(t *testing.T) {
 			},
 		},
 	}
-	healthcheckManager := NewHealthCheckManager(HealthCheckManagerConfig{
+	healthcheckManager, err := NewHealthCheckManager(HealthCheckManagerConfig{
 		Targets: rpcGatewayConfig.Targets,
 		Config:  rpcGatewayConfig.HealthChecks,
 		Logger:  slog.New(slog.NewTextHandler(os.Stderr, nil)),
 	})
+	assert.NotNil(t, healthcheckManager)
+	assert.NoError(t, err)
+
 	rpcGatewayConfig.HealthcheckManager = healthcheckManager
 	// Setup HttpFailoverProxy but not starting the HealthCheckManager so the
 	// no target will be tainted or marked as unhealthy by the
 	// HealthCheckManager the failoverProxy should automatically reroute the
 	// request to the second RPC Server by itself
 
-	httpFailoverProxy := NewProxy(rpcGatewayConfig)
+	httpFailoverProxy, err := NewProxy(rpcGatewayConfig)
+	assert.NotNil(t, httpFailoverProxy)
+	assert.NoError(t, err)
 
 	requestBody := bytes.NewBufferString(`{"this_is": "body"}`)
 	req, err := http.NewRequest(http.MethodPost, "/", requestBody)

internal/rpcgateway/rpcgateway.go

@@ -12,6 +12,7 @@ import (
 	"github.com/0xProject/rpc-gateway/internal/proxy"
 	"github.com/carlmjohnson/flowmatic"
 	"github.com/go-chi/chi/v5"
+	"github.com/go-chi/chi/v5/middleware"
 	"github.com/go-chi/httplog/v2"
 	"github.com/pkg/errors"
 	"gopkg.in/yaml.v2"
@@ -57,7 +58,7 @@ func (r *RPCGateway) Stop(c context.Context) error {
 	)
 }
 
-func NewRPCGateway(config RPCGatewayConfig) *RPCGateway {
+func NewRPCGateway(config RPCGatewayConfig) (*RPCGateway, error) {
 	logLevel := slog.LevelWarn
 	if os.Getenv("DEBUG") == "true" {
 		logLevel = slog.LevelDebug
@@ -69,7 +70,7 @@ func NewRPCGateway(config RPCGatewayConfig) *RPCGateway {
 		LogLevel:       logLevel,
 	})
 
-	hcm := proxy.NewHealthCheckManager(
+	hcm, err := proxy.NewHealthCheckManager(
 		proxy.HealthCheckManagerConfig{
 			Targets: config.Targets,
 			Config:  config.HealthChecks,
@@ -78,17 +79,31 @@ func NewRPCGateway(config RPCGatewayConfig) *RPCGateway {
 					Level: logLevel,
 				})),
 		})
-	proxy := proxy.NewProxy(
+	if err != nil {
+		return nil, errors.Wrap(err, "healthcheckmanager failed")
+	}
+
+	proxy, err := proxy.NewProxy(
 		proxy.Config{
 			Proxy:              config.Proxy,
 			Targets:            config.Targets,
 			HealthChecks:       config.HealthChecks,
 			HealthcheckManager: hcm,
 		},
 	)
+	if err != nil {
+		return nil, errors.Wrap(err, "proxy failed")
+	}
 
 	r := chi.NewRouter()
 	r.Use(httplog.RequestLogger(logger))
+
+	// Recoverer is a middleware that recovers from panics, logs the panic (and
+	// a backtrace), and returns a HTTP 500 (Internal Server Error) status if
+	// possible. Recoverer prints a request ID if one is provided.
+	//
+	r.Use(middleware.Recoverer)
+
 	r.Handle("/", proxy)
 
 	return &RPCGateway{
@@ -107,7 +122,7 @@ func NewRPCGateway(config RPCGatewayConfig) *RPCGateway {
 			ReadTimeout:       time.Second * 15,
 			ReadHeaderTimeout: time.Second * 5,
 		},
-	}
+	}, nil
 }
 
 func NewRPCGatewayFromConfigFile(path string) (*RPCGatewayConfig, error) {

internal/rpcgateway/rpcgateway_test.go

@@ -95,7 +95,10 @@ func TestRpcGatewayFailover(t *testing.T) {
 	config, err := NewRPCGatewayFromConfigString(configString)
 	assert.NoError(t, err)
 
-	gateway := NewRPCGateway(*config)
+	gateway, err := NewRPCGateway(*config)
+	assert.NotNil(t, gateway)
+	assert.NoError(t, err)
+
 	go gateway.Start(context.TODO())
 	gs := httptest.NewServer(gateway)
 

main.go

@@ -33,7 +33,10 @@ func main() {
 				return err
 			}
 
-			service := rpcgateway.NewRPCGateway(*config)
+			service, err := rpcgateway.NewRPCGateway(*config)
+			if err != nil {
+				return errors.Wrap(err, "rpc-gateway failed")
+			}
 
 			return flowmatic.Do(
 				func() error {