From 557d86316bbfa5de9986e877113416fe8da5ba68 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Thu, 27 Jan 2022 11:49:08 -0800 Subject: [PATCH] rpc: clean up unused non-default websocket client options (#7713) These are only ever used with the defaults, except in our own tests. A search of cs.github.com shows no other callers. The use in the test was solely to bug out the go-metrics package so its goroutines don't trigger the leak checker. Use the package's own flag for that purpose instead. Note that calling "Stop" on the metric helps, but is not sufficient -- the Stop does not wait for its goroutine to exit. --- CHANGELOG_PENDING.md | 4 ++- rpc/client/http/http.go | 16 ++------- rpc/client/http/ws.go | 46 ++------------------------ rpc/jsonrpc/client/http_json_client.go | 4 +-- rpc/jsonrpc/client/ws_client.go | 43 ++++++++---------------- rpc/jsonrpc/client/ws_client_test.go | 14 +++++--- rpc/jsonrpc/jsonrpc_test.go | 12 ++----- 7 files changed, 37 insertions(+), 102 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index c693cb655..6073ec802 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -12,10 +12,11 @@ Special thanks to external contributors on this release: - CLI/RPC/Config - - [rpc] \#7575 Rework how RPC responses are written back via HTTP. (@creachadair) - [rpc] \#7121 Remove the deprecated gRPC interface to the RPC service. (@creachadair) - [blocksync] \#7159 Remove support for disabling blocksync in any circumstance. (@tychoish) - [mempool] \#7171 Remove legacy mempool implementation. (@tychoish) + - [rpc] \#7575 Rework how RPC responses are written back via HTTP. (@creachadair) + - [rpc] \#7713 Remove unused options for websocket clients. (@creachadair) - Apps @@ -58,6 +59,7 @@ Special thanks to external contributors on this release: - [consensus] \#7711 Use the proposer timestamp for the first height instead of the genesis time. Chains will still start consensus at the genesis time. (@anca) ### IMPROVEMENTS + - [internal/protoio] \#7325 Optimized `MarshalDelimited` by inlining the common case and using a `sync.Pool` in the worst case. (@odeke-em) - [consensus] \#6969 remove logic to 'unlock' a locked block. - [pubsub] \#7319 Performance improvements for the event query API (@creachadair) diff --git a/rpc/client/http/http.go b/rpc/client/http/http.go index 523253523..ebdc18eb2 100644 --- a/rpc/client/http/http.go +++ b/rpc/client/http/http.go @@ -122,19 +122,9 @@ func NewWithTimeout(remote string, t time.Duration) (*HTTP, error) { return NewWithClient(remote, c) } -// NewWithClient allows you to set a custom http client. An error is returned -// on invalid remote. The function returns an error when client is nil -// or an invalid remote. +// NewWithClient constructs an RPC client using a custom HTTP client. +// An error is reported if c == nil or remote is an invalid address. func NewWithClient(remote string, c *http.Client) (*HTTP, error) { - if c == nil { - return nil, errors.New("nil client") - } - return NewWithClientAndWSOptions(remote, c, DefaultWSOptions()) -} - -// NewWithClientAndWSOptions allows you to set a custom http client and -// WebSocket options. An error is returned on invalid remote or nil client. -func NewWithClientAndWSOptions(remote string, c *http.Client, wso WSOptions) (*HTTP, error) { if c == nil { return nil, errors.New("nil client") } @@ -143,7 +133,7 @@ func NewWithClientAndWSOptions(remote string, c *http.Client, wso WSOptions) (*H return nil, err } - wsEvents, err := newWsEvents(remote, wso) + wsEvents, err := newWsEvents(remote) if err != nil { return nil, err } diff --git a/rpc/client/http/ws.go b/rpc/client/http/ws.go index 166a6e990..c36ad9fc6 100644 --- a/rpc/client/http/ws.go +++ b/rpc/client/http/ws.go @@ -3,7 +3,6 @@ package http import ( "context" "encoding/json" - "errors" "fmt" "strings" "sync" @@ -15,34 +14,6 @@ import ( jsonrpcclient "github.com/tendermint/tendermint/rpc/jsonrpc/client" ) -// WSOptions for the WS part of the HTTP client. -type WSOptions struct { - Path string // path (e.g. "/ws") - - jsonrpcclient.WSOptions // WSClient options -} - -// DefaultWSOptions returns default WS options. -// See jsonrpcclient.DefaultWSOptions. -func DefaultWSOptions() WSOptions { - return WSOptions{ - Path: "/websocket", - WSOptions: jsonrpcclient.DefaultWSOptions(), - } -} - -// Validate performs a basic validation of WSOptions. -func (wso WSOptions) Validate() error { - if len(wso.Path) <= 1 { - return errors.New("empty Path") - } - if wso.Path[0] != '/' { - return errors.New("leading slash is missing in Path") - } - - return nil -} - // wsEvents is a wrapper around WSClient, which implements EventsClient. type wsEvents struct { *rpcclient.RunState @@ -60,25 +31,14 @@ type wsSubscription struct { var _ rpcclient.EventsClient = (*wsEvents)(nil) -func newWsEvents(remote string, wso WSOptions) (*wsEvents, error) { - // validate options - if err := wso.Validate(); err != nil { - return nil, fmt.Errorf("invalid WSOptions: %w", err) - } - - // remove the trailing / from the remote else the websocket endpoint - // won't parse correctly - if remote[len(remote)-1] == '/' { - remote = remote[:len(remote)-1] - } - +func newWsEvents(remote string) (*wsEvents, error) { w := &wsEvents{ + RunState: rpcclient.NewRunState("wsEvents", nil), subscriptions: make(map[string]*wsSubscription), } - w.RunState = rpcclient.NewRunState("wsEvents", nil) var err error - w.ws, err = jsonrpcclient.NewWSWithOptions(remote, wso.Path, wso.WSOptions) + w.ws, err = jsonrpcclient.NewWS(strings.TrimSuffix(remote, "/"), "/websocket") if err != nil { return nil, fmt.Errorf("can't create WS client: %w", err) } diff --git a/rpc/jsonrpc/client/http_json_client.go b/rpc/jsonrpc/client/http_json_client.go index b2823d5e7..07f739cf0 100644 --- a/rpc/jsonrpc/client/http_json_client.go +++ b/rpc/jsonrpc/client/http_json_client.go @@ -150,8 +150,8 @@ func New(remote string) (*Client, error) { } // NewWithHTTPClient returns a Client pointed at the given address using a -// custom http client. An error is returned on invalid remote. The function -// panics when client is nil. +// custom HTTP client. It reports an error if c == nil or if remote is not a +// valid URL. func NewWithHTTPClient(remote string, c *http.Client) (*Client, error) { if c == nil { return nil, errors.New("nil client") diff --git a/rpc/jsonrpc/client/ws_client.go b/rpc/jsonrpc/client/ws_client.go index 8be59737a..8e232eb25 100644 --- a/rpc/jsonrpc/client/ws_client.go +++ b/rpc/jsonrpc/client/ws_client.go @@ -17,23 +17,20 @@ import ( rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) -// WSOptions for WSClient. -type WSOptions struct { +// wsOptions carries optional settings for a websocket connection. +type wsOptions struct { MaxReconnectAttempts uint // maximum attempts to reconnect ReadWait time.Duration // deadline for any read op WriteWait time.Duration // deadline for any write op PingPeriod time.Duration // frequency with which pings are sent - SkipMetrics bool // do not keep metrics for ping/pong latency } -// DefaultWSOptions returns default WS options. -func DefaultWSOptions() WSOptions { - return WSOptions{ - MaxReconnectAttempts: 10, // first: 2 sec, last: 17 min. - WriteWait: 10 * time.Second, - ReadWait: 0, - PingPeriod: 0, - } +// defaultWSOptions are the default websocket connection settings. +var defaultWSOptions = wsOptions{ + MaxReconnectAttempts: 10, // first: 2 sec, last: 17 min. + WriteWait: 10 * time.Second, + ReadWait: 0, + PingPeriod: 0, } // WSClient is a JSON-RPC client, which uses WebSocket for communication with @@ -89,15 +86,10 @@ type WSClient struct { // nolint: maligned PingPongLatencyTimer metrics.Timer } -// NewWS returns a new client. The endpoint argument must begin with a `/`. An -// error is returned on invalid remote. -// It uses DefaultWSOptions. +// NewWS returns a new client with default options. The endpoint argument must +// begin with a `/`. An error is returned on invalid remote. func NewWS(remoteAddr, endpoint string) (*WSClient, error) { - return NewWSWithOptions(remoteAddr, endpoint, DefaultWSOptions()) -} - -// NewWSWithOptions allows you to provide custom WSOptions. -func NewWSWithOptions(remoteAddr, endpoint string, opts WSOptions) (*WSClient, error) { + opts := defaultWSOptions parsedURL, err := newParsedURL(remoteAddr) if err != nil { return nil, err @@ -126,13 +118,7 @@ func NewWSWithOptions(remoteAddr, endpoint string, opts WSOptions) (*WSClient, e // sentIDs: make(map[types.JSONRPCIntID]bool), } - switch opts.SkipMetrics { - case true: - c.PingPongLatencyTimer = metrics.NilTimer{} - case false: - c.PingPongLatencyTimer = metrics.NewTimer() - } - + c.PingPongLatencyTimer = metrics.NewTimer() return c, nil } @@ -182,6 +168,7 @@ func (c *WSClient) Stop() error { // only close user-facing channels when we can't write to them c.wg.Wait() + c.PingPongLatencyTimer.Stop() close(c.ResponsesCh) return nil @@ -430,10 +417,6 @@ func (c *WSClient) writeRoutine(ctx context.Context) { func (c *WSClient) readRoutine(ctx context.Context) { defer func() { c.conn.Close() - // err != nil { - // ignore error; it will trigger in tests - // likely because it's closing an already closed connection - // } c.wg.Done() }() diff --git a/rpc/jsonrpc/client/ws_client_test.go b/rpc/jsonrpc/client/ws_client_test.go index 31cd06743..5a74210ce 100644 --- a/rpc/jsonrpc/client/ws_client_test.go +++ b/rpc/jsonrpc/client/ws_client_test.go @@ -12,13 +12,21 @@ import ( "github.com/fortytw2/leaktest" "github.com/gorilla/websocket" + metrics "github.com/rcrowley/go-metrics" "github.com/stretchr/testify/require" "github.com/tendermint/tendermint/libs/log" rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) -var wsCallTimeout = 5 * time.Second +func init() { + // Disable go-metrics metrics in tests, since they start unsupervised + // goroutines that trip the leak tester. Calling Stop on the metric is not + // sufficient, as that does not wait for the goroutine. + metrics.UseNilMetrics = true +} + +const wsCallTimeout = 5 * time.Second type myTestHandler struct { closeConnAfterRead bool @@ -223,9 +231,7 @@ func startClient(ctx context.Context, t *testing.T, addr string) *WSClient { t.Cleanup(leaktest.Check(t)) - opts := DefaultWSOptions() - opts.SkipMetrics = true - c, err := NewWSWithOptions(addr, "/websocket", opts) + c, err := NewWS(addr, "/websocket") require.NoError(t, err) err = c.Start(ctx) diff --git a/rpc/jsonrpc/jsonrpc_test.go b/rpc/jsonrpc/jsonrpc_test.go index b167e91ec..9c34f9fae 100644 --- a/rpc/jsonrpc/jsonrpc_test.go +++ b/rpc/jsonrpc/jsonrpc_test.go @@ -287,9 +287,7 @@ func TestServersAndClientsBasic(t *testing.T) { fmt.Printf("=== testing server on %s using JSONRPC client", addr) testWithHTTPClient(ctx, t, cl2) - opts := client.DefaultWSOptions() - opts.SkipMetrics = true - cl3, err := client.NewWSWithOptions(addr, websocketEndpoint, opts) + cl3, err := client.NewWS(addr, websocketEndpoint) require.NoError(t, err) cl3.Logger = logger err = cl3.Start(ctx) @@ -307,9 +305,7 @@ func TestWSNewWSRPCFunc(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - opts := client.DefaultWSOptions() - opts.SkipMetrics = true - cl, err := client.NewWSWithOptions(tcpAddr, websocketEndpoint, opts) + cl, err := client.NewWS(tcpAddr, websocketEndpoint) require.NoError(t, err) cl.Logger = log.NewNopLogger() err = cl.Start(ctx) @@ -346,9 +342,7 @@ func TestWSClientPingPong(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - opts := client.DefaultWSOptions() - opts.SkipMetrics = true - cl, err := client.NewWSWithOptions(tcpAddr, websocketEndpoint, opts) + cl, err := client.NewWS(tcpAddr, websocketEndpoint) require.NoError(t, err) cl.Logger = log.NewNopLogger() err = cl.Start(ctx)