Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eth, rpc: add configurable option for wsMessageSizeLimit #27801

Merged
merged 7 commits into from
Oct 3, 2023
Merged

eth, rpc: add configurable option for wsMessageSizeLimit #27801

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e4e11f
add configurable option for wsMessageSizeLimit
tylerni7 Jul 29, 2023
53703b2
fix formatting
tylerni7 Jul 29, 2023
add89f1
don't use client config as argument for newWebsocketCodec
tylerni7 Jul 30, 2023
d72acd2
Merge branch 'master' into wsMessageSizeLimit
tylerni7 Aug 17, 2023
c814976
fix formatting issue from merging with github web interface
tylerni7 Sep 2, 2023
e28e681
add more info about behavior of 0
tylerni7 Sep 5, 2023
d6a104c
rpc: rename wsMessageSizeLimit (it's now a default), minor test-changes
holiman Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion rpc/client_opt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ type clientConfig struct {
httpAuth HTTPAuth

// WebSocket options
wsDialer *websocket.Dialer
wsDialer *websocket.Dialer
wsMessageSizeLimit *int64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think declare this variable as int64 is sufficient, default 0 means:

  1. no limit(not compatible with current version);
  2. or the same to wsMessageSizeLimit

Copy link
Contributor Author

@tylerni7 tylerni7 Jul 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ambiguity around 0 is why this is currently *int64. nil signifies nothing is set (use hard coded value), non-nil means this has been specified and should be applied. Otherwise it's unclear how to set no limit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for no limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gorilla websocket library treats 0 as no limit, so it seems cleaner to try to match that in the settings here. Is there a problem using nil as the default value (matching the current behavior)?
I can change this if you feel strongly about it...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let's add a comment to say that 0 means no limit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I added a comment on the WithWebsocketMessageSizeLimit method I created to mention 0 means no limit.


// RPC handler options
idgen func() ID
Expand Down Expand Up @@ -66,6 +67,14 @@ func WithWebsocketDialer(dialer websocket.Dialer) ClientOption {
})
}

// WithWebsocketMessageSizeLimit configures the websocket message size limit used by the RPC
// client. Passing a limit of 0 means no limit.
func WithWebsocketMessageSizeLimit(messageSizeLimit int64) ClientOption {
return optionFunc(func(cfg *clientConfig) {
cfg.wsMessageSizeLimit = &messageSizeLimit
})
}

// WithHeader configures HTTP headers set by the RPC client. Headers set using this option
// will be used for both HTTP and WebSocket connections.
func WithHeader(key, value string) ClientOption {
Expand Down
2 changes: 1 addition & 1 deletion rpc/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func TestServerRegisterName(t *testing.T) {
t.Fatalf("Expected service calc to be registered")
}

wantCallbacks := 13
wantCallbacks := 14
if len(svc.callbacks) != wantCallbacks {
t.Errorf("Expected %d callbacks for service 'service', got %d", wantCallbacks, len(svc.callbacks))
}
Expand Down
4 changes: 4 additions & 0 deletions rpc/testservice_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ func (s *testService) EchoWithCtx(ctx context.Context, str string, i int, args *
return echoResult{str, i, args}
}

func (s *testService) Repeat(msg string, i int) string {
return strings.Repeat(msg, i)
}

func (s *testService) PeerInfo(ctx context.Context) PeerInfo {
return PeerInfoFromContext(ctx)
}
Expand Down
14 changes: 10 additions & 4 deletions rpc/websocket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (s *Server) WebsocketHandler(allowedOrigins []string) http.Handler {
log.Debug("WebSocket upgrade failed", "err", err)
return
}
codec := newWebsocketCodec(conn, r.Host, r.Header)
codec := newWebsocketCodec(conn, r.Host, r.Header, wsMessageSizeLimit)
s.ServeCodec(codec, 0)
})
}
Expand Down Expand Up @@ -251,7 +251,13 @@ func newClientTransportWS(endpoint string, cfg *clientConfig) (reconnectFunc, er
}
return nil, hErr
}
return newWebsocketCodec(conn, dialURL, header), nil
var messageSizeLimit int64
if cfg.wsMessageSizeLimit != nil {
messageSizeLimit = *cfg.wsMessageSizeLimit
} else {
messageSizeLimit = wsMessageSizeLimit
}
return newWebsocketCodec(conn, dialURL, header, messageSizeLimit), nil
}
return connect, nil
}
Expand Down Expand Up @@ -283,8 +289,8 @@ type websocketCodec struct {
pongReceived chan struct{}
}

func newWebsocketCodec(conn *websocket.Conn, host string, req http.Header) ServerCodec {
conn.SetReadLimit(wsMessageSizeLimit)
func newWebsocketCodec(conn *websocket.Conn, host string, req http.Header, messageSizeLimit int64) ServerCodec {
conn.SetReadLimit(messageSizeLimit)
encode := func(v interface{}, isErrorResponse bool) error {
return conn.WriteJSON(v)
}
Expand Down
46 changes: 46 additions & 0 deletions rpc/websocket_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,52 @@ func TestWebsocketLargeCall(t *testing.T) {
}
}

// This test checks whether the wsMessageSizeLimit option is obeyed.
func TestWebsocketLargeRead(t *testing.T) {
t.Parallel()

var (
srv = newTestServer()
httpsrv = httptest.NewServer(srv.WebsocketHandler([]string{"*"}))
wsURL = "ws:" + strings.TrimPrefix(httpsrv.URL, "http:")
)
defer srv.Stop()
defer httpsrv.Close()

testLimit := func(limit int64) {
opts := []ClientOption{}
if limit >= 0 {
opts = append(opts, WithWebsocketMessageSizeLimit(limit))
} else {
limit = wsMessageSizeLimit
}
client, err := DialOptions(context.Background(), wsURL, opts...)
if err != nil {
t.Fatalf("can't dial: %v", err)
}
defer client.Close()

// Remove some bytes for json encoding overhead.
underLimit := int(limit - 128)
var res string
err = client.Call(&res, "test_repeat", "A", underLimit)
if err != nil {
t.Fatalf("unexpected error with limit %d: %v", limit, err)
}
if len(res) != underLimit || strings.Count(res, "A") != underLimit {
t.Fatal("incorrect data")
}

err = client.Call(&res, "test_repeat", "A", limit+1)
if err == nil || err != websocket.ErrReadLimit {
t.Fatalf("wrong error with limit %d: %v expecting %v", limit, err, websocket.ErrReadLimit)
}
}

testLimit(-1)
testLimit(wsMessageSizeLimit * 2)
}

func TestWebsocketPeerInfo(t *testing.T) {
var (
s = newTestServer()
Expand Down