feat(servers): add max_response_size
#711
Conversation
There was a problem hiding this comment.
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
| let uri = to_http_uri(addr); | ||
| let handle = server.start(module).unwrap(); | ||
|
|
||
| // Oversized response. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
you mean that actual max response works when the actual response_size <= max_response_body_size?
There was a problem hiding this comment.
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(); | ||
|
|
There was a problem hiding this comment.
Same as my above comment; should we check that setting one hasn't impacted the other here too?
| @@ -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, | |||
There was a problem hiding this comment.
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 😄
niklasad1
changed the title
max_response_size
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:
jsonrpsee/http-server/src/server.rs
Line 449 in 9bd2127
to fix this problem, this PR add a max_response_size to the builder, the default value is u32::MAX.