rpc: modify New* functions to return error (#4274)

The New* client functions return an error instead
of panicking when the remote address is invalid.

Fixes #3953

CHANGELOG_PENDING.md

@@ -65,6 +65,7 @@ program](https://hackerone.com/tendermint).
 
 - Go API
 
+  - [rpc] \#3953 Modify NewHTTP, NewXXXClient functions to return an error on invalid remote instead of panicking (@mrekucci)
   - [rpc/client] \#3471 `Validators` now requires two more args: `page` and `perPage`
   - [libs/common] \#3262 Make error the last parameter of `Task` (@PSalant726)
   - [cs/types] \#3262 Rename `GotVoteFromUnwantedRoundError` to `ErrGotVoteFromUnwantedRound` (@PSalant726)

cmd/tendermint/commands/debug/dump.go

@@ -58,7 +58,10 @@ func dumpCmdHandler(_ *cobra.Command, args []string) error {
 		}
 	}
 
-	rpc := rpcclient.NewHTTP(nodeRPCAddr, "/websocket")
+	rpc, err := rpcclient.NewHTTP(nodeRPCAddr, "/websocket")
+	if err != nil {
+		return errors.Wrap(err, "failed to create new http client")
+	}
 
 	home := viper.GetString(cli.HomeFlag)
 	conf := cfg.DefaultConfig()

cmd/tendermint/commands/debug/kill.go

@@ -44,7 +44,10 @@ func killCmdHandler(cmd *cobra.Command, args []string) error {
 		return errors.New("invalid output file")
 	}
 
-	rpc := rpcclient.NewHTTP(nodeRPCAddr, "/websocket")
+	rpc, err := rpcclient.NewHTTP(nodeRPCAddr, "/websocket")
+	if err != nil {
+		return errors.Wrap(err, "failed to create new http client")
+	}
 
 	home := viper.GetString(cli.HomeFlag)
 	conf := cfg.DefaultConfig()

cmd/tendermint/commands/lite.go

@@ -80,7 +80,10 @@ func runProxy(cmd *cobra.Command, args []string) error {
 
 	// First, connect a client
 	logger.Info("Connecting to source HTTP client...")
-	node := rpcclient.NewHTTP(nodeAddr, "/websocket")
+	node, err := rpcclient.NewHTTP(nodeAddr, "/websocket")
+	if err != nil {
+		return errors.Wrap(err, "new HTTP client")
+	}
 
 	logger.Info("Constructing Verifier...")
 	cert, err := proxy.NewVerifier(chainID, home, node, logger, cacheSize)

lite/client/provider.go

@@ -39,8 +39,12 @@ func NewProvider(chainID string, client SignStatusClient) lite.Provider {
 
 // NewHTTPProvider can connect to a tendermint json-rpc endpoint
 // at the given url, and uses that as a read-only provider.
-func NewHTTPProvider(chainID, remote string) lite.Provider {
-	return NewProvider(chainID, rpcclient.NewHTTP(remote, "/websocket"))
+func NewHTTPProvider(chainID, remote string) (lite.Provider, error) {
+	httpClient, err := rpcclient.NewHTTP(remote, "/websocket")
+	if err != nil {
+		return nil, err
+	}
+	return NewProvider(chainID, httpClient), nil
 }
 
 // Implements Provider.

lite/client/provider_test.go

@@ -35,8 +35,9 @@ func TestProvider(t *testing.T) {
 	}
 	chainID := genDoc.ChainID
 	t.Log("chainID:", chainID)
-	p := NewHTTPProvider(chainID, rpcAddr)
-	require.NotNil(t, p)
+	p, err := NewHTTPProvider(chainID, rpcAddr)
+	require.Nil(err)
+	require.NotNil(p)
 
 	// let it produce some blocks
 	err = rpcclient.WaitForHeight(p.(*provider).client, 6, nil)

lite2/provider/http/http.go

@@ -23,8 +23,12 @@ type http struct {
 
 // New creates a HTTP provider, which is using the rpcclient.HTTP
 // client under the hood.
-func New(chainID, remote string) provider.Provider {
-	return NewWithClient(chainID, rpcclient.NewHTTP(remote, "/websocket"))
+func New(chainID, remote string) (provider.Provider, error) {
+	httpClient, err := rpcclient.NewHTTP(remote, "/websocket")
+	if err != nil {
+		return nil, err
+	}
+	return NewWithClient(chainID, httpClient), nil
 }
 
 // NewWithClient allows you to provide custom SignStatusClient.

lite2/provider/http/http_test.go

@@ -33,7 +33,8 @@ func TestProvider(t *testing.T) {
 	}
 	chainID := genDoc.ChainID
 	t.Log("chainID:", chainID)
-	p := New(chainID, rpcAddr)
+	p, err := New(chainID, rpcAddr)
+	require.Nil(t, err)
 	require.NotNil(t, p)
 
 	// let it produce some blocks

rpc/client/examples_test.go

@@ -18,7 +18,10 @@ func ExampleHTTP_simple() {
 
 	// Create our RPC client
 	rpcAddr := rpctest.GetConfig().RPC.ListenAddress
-	c := client.NewHTTP(rpcAddr, "/websocket")
+	c, err := client.NewHTTP(rpcAddr, "/websocket")
+	if err != nil {
+		panic(err)
+	}
 
 	// Create a transaction
 	k := []byte("name")
@@ -68,7 +71,10 @@ func ExampleHTTP_batching() {
 
 	// Create our RPC client
 	rpcAddr := rpctest.GetConfig().RPC.ListenAddress
-	c := client.NewHTTP(rpcAddr, "/websocket")
+	c, err := client.NewHTTP(rpcAddr, "/websocket")
+	if err != nil {
+		panic(err)
+	}
 
 	// Create our two transactions
 	k1 := []byte("firstName")

rpc/client/httpclient.go

@@ -87,29 +87,38 @@ var _ rpcClient = (*baseRPCClient)(nil)
 
 // NewHTTP takes a remote endpoint in the form <protocol>://<host>:<port> and
 // the websocket path (which always seems to be "/websocket")
-// The function panics if the provided remote is invalid.<Paste>
-func NewHTTP(remote, wsEndpoint string) *HTTP {
-	httpClient := rpcclient.DefaultHTTPClient(remote)
+// An error is returned on invalid remote. The function panics when remote is nil.
+func NewHTTP(remote, wsEndpoint string) (*HTTP, error) {
+	httpClient, err := rpcclient.DefaultHTTPClient(remote)
+	if err != nil {
+		return nil, err
+	}
 	return NewHTTPWithClient(remote, wsEndpoint, httpClient)
 }
 
-// NewHTTPWithClient allows for setting a custom http client. See NewHTTP
-// The function panics if the provided client is nil or remote is invalid.
-func NewHTTPWithClient(remote, wsEndpoint string, client *http.Client) *HTTP {
+// NewHTTPWithClient allows for setting a custom http client (See NewHTTP).
+// An error is returned on invalid remote. The function panics when remote is nil.
+func NewHTTPWithClient(remote, wsEndpoint string, client *http.Client) (*HTTP, error) {
 	if client == nil {
 		panic("nil http.Client provided")
 	}
-	rc := rpcclient.NewJSONRPCClientWithHTTPClient(remote, client)
+
+	rc, err := rpcclient.NewJSONRPCClientWithHTTPClient(remote, client)
+	if err != nil {
+		return nil, err
+	}
 	cdc := rc.Codec()
 	ctypes.RegisterAmino(cdc)
 	rc.SetCodec(cdc)
 
-	return &HTTP{
+	httpClient := &HTTP{
 		rpc:           rc,
 		remote:        remote,
 		baseRPCClient: &baseRPCClient{caller: rc},
 		WSEvents:      newWSEvents(cdc, remote, wsEndpoint),
 	}
+
+	return httpClient, nil
 }
 
 var _ Client = (*HTTP)(nil)
@@ -404,15 +413,18 @@ func newWSEvents(cdc *amino.Codec, remote, endpoint string) *WSEvents {
 }
 
 // OnStart implements service.Service by starting WSClient and event loop.
-func (w *WSEvents) OnStart() error {
-	w.ws = rpcclient.NewWSClient(w.remote, w.endpoint, rpcclient.OnReconnect(func() {
+func (w *WSEvents) OnStart() (err error) {
+	w.ws, err = rpcclient.NewWSClient(w.remote, w.endpoint, rpcclient.OnReconnect(func() {
 		// resubscribe immediately
 		w.redoSubscriptionsAfter(0 * time.Second)
 	}))
+	if err != nil {
+		return err
+	}
 	w.ws.SetCodec(w.cdc)
 	w.ws.SetLogger(w.Logger)
 
-	err := w.ws.Start()
+	err = w.ws.Start()
 	if err != nil {
 		return err
 	}

rpc/client/rpc_test.go

@@ -29,7 +29,10 @@ import (
 
 func getHTTPClient() *client.HTTP {
 	rpcAddr := rpctest.GetConfig().RPC.ListenAddress
-	c := client.NewHTTP(rpcAddr, "/websocket")
+	c, err := client.NewHTTP(rpcAddr, "/websocket")
+	if err != nil {
+		panic(err)
+	}
 	c.SetLogger(log.TestingLogger())
 	return c
 }
@@ -48,16 +51,17 @@ func GetClients() []client.Client {
 
 func TestNilCustomHTTPClient(t *testing.T) {
 	require.Panics(t, func() {
-		client.NewHTTPWithClient("http://example.com", "/websocket", nil)
+		_, _ = client.NewHTTPWithClient("http://example.com", "/websocket", nil)
 	})
 	require.Panics(t, func() {
-		rpcclient.NewJSONRPCClientWithHTTPClient("http://example.com", nil)
+		_, _ = rpcclient.NewJSONRPCClientWithHTTPClient("http://example.com", nil)
 	})
 }
 
 func TestCustomHTTPClient(t *testing.T) {
 	remote := rpctest.GetConfig().RPC.ListenAddress
-	c := client.NewHTTPWithClient(remote, "/websocket", http.DefaultClient)
+	c, err := client.NewHTTPWithClient(remote, "/websocket", http.DefaultClient)
+	require.Nil(t, err)
 	status, err := c.Status()
 	require.NoError(t, err)
 	require.NotNil(t, status)

rpc/lib/client/http_json_client.go

@@ -67,27 +67,35 @@ var _ JSONRPCCaller = (*JSONRPCClient)(nil)
 var _ JSONRPCCaller = (*JSONRPCRequestBatch)(nil)
 
 // NewJSONRPCClient returns a JSONRPCClient pointed at the given address.
-func NewJSONRPCClient(remote string) *JSONRPCClient {
-	return NewJSONRPCClientWithHTTPClient(remote, DefaultHTTPClient(remote))
+// An error is returned on invalid remote. The function panics when remote is nil.
+func NewJSONRPCClient(remote string) (*JSONRPCClient, error) {
+	httpClient, err := DefaultHTTPClient(remote)
+	if err != nil {
+		return nil, err
+	}
+	return NewJSONRPCClientWithHTTPClient(remote, httpClient)
 }
 
-// NewJSONRPCClientWithHTTPClient returns a JSONRPCClient pointed at the given address using a custom http client
-// The function panics if the provided client is nil or remote is invalid.
-func NewJSONRPCClientWithHTTPClient(remote string, client *http.Client) *JSONRPCClient {
+// NewJSONRPCClientWithHTTPClient returns a JSONRPCClient pointed at the given
+// address using a custom http client. An error is returned on invalid remote.
+// The function panics when remote is nil.
+func NewJSONRPCClientWithHTTPClient(remote string, client *http.Client) (*JSONRPCClient, error) {
 	if client == nil {
 		panic("nil http.Client provided")
 	}
 
 	clientAddress, err := toClientAddress(remote)
 	if err != nil {
-		panic(fmt.Sprintf("invalid remote %s: %s", remote, err))
+		return nil, fmt.Errorf("invalid remote %s: %s", remote, err)
 	}
 
-	return &JSONRPCClient{
+	rpcClient := &JSONRPCClient{
 		address: clientAddress,
 		client:  client,
 		cdc:     amino.NewCodec(),
 	}
+
+	return rpcClient, nil
 }
 
 // Call issues a POST HTTP request. Requests are JSON encoded. Content-Type:
@@ -298,16 +306,10 @@ func parseRemoteAddr(remoteAddr string) (network string, s string, err error) {
 	return protocol, address, nil
 }
 
-func makeErrorDialer(err error) func(string, string) (net.Conn, error) {
-	return func(_ string, _ string) (net.Conn, error) {
-		return nil, err
-	}
-}
-
-func makeHTTPDialer(remoteAddr string) func(string, string) (net.Conn, error) {
+func makeHTTPDialer(remoteAddr string) (func(string, string) (net.Conn, error), error) {
 	protocol, address, err := parseRemoteAddr(remoteAddr)
 	if err != nil {
-		return makeErrorDialer(err)
+		return nil, err
 	}
 
 	// accept http(s) as an alias for tcp
@@ -316,20 +318,30 @@ func makeHTTPDialer(remoteAddr string) func(string, string) (net.Conn, error) {
 		protocol = protoTCP
 	}
 
-	return func(proto, addr string) (net.Conn, error) {
+	dialFn := func(proto, addr string) (net.Conn, error) {
 		return net.Dial(protocol, address)
 	}
+
+	return dialFn, nil
 }
 
 // DefaultHTTPClient is used to create an http client with some default parameters.
 // We overwrite the http.Client.Dial so we can do http over tcp or unix.
-// remoteAddr should be fully featured (eg. with tcp:// or unix://)
-func DefaultHTTPClient(remoteAddr string) *http.Client {
-	return &http.Client{
+// remoteAddr should be fully featured (eg. with tcp:// or unix://).
+// An error will be returned in case of invalid remoteAddr.
+func DefaultHTTPClient(remoteAddr string) (*http.Client, error) {
+	dialFn, err := makeHTTPDialer(remoteAddr)
+	if err != nil {
+		return nil, err
+	}
+
+	client := &http.Client{
 		Transport: &http.Transport{
 			// Set to true to prevent GZIP-bomb DoS attacks
 			DisableCompression: true,
-			Dial:               makeHTTPDialer(remoteAddr),
+			Dial:               dialFn,
 		},
 	}
+
+	return client, nil
 }

rpc/lib/client/http_json_client_test.go

@@ -12,7 +12,8 @@ func TestHTTPClientMakeHTTPDialer(t *testing.T) {
 	for _, f := range remote {
 		protocol, address, err := parseRemoteAddr(f)
 		require.NoError(t, err)
-		dialFn := makeHTTPDialer(f)
+		dialFn, err := makeHTTPDialer(f)
+		require.Nil(t, err)
 
 		addr, err := dialFn(protocol, address)
 		require.NoError(t, err)

rpc/lib/client/http_uri_client.go

@@ -1,7 +1,6 @@
 package rpcclient
 
 import (
-	"fmt"
 	"io/ioutil"
 	"net/http"
 
@@ -33,17 +32,26 @@ type URIClient struct {
 var _ HTTPClient = (*URIClient)(nil)
 
 // NewURIClient returns a new client.
-// The function panics if the provided remote is invalid.
-func NewURIClient(remote string) *URIClient {
+// An error is returned on invalid remote.
+// The function panics when remote is nil.
+func NewURIClient(remote string) (*URIClient, error) {
 	clientAddress, err := toClientAddress(remote)
 	if err != nil {
-		panic(fmt.Sprintf("invalid remote %s: %s", remote, err))
+		return nil, err
 	}
-	return &URIClient{
+
+	httpClient, err := DefaultHTTPClient(remote)
+	if err != nil {
+		return nil, err
+	}
+
+	uriClient := &URIClient{
 		address: clientAddress,
-		client:  DefaultHTTPClient(remote),
+		client:  httpClient,
 		cdc:     amino.NewCodec(),
 	}
+
+	return uriClient, nil
 }
 
 // Call issues a POST form HTTP request.

rpc/lib/client/integration_test.go

@@ -28,7 +28,8 @@ func TestWSClientReconnectWithJitter(t *testing.T) {
 	buf := new(bytes.Buffer)
 	logger := log.NewTMLogger(buf)
 	for i := 0; i < n; i++ {
-		c := NewWSClient("tcp://foo", "/websocket")
+		c, err := NewWSClient("tcp://foo", "/websocket")
+		require.Nil(t, err)
 		c.Dialer = func(string, string) (net.Conn, error) {
 			return nil, errNotConnected
 		}

rpc/lib/client/ws_client.go

@@ -84,21 +84,26 @@ type WSClient struct { // nolint: maligned
 // NewWSClient returns a new client. See the commentary on the func(*WSClient)
 // functions for a detailed description of how to configure ping period and
 // pong wait time. The endpoint argument must begin with a `/`.
-// The function panics if the provided address is invalid.
-func NewWSClient(remoteAddr, endpoint string, options ...func(*WSClient)) *WSClient {
+// An error is returned on invalid remote. The function panics when remote is nil.
+func NewWSClient(remoteAddr, endpoint string, options ...func(*WSClient)) (*WSClient, error) {
 	protocol, addr, err := toClientAddrAndParse(remoteAddr)
 	if err != nil {
-		panic(fmt.Sprintf("invalid remote %s: %s", remoteAddr, err))
+		return nil, err
 	}
 	// default to ws protocol, unless wss is explicitly specified
 	if protocol != "wss" {
 		protocol = "ws"
 	}
 
+	dialFn, err := makeHTTPDialer(remoteAddr)
+	if err != nil {
+		return nil, err
+	}
+
 	c := &WSClient{
 		cdc:                  amino.NewCodec(),
 		Address:              addr,
-		Dialer:               makeHTTPDialer(remoteAddr),
+		Dialer:               dialFn,
 		Endpoint:             endpoint,
 		PingPongLatencyTimer: metrics.NewTimer(),
 
@@ -114,7 +119,7 @@ func NewWSClient(remoteAddr, endpoint string, options ...func(*WSClient)) *WSCli
 	for _, option := range options {
 		option(c)
 	}
-	return c
+	return c, nil
 }
 
 // MaxReconnectAttempts sets the maximum number of reconnect attempts before returning an error.

rpc/lib/client/ws_client_test.go

@@ -200,8 +200,9 @@ func TestNotBlockingOnStop(t *testing.T) {
 }
 
 func startClient(t *testing.T, addr string) *WSClient {
-	c := NewWSClient(addr, "/websocket")
-	err := c.Start()
+	c, err := NewWSClient(addr, "/websocket")
+	require.Nil(t, err)
+	err = c.Start()
 	require.Nil(t, err)
 	c.SetLogger(log.TestingLogger())
 	return c

rpc/lib/rpc_test.go

@@ -274,17 +274,20 @@ func testWithWSClient(t *testing.T, cl *client.WSClient) {
 func TestServersAndClientsBasic(t *testing.T) {
 	serverAddrs := [...]string{tcpAddr, unixAddr}
 	for _, addr := range serverAddrs {
-		cl1 := client.NewURIClient(addr)
+		cl1, err := client.NewURIClient(addr)
+		require.Nil(t, err)
 		fmt.Printf("=== testing server on %s using URI client", addr)
 		testWithHTTPClient(t, cl1)
 
-		cl2 := client.NewJSONRPCClient(addr)
+		cl2, err := client.NewJSONRPCClient(addr)
+		require.Nil(t, err)
 		fmt.Printf("=== testing server on %s using JSONRPC client", addr)
 		testWithHTTPClient(t, cl2)
 
-		cl3 := client.NewWSClient(addr, websocketEndpoint)
+		cl3, err := client.NewWSClient(addr, websocketEndpoint)
+		require.Nil(t, err)
 		cl3.SetLogger(log.TestingLogger())
-		err := cl3.Start()
+		err = cl3.Start()
 		require.Nil(t, err)
 		fmt.Printf("=== testing server on %s using WS client", addr)
 		testWithWSClient(t, cl3)
@@ -293,7 +296,8 @@ func TestServersAndClientsBasic(t *testing.T) {
 }
 
 func TestHexStringArg(t *testing.T) {
-	cl := client.NewURIClient(tcpAddr)
+	cl, err := client.NewURIClient(tcpAddr)
+	require.Nil(t, err)
 	// should NOT be handled as hex
 	val := "0xabc"
 	got, err := echoViaHTTP(cl, val)
@@ -302,7 +306,8 @@ func TestHexStringArg(t *testing.T) {
 }
 
 func TestQuotedStringArg(t *testing.T) {
-	cl := client.NewURIClient(tcpAddr)
+	cl, err := client.NewURIClient(tcpAddr)
+	require.Nil(t, err)
 	// should NOT be unquoted
 	val := "\"abc\""
 	got, err := echoViaHTTP(cl, val)
@@ -311,9 +316,10 @@ func TestQuotedStringArg(t *testing.T) {
 }
 
 func TestWSNewWSRPCFunc(t *testing.T) {
-	cl := client.NewWSClient(tcpAddr, websocketEndpoint)
+	cl, err := client.NewWSClient(tcpAddr, websocketEndpoint)
+	require.Nil(t, err)
 	cl.SetLogger(log.TestingLogger())
-	err := cl.Start()
+	err = cl.Start()
 	require.Nil(t, err)
 	defer cl.Stop()
 
@@ -336,9 +342,10 @@ func TestWSNewWSRPCFunc(t *testing.T) {
 }
 
 func TestWSHandlesArrayParams(t *testing.T) {
-	cl := client.NewWSClient(tcpAddr, websocketEndpoint)
+	cl, err := client.NewWSClient(tcpAddr, websocketEndpoint)
+	require.Nil(t, err)
 	cl.SetLogger(log.TestingLogger())
-	err := cl.Start()
+	err = cl.Start()
 	require.Nil(t, err)
 	defer cl.Stop()
 
@@ -361,9 +368,10 @@ func TestWSHandlesArrayParams(t *testing.T) {
 // TestWSClientPingPong checks that a client & server exchange pings
 // & pongs so connection stays alive.
 func TestWSClientPingPong(t *testing.T) {
-	cl := client.NewWSClient(tcpAddr, websocketEndpoint)
+	cl, err := client.NewWSClient(tcpAddr, websocketEndpoint)
+	require.Nil(t, err)
 	cl.SetLogger(log.TestingLogger())
-	err := cl.Start()
+	err = cl.Start()
 	require.Nil(t, err)
 	defer cl.Stop()
 

rpc/test/helpers.go

@@ -37,7 +37,10 @@ var defaultOptions = Options{
 
 func waitForRPC() {
 	laddr := GetConfig().RPC.ListenAddress
-	client := rpcclient.NewJSONRPCClient(laddr)
+	client, err := rpcclient.NewJSONRPCClient(laddr)
+	if err != nil {
+		panic(err)
+	}
 	ctypes.RegisterAmino(client.Codec())
 	result := new(ctypes.ResultStatus)
 	for {