`max_decoding_message_size` not applied in some cases

tgolsson commented 1 year ago

Bug Report

Version

Reproduced on HEAD, see test in linked diff.

Platform

N/A

Description

Hey,

I'm working with a slightly unorthodox setup involving FFI, and have a custom tower::Service to pass the FFI barrier. After upgrading to 0.10, I noticed some of our bulky messages still fail to decode despite setting MyClient::max_decoding_message_size to a significantly higher limit.

After some digging, I found #1353. I reduced the repro from my repository to (approximately) what's now in the max_message_size_blob test case, which fails with the code as is. My analysis is that due to how our FFI code is implemented, we end up consuming the trailers into headers immediately (not supporting streaming). This triggers the Streaming::empty case, which as noted in the previous issue does not pass the max_decoding_message_size.

I think this is actually a bug in how we re-construct the message because there's no easy way to construct a Body with Trailers from "owned" data. It seems like the gRPC spec doesn't allow passing the trailers in the headers, and the issue goes away if I create a body with two frames of data. However; I'd argue there's still a bug here insofar that deserialization works as expected up to 4MB even with our potentially malformed responses; and beyond 4MB if I ensure the max_decoding_message_size is forwarded. I'd thus argue for still passing it to Streaming::empty(), as proposed in the original issue and in my eventual PR. This would lead to a more consistent behavior.

Eshanatnight commented 5 months ago

Hi, I wanted to ask if there is any news with this issue.

I am implementing a Arow Flight Server. and when I am hitting the do_get call. I end up receiving Error, message length too large: found 6624665 bytes, the limit is: 4194304 bytes.

If I query data that is somewhat less, It works as intended.

I even set the values to Max

    let svc = FlightServiceServer::new(service)
        .max_encoding_message_size(usize::MAX)
        .max_decoding_message_size(usize::MAX)
        .send_compressed(CompressionEncoding::Gzip);

but it did not work.

If I dbg!(&svc); I can see that the values were was set properly

zhixinwen commented 2 months ago

I may be hit by the same issue. I am trying to understand why the code assumes resp is empty when it can read status from header? https://github.com/hyperium/tonic/blob/9990e6ef9d00394b5662719662062cc85e6e4700/tonic/src/client/grpc.rs#L338C36-L338C59

hyperium / tonic