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)