-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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.
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
Conversation
rpc/client_opt.go
Outdated
@@ -34,7 +34,8 @@ type clientConfig struct { | |||
httpAuth HTTPAuth | |||
|
|||
// WebSocket options | |||
wsDialer *websocket.Dialer | |||
wsDialer *websocket.Dialer | |||
wsMessageSizeLimit *int64 |
There was a problem hiding this comment.
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:
- no limit(not compatible with current version);
- or the same to
wsMessageSizeLimit
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
for no limit?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1e92c0f
to
d6a104c
Compare
I know you all are busy, but just checking if this can get merged in? (I don't have permissions, obviously). Would be great since this closes #23754 which has been bothering me for a while :) |
) This change adds a configurable limit to websocket message. --------- Co-authored-by: Martin Holst Swende <[email protected]>
) This change adds a configurable limit to websocket message. --------- Co-authored-by: Martin Holst Swende <[email protected]>
) This change adds a configurable limit to websocket message. --------- Co-authored-by: Martin Holst Swende <[email protected]>
) This change adds a configurable limit to websocket message. --------- Co-authored-by: Martin Holst Swende <[email protected]>
…ereum#27801)" This reverts commit 07bb397.
…ereum#27801)" This reverts commit 07bb397.
Currently the websocket client has a hardcoded message size.
This has caused issues in the past (ex #26883 )
Increasing the limit wasn't really sufficient to address this (ex #26967 )
There is also an active issue open to make this configurable:
#23754
Hopefully this can address these problems!