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

feat(servers): add max_response_size #711

Merged
merged 2 commits into from
Mar 22, 2022

Conversation

quake
Copy link
Contributor

@quake quake commented Mar 19, 2022

I wanted to limit max_request_body_size to a relatively small value, say 10 kb, but I found that this setting affected max_response_size, and after reading the code I think the parameter passed here is incorrect:

let sink = MethodSink::new_with_limit(tx, max_request_body_size);

to fix this problem, this PR add a max_response_size to the builder, the default value is u32::MAX.

@quake quake requested a review from a team as a code owner March 19, 2022 08:03
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM except the default limit for max_response_size, it should be same default for both requests and responses. I think 10MB is reasonable but I'm open for suggestions...

FWIW, the old code was not wrong simply that we didn't allow that kind of granularity and applied max_request_size to responses as well.

However, this is good thank you

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Thank you, good job

Copy link

@0xslipk 0xslipk left a comment

Choose a reason for hiding this comment

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

LGTM

let uri = to_http_uri(addr);
let handle = server.start(module).unwrap();

// Oversized response.
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above was done above test, is it worth also checking that the request size is not an issue when only the response size is limited?

Copy link
Member

@niklasad1 niklasad1 Mar 21, 2022

Choose a reason for hiding this comment

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

you mean that actual max response works when the actual response_size <= max_response_body_size?

Copy link
Member

Choose a reason for hiding this comment

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

it won't work unless you register another method for that.

let handle = server.start(module).unwrap();

let mut client = WebSocketTestClient::new(addr).await.unwrap();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as my above comment; should we check that setting one hasn't impacted the other here too?

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM; comments are minor

@@ -66,6 +67,7 @@ impl Default for Builder {
fn default() -> Self {
Self {
max_request_body_size: TEN_MB_SIZE_BYTES,
max_response_body_size: TEN_MB_SIZE_BYTES,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would it make more sense to define this as MAX_RESPONSE_BODY_SIZE instead of TEN_MB_SIZE_BYTES, to decouple naming from underlying value? But I think this is outside the scope of this PR, as the constant was already there 😄

Copy link
Member

Choose a reason for hiding this comment

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

yeah, could make sense :)

@niklasad1 niklasad1 merged commit 78055fe into paritytech:master Mar 22, 2022
@niklasad1 niklasad1 changed the title fix: max_request_body_size setting should not override max_response_size feat(servers): add max_response_size Apr 4, 2022
@niklasad1 niklasad1 mentioned this pull request Apr 4, 2022
7 tasks
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.

5 participants