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

Configure batch support max items #987

Closed
xlc opened this issue Jan 24, 2023 · 8 comments · Fixed by #1073
Closed

Configure batch support max items #987

xlc opened this issue Jan 24, 2023 · 8 comments · Fixed by #1073

Comments

@xlc
Copy link
Contributor

xlc commented Jan 24, 2023

We would like to enable batch support but with a relatively low limits. So it will be good if we can configure the max items in a batch request.

@lexnv
Copy link
Contributor

lexnv commented Jan 25, 2023

Hey,

We are providing currently the ability to specify the max body size of those items, from here:

pub(crate) async fn process_batch_request<L: Logger>(b: Batch<'_, L>) -> Option<BatchResponse> {
let Batch { data, call } = b;
if let Ok(batch) = serde_json::from_slice::<Vec<&JsonRawValue>>(&data) {
let mut got_notif = false;
let mut batch_response = BatchResponseBuilder::new_with_limit(call.max_response_body_size as usize);

Similarly, we could extend that area to support a call.max_batch.items. And that seems like a good idea!

We would greatly appreciate any help with this matter and any improvements made here. Let us know if you are interested and could provide further guidance here! Thanks for taking an interest in this!

@xlc
Copy link
Contributor Author

xlc commented Jan 25, 2023

As you may have noticed that I raised multiple issues in past two days. To give you a bit more context, I am researching on using jsonrpsee to implement a JSON RPC reverse proxy server. Something similar to http://github.com/tomusdrw/jsonrpc-proxy but with up-to-date deps.

So far I have identified multiple lacking features and raised issues here but none of them are significant enough to be a blocker. So I am likely to stick with the original plan using jsonrpsee for JSON RPC handling and making necessary changes in own fork and contribute them upstream.

This is the project: https://github.com/AcalaNetwork/subway

@lexnv
Copy link
Contributor

lexnv commented Jan 25, 2023

Thanks a lot for all context and interest!

@crystalin
Copy link

I confirm this is also important for us.
Attackers have been able to exploit "batch" to OOM the machines easily. Currently with a single batch request, even with a 10Mb output limit, you can have json_serde use > 10GB of memory easily (I didn't try the maximum but I'm pretty sure it is close to unlimited).

@niklasad1
Copy link
Member

niklasad1 commented Feb 1, 2023

I confirm this is also important for us.
Attackers have been able to exploit "batch" to OOM the machines easily. Currently with a single batch request, even with a 10Mb output limit, you can have json_serde use > 10GB of memory easily (I didn't try the maximum but I'm pretty sure it is close to unlimited).

If the response is bigger than that limit we simply bail out and drop the rest of the futures/calls in batch. However, until then I suppose the calls in the batch could use plenty of memory until it's dropped as it's executed "concurrently"

Are you really sure that batch requests is the cause that serde_json is using > 10GB? How big batch requests then?

Fine, but it already possible disable batch requests completly but let's add this to the next release then.

@crystalin
Copy link

Yes

top 1. Memory Size: 9.429 G (2127387 pages)
...
#23 [moonbeam] jsonrpsee_types::response::_::<impl serde::ser::Serialize for jsonrpsee_types::response::Response<T>>::serialize
#24 [moonbeam] <fc_rpc_core::types::bytes::Bytes as serde::ser::Serialize>::serialize
#25 [moonbeam] serde_json::ser::format_escaped_str
#29 [moonbeam] alloc::raw_vec::finish_grow
#30 [libbytehound.so] realloc [api.rs:378]

I didn't go too deep into it yet but what is strange is that it only applies to the batch through Websocket. The same batch with HTTPS doesn't trigger such a big memory

@niklasad1
Copy link
Member

The HTTP server doesn't have internal mpsc channel which the WS connection has which may use a bunch of memory.

We are in process moving everything to the bounded channels/backpressure that PR will probably go in a couple of weeks, #962

@niklasad1
Copy link
Member

niklasad1 commented May 11, 2023

@crystalin

It would quite interesting if you could try paritytech/substrate#13992

As this PR is included we could actually make the batch request size configurable in substrate CLI as well but my hunch is that backpressure should be sufficient.

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 a pull request may close this issue.

4 participants