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

@@ -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)

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
 	}

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)
 	}

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")

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()
 	}()
 

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)

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)