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

compression: handle compression flag but no header #763

Merged
merged 3 commits into from Oct 20, 2021
Merged

compression: handle compression flag but no header #763

merged 3 commits into from Oct 20, 2021

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Sep 12, 2021

Motivation

Due to some yet unknown reason I ended up with the crash described in #755 where the client set the compressed bit but doesn't send the correct header for the server for compression and the server panics as it reaches an unreachable! statement with:

thread 'tokio-runtime-worker' panicked at 'internal error: entered unreachable code: message was compressed but `Streaming.encoding` was `None`. This is a bug in Tonic.

Note: the client issue might be a problem with one of the configured interceptors in my codebase, to be investigated separately.

Solution

The case where a compression flag is set but no compression header is present is checked while transitioning between the header and body phases an internal error is returned in this case.

@QuentinPerez
Copy link
Contributor

hey @davidpdrsn 👋

by chance, could you take a look on this patch ? we would like to move away from our patches/forks 🙂

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

});

let mut client = test_client::TestClient::with_interceptor(mock_io_channel(client).await, move |mut req: Request<()>| {
req.metadata_mut().remove("grpc-encoding");
Copy link
Member

Choose a reason for hiding this comment

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

neat approach!

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.

None yet

3 participants