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.

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

Expose direct configuration of max in and out buffer capacity #594

Merged
merged 4 commits into from
Oct 5, 2020

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Oct 1, 2020

Closes #590, see issue for feature description.

If not set, max_in_buffer_capacity and max_out_buffer_capacity explicitly defaults to 10,485,760, which is the same as parity-ws Settings::default.

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Looks good, let's add a test?

ws/src/server_builder.rs Outdated Show resolved Hide resolved
@emostov
Copy link
Contributor Author

emostov commented Oct 1, 2020

@niklasad1, @maciejhirsz, @dvdplm - added some basic tests. I originally was going to assert every config value, but I could not think of a zero overhead way around the fact some of the more complex types don't support equality comparisons. It also looks like those complex types are tested implicitly in some of the higher level tests so I think that means technically they are covered.

Unrelated to the tests, I realized this PR may break the API since it changes the defaults for max_in_buffer_capacity and max_out_buffer_capacity. Previously they defaulted to 5,242,880 (the default of max_payload_bytes), now they default to 10,485,760.

@niklasad1
Copy link
Member

niklasad1 commented Oct 1, 2020

IMO, the tests are sufficient for this change

Previously they defaulted to 5,242,880 (the default of max_payload_bytes), now they default to 10,485,760

Ok, because ws-rs defaults max_buffer_i/o_size to 10MB and it is changed in this PR too but configured explicitly?

If we want to go with 10MB instead would it be reasonable to use it for max_payload_bytes too?

However, I'm not familiar with the reasoning about these limits but 5MB should cover most use-cases AFAIK but perhaps @maciejhirsz or @tomusdrw could explain the reasoning about these limits :)

@tomusdrw
Copy link
Contributor

tomusdrw commented Oct 2, 2020

However, I'm not familiar with the reasoning about these limits but 5MB should cover most use-cases AFAIK but perhaps @maciejhirsz or @tomusdrw could explain the reasoning about these limits :)

I can't remember exactly, but wasn't this the same limit as ws-rs was setting? If not then the values are probably arbitrary, fine to change them to whatever, obv with semver compliance.

@maciejhirsz
Copy link
Contributor

For the record, the default 10mb limit in ws-rs/parity-ws is there just so that we don't have an unlimited default. I didn't do any science for it, it should be high enough to make everyday regular use work without issues, so I figured it's better to err on the larger size rather than smaller as long as it doesn't crash things.

@emostov
Copy link
Contributor Author

emostov commented Oct 3, 2020

Just need one more approval :)

@niklasad1
Copy link
Member

@dvdplm: can you take a look at this again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsonrpc_ws_server::ServerBuilder doesn't allow configuring separate values for buffer capacity
5 participants