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

Bug: user-defined max_decoding_message_size is not used in some cases (client streaming response decoding) #1353

Closed
checkline1052 opened this issue Apr 13, 2023 · 2 comments

Comments

@checkline1052
Copy link

checkline1052 commented Apr 13, 2023

Bug Report

Version

tonic 0.9.1
tonic-build 0.9.1

Platform

Fedora 37 x86_64

Crates

tonic

Description

I recently updated tonic to v0.9.1, and I experienced breaking changes due to DEFAULT_MAX_RECV_MESSAGE_SIZE. It's ok, we can increase this limit with max_decoding_message_size() and max_encoding_message_size() (in both server and client !), and it did fix "message lenth too large" errors for normal requests/responses (I increased it to 60MB).

However, I still have these errors in stream responses decoding, when calling if let Some(stream_response) = request.message().await.unwrap() :

TonicStatus status: OutOfRange, message "Error, message length too large: found 6877902 bytes, the limit is: 4194304 bytes", details: [], metadata: MetadataMap { headers: {} }

In Grpc::create_response() if found that in a some cases, the codec::decode::Streaming is created without the user-defined max_decoding_message_size, when calling Streaming::new_empty(...) here :

let response = response.map(|body| {
if expect_additional_trailers {
Streaming::new_response(
decoder,
body,
status_code,
encoding,
self.config.max_decoding_message_size,
)
} else {
Streaming::new_empty(decoder, body)
}
});

But I do no have a complete understanding of the tonic crate, so I did not open a pull-request for this issue yet

Thank you !

@LucioFranco
Copy link
Member

Hi @checkline1052, I wrote a test to try and reproduce what you were stating above but from what I can tell everything works as intended. Please take a look at #1363 and see if this differs from your code.

@checkline1052
Copy link
Author

Thanks you for the integration test, I was doing something wrong !

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

No branches or pull requests

2 participants