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

feat(tonic): compression support #692

Merged
merged 29 commits into from
Jul 2, 2021
Merged

feat(tonic): compression support #692

merged 29 commits into from
Jul 2, 2021

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Jun 25, 2021

Don't be scared by the large number of lines added. Most of it is tests 😊

This adds compression/decompression support to tonic::server::Grpc and tonic::client::Grpc.

Features:

  • Supports transparently compressing/decompressing requests, responses, and streams. Only encoding supported so far is gzip but adding new ones is straight forward.
  • Uses flate2 but the compression backend can be changed without breaking changes.
  • Generated clients and servers have some new methods for configuring which encodings are accepted/enabled. All these methods forward for tonic::server::Grpc or tonic::client::Grpc respectively:
    • accept_gzip (client): It enables receiving gzip compressed responses (sets grpc-accept-encoding accordingly).
    • accept_gzip (server): It enables receiving gzip compressed requests. If a server receives a request with an unsupported encoding it responds with Unimplemented as per the gRPC spec.
    • send_gzip (client): This will compress requests with gzip. Again server will respond with Unimplemented if that isn't supported/enabled.
    • send_gzip (server): This will compress responses if the client supports it.

Example usage

// build server the supports receiving and sending gzip compressed content
let svc = test_server::TestServer::new(Svc).accept_gzip().send_gzip();

// spawn the server
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();
tokio::spawn(async move {
    Server::builder()
        .add_service(svc)
        .serve_with_incoming(tokio_stream::wrappers::TcpListenerStream::new(listener))
        .await
        .unwrap();
});

// create channel connected to the server
let channel = Channel::builder(format!("http://{}", addr).parse().unwrap())
    .connect()
    .await
    .unwrap();

// create client that also supports receiving and sending gzip compressed content
let mut client = test_client::TestClient::new(channel)
    .accept_gzip()
    .send_gzip();

// make a request as per usual
let response = client
    .compress_input_unary(SomeData {
        data: vec![1, 2, 3, 4],
    })
    .await
    .unwrap();

Why not compress in the transport layer?

My original thought was to bake compression/decompression into the transport layer. So to enable compression on a client you'd call accept_gzip on the Endpoint (Channel builder), and for the server you'd call it on Server. However it turns out to not work great. The issue being that compression settings need to be funneled into tonic::server::Grpc and tonic::client::Grpc because thats where the actual protocol stuff happens.

The downside to that is that it requires new generated methods for client and server structs, which hurts discoverability. Hopefully some decent docs and an example will help with that.

Outstanding issues

Compression is currently enabled or disabled globally for a client. It is not possible to configure for individual requests/responses/stream messages. Its easy enough for requests/responses, can be done with a new field on Request/Response, or with an extension. However since stream messages aren't wrapped in anything there is no place for tonic to add additional configuring hooks. I've considered a breaking change where streams are required to yield something like tonic::StreamMessage<ActualMessage> but not sure. What do you think @LucioFranco?

Edit: Compression on unary/client stream responses can now be disabled with Response::disable_compression. It doesn't do anything for server streams as that would require a config on each item in the stream.

TODO

Besides the comments I've left for myself below:

  • Add example
  • When server receives request with encoding that isn't supported, include grpc-accept-encoding with which encodings are supported.
  • Test bidirectional_stream::client_enabled_server_enabled is flaky. It sometimes fails. Have to fix that.

Encoding/decoding parts based on #476

Fixes #282

@davidpdrsn davidpdrsn added C-enhancement Category: New feature or request A-tonic labels Jun 25, 2021
@davidpdrsn davidpdrsn mentioned this pull request Jun 25, 2021

impl<T> std::fmt::Debug for #service_ident<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, #struct_debug)
Copy link
Member Author

Choose a reason for hiding this comment

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

tonic::client::Grpc actually contains useful data now. So figured it makes sense to change this Clone and Debug impl to derives. Made sure the Derive doesn't require T: Derive.

tonic/src/client/grpc.rs Outdated Show resolved Hide resolved
tonic-build/src/client.rs Outdated Show resolved Hide resolved
tonic-build/src/server.rs Outdated Show resolved Hide resolved
let mut gzip_encoder = GzEncoder::new(
&decompressed_buf[0..len],
// FIXME: support customizing the compression level
flate2::Compression::new(6),
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to support customizing this. I'll look into that when this has been merged while I also look into adding support for more compression encodings.

);
let mut out_writer = out_buf.writer();

tokio::task::block_in_place(|| std::io::copy(&mut gzip_encoder, &mut out_writer))?;
Copy link
Member Author

Choose a reason for hiding this comment

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

Have to use block_in_place here to avoid blocking the executor. Making compress and decompress async is be quite hard because of how Streaming works.

Copy link
Member

Choose a reason for hiding this comment

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

ugh I really don't like adding this as a dep basically doens't allow you to use compression when using tokio::test. Its not really doing any io could we get away without the block in place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed it in 5458db1.

@@ -156,19 +173,13 @@ impl<T> Streaming<T> {

fn decode_chunk(&mut self) -> Result<Option<T>, Status> {
if let State::ReadHeader = self.state {
if self.buf.remaining() < 5 {
if self.buf.remaining() < HEADER_SIZE {
Copy link
Member Author

Choose a reason for hiding this comment

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

Took a while to figure out what this hardcoded 5 was. Figured it made sense to extract into a const.

"Message compressed, compression not supported yet.".to_string(),
));
}
1 => true,
Copy link
Member Author

Choose a reason for hiding this comment

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

🎊


if let Err(err) = decompress(
self.encoding.unwrap_or_else(|| {
unreachable!("message was compressed but `Streaming.encoding` was `None`. This is a bug in Tonic. Please file an issue")
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be caught by higher levels so this case should be unreachable 🤞

tonic/src/server/grpc.rs Outdated Show resolved Hide resolved
@roblabla
Copy link
Contributor

Am I understanding this right that this won't allow consumers to pass custom compression algorithms? If so, that kinda sucks. I've been using a fork of tonic for my application to support passing a custom codec that does AES compression (see #461).

Back then, the discussion was concluded that it should be handled at the compression layer (which makes sense, since we'd want to compress then encrypt), but it now appears like the compression layer will only allow a fixed, hardcoded list of compression algorithms, with no way to plug our own.

Would it be possible to expose a trait so tonic users may provide their own "compression" routines?

@davidpdrsn
Copy link
Member Author

@roblabla supporting custom compression algorithms was discussed a bit in #476. Personally I don't feel its worth the compressing it brings. Other crates like reqwest and warp don't support custom algorithms either. If its something there is more demand for we can of course reconsider.

@LucioFranco What do you think?

@mat-gas
Copy link

mat-gas commented Jun 30, 2021

@davidpdrsn I think there's a slight misunderstanding about what @roblabla said :
I think he actually wants to use AES encryption (and not compression, typo in his message) so it's not a custom compression algorithm he wants, but the hability to chain compression than encryption (because encrypting data then compressing is totally useless) using custom codecs or whatever

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 overall just a few questions before I approve

tests/compression/src/bidirectional_stream.rs Show resolved Hide resolved
tests/compression/src/bidirectional_stream.rs Outdated Show resolved Hide resolved
tests/compression/src/client_stream.rs Outdated Show resolved Hide resolved
tests/compression/src/client_stream.rs Outdated Show resolved Hide resolved
#[cfg(not(feature = "compression"))]
pub fn send_gzip(self) -> Self {
panic!(
"`send_gzip` called on a server but the `compression` feature is not enabled on tonic"
Copy link
Member

Choose a reason for hiding this comment

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

This mentions server but we are in the client? Also, wouldn't it be better to just have this not compile, aka just don't include a not version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes not compiling would be better, and its also what I did initially, but it doesn't work with the "Check features" step in CI. It seems to test all combinations of features. It doesn't seem to be possible to tell it to exclude a set of features.

match header_value_str {
"gzip" if gzip => Ok(Some(CompressionEncoding::Gzip)),
other => {
let mut status = Status::unimplemented(format!(
Copy link
Member

Choose a reason for hiding this comment

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

is this the correct status other implementations return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes:

If a client message is compressed by an algorithm that is not supported by a server, the message WILL result in an UNIMPLEMENTED error status on the server. The server will then include a grpc-accept-encoding response header which specifies the algorithms that the server accepts. If the client message is compressed using one of the algorithms from the grpc-accept-encoding header and an UNIMPLEMENTED error status is returned from the server, the cause of the error MUST NOT be related to compression. If a server sent data which is compressed by an algorithm that is not supported by the client, an INTERNAL error status will occur on the client side.

From https://github.com/grpc/grpc/blob/master/doc/compression.md

);
let mut out_writer = out_buf.writer();

tokio::task::block_in_place(|| std::io::copy(&mut gzip_encoder, &mut out_writer))?;
Copy link
Member

Choose a reason for hiding this comment

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

ugh I really don't like adding this as a dep basically doens't allow you to use compression when using tokio::test. Its not really doing any io could we get away without the block in place?

tonic/src/server/grpc.rs Outdated Show resolved Hide resolved
@LucioFranco LucioFranco added this to the 0.5 milestone Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tonic C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compression Support
4 participants