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

Jump to bottom

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

merged 7 commits into from
Oct 3, 2023

Conversation

tylerni7
Copy link
Contributor

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!

@tylerni7 tylerni7 requested review from fjl and holiman as code owners July 29, 2023 00:34
jsvisa
jsvisa reviewed Jul 29, 2023
View reviewed changes
rpc/websocket.go Outdated Show resolved Hide resolved
rpc/client_opt.go Outdated
@@ -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.

@ethereum ethereum deleted a comment Jul 31, 2023
@ethereum ethereum deleted a comment Jul 31, 2023
@ethereum ethereum deleted a comment Jul 31, 2023
@tylerni7 tylerni7 requested a review from jsvisa August 1, 2023 07:07
@tylerni7
Copy link
Contributor Author

@fjl or @jsvisa don't mean to be a bother, but any chance for a review? I'm not sure if something is blocking this being merged, and it would be really helpful to have it included :)

holiman
holiman approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@tylerni7
Copy link
Contributor Author

tylerni7 commented Oct 3, 2023

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

@holiman holiman added this to the 1.13.3 milestone Oct 3, 2023
@holiman holiman merged commit 705a51e into ethereum:master Oct 3, 2023
@holiman holiman mentioned this pull request Oct 3, 2023
Closed
hyunchel pushed a commit to hyunchel/go-ethereum that referenced this pull request Oct 6, 2023
@tylerni7 @holiman @hyunchel
eth, rpc: add configurable option for wsMessageSizeLimit (ethereum#27801
315607d 
)

This change adds a configurable limit to websocket message. 
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
@tylerni7 @holiman @tyler-smith
eth, rpc: add configurable option for wsMessageSizeLimit (ethereum#27801
0c18ce5 
)

This change adds a configurable limit to websocket message. 
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 16, 2023
@tylerni7 @holiman @tyler-smith
eth, rpc: add configurable option for wsMessageSizeLimit (ethereum#27801
a2e5805 
)

This change adds a configurable limit to websocket message. 
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@tylerni7 @holiman @devopsbo3
eth, rpc: add configurable option for wsMessageSizeLimit (ethereum#27801
07bb397 
)

This change adds a configurable limit to websocket message. 
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@devopsbo3
Revert "eth, rpc: add configurable option for wsMessageSizeLimit (eth…
3764eef 
…ereum#27801)"

This reverts commit 07bb397.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@devopsbo3
Revert "eth, rpc: add configurable option for wsMessageSizeLimit (eth…
47e1ca5 
…ereum#27801)"

This reverts commit 07bb397.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsvisa jsvisa jsvisa approved these changes

@holiman holiman holiman approved these changes

@fjl fjl Awaiting requested review from fjl fjl is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.13.3
Development

Successfully merging this pull request may close these issues.
3 participants
@tylerni7 @holiman @jsvisa