diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index e56bb366f..05ecbb163 100644 --- a/CHANGELOG_PENDING.md +++ b/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) diff --git a/cmd/tendermint/commands/debug/dump.go b/cmd/tendermint/commands/debug/dump.go index f742b5bff..80ca15c6b 100644 --- a/cmd/tendermint/commands/debug/dump.go +++ b/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() diff --git a/cmd/tendermint/commands/debug/kill.go b/cmd/tendermint/commands/debug/kill.go index 2e0b442f6..52defc69c 100644 --- a/cmd/tendermint/commands/debug/kill.go +++ b/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() diff --git a/cmd/tendermint/commands/lite.go b/cmd/tendermint/commands/lite.go index a63daf20f..afd049a43 100644 --- a/cmd/tendermint/commands/lite.go +++ b/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) diff --git a/lite/client/provider.go b/lite/client/provider.go index 2bcb8f731..e24dbe0e4 100644 --- a/lite/client/provider.go +++ b/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. diff --git a/lite/client/provider_test.go b/lite/client/provider_test.go index ce8f76e38..1dccdd172 100644 --- a/lite/client/provider_test.go +++ b/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) diff --git a/lite2/provider/http/http.go b/lite2/provider/http/http.go index 242da0b71..53bebc8c2 100644 --- a/lite2/provider/http/http.go +++ b/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. diff --git a/lite2/provider/http/http_test.go b/lite2/provider/http/http_test.go index 991f92aea..1e5f4cb2b 100644 --- a/lite2/provider/http/http_test.go +++ b/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 diff --git a/rpc/client/examples_test.go b/rpc/client/examples_test.go index e65d83ad0..a543de70d 100644 --- a/rpc/client/examples_test.go +++ b/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") diff --git a/rpc/client/httpclient.go b/rpc/client/httpclient.go index 5b0703529..29175fe19 100644 --- a/rpc/client/httpclient.go +++ b/rpc/client/httpclient.go @@ -87,29 +87,38 @@ var _ rpcClient = (*baseRPCClient)(nil) // NewHTTP takes a remote endpoint in the form ://: and // the websocket path (which always seems to be "/websocket") -// The function panics if the provided remote is invalid. -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 } diff --git a/rpc/client/rpc_test.go b/rpc/client/rpc_test.go index e0dba3f56..3cab1b6af 100644 --- a/rpc/client/rpc_test.go +++ b/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) diff --git a/rpc/lib/client/http_json_client.go b/rpc/lib/client/http_json_client.go index d828a3aa6..788564cd6 100644 --- a/rpc/lib/client/http_json_client.go +++ b/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 } diff --git a/rpc/lib/client/http_json_client_test.go b/rpc/lib/client/http_json_client_test.go index b74425a54..840de6367 100644 --- a/rpc/lib/client/http_json_client_test.go +++ b/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) diff --git a/rpc/lib/client/http_uri_client.go b/rpc/lib/client/http_uri_client.go index 8efbd6608..99f8b08a5 100644 --- a/rpc/lib/client/http_uri_client.go +++ b/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. diff --git a/rpc/lib/client/integration_test.go b/rpc/lib/client/integration_test.go index c462d6002..393783c51 100644 --- a/rpc/lib/client/integration_test.go +++ b/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 } diff --git a/rpc/lib/client/ws_client.go b/rpc/lib/client/ws_client.go index 06f7a2469..eff9c300e 100644 --- a/rpc/lib/client/ws_client.go +++ b/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. diff --git a/rpc/lib/client/ws_client_test.go b/rpc/lib/client/ws_client_test.go index 67dfd418d..f0f40dec8 100644 --- a/rpc/lib/client/ws_client_test.go +++ b/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 diff --git a/rpc/lib/rpc_test.go b/rpc/lib/rpc_test.go index 8bbaef717..fbf041f57 100644 --- a/rpc/lib/rpc_test.go +++ b/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() diff --git a/rpc/test/helpers.go b/rpc/test/helpers.go index 32c465c2e..0b25b2e5f 100644 --- a/rpc/test/helpers.go +++ b/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 {