-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
|
||
impl<T> std::fmt::Debug for #service_ident<T> { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
write!(f, #struct_debug) |
There was a problem hiding this comment.
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
.
let mut gzip_encoder = GzEncoder::new( | ||
&decompressed_buf[0..len], | ||
// FIXME: support customizing the compression level | ||
flate2::Compression::new(6), |
There was a problem hiding this comment.
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.
tonic/src/codec/compression.rs
Outdated
); | ||
let mut out_writer = out_buf.writer(); | ||
|
||
tokio::task::block_in_place(|| std::io::copy(&mut gzip_encoder, &mut out_writer))?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
.
tonic/src/codec/decode.rs
Outdated
"Message compressed, compression not supported yet.".to_string(), | ||
)); | ||
} | ||
1 => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎊
tonic/src/codec/decode.rs
Outdated
|
||
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") |
There was a problem hiding this comment.
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 🤞
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? |
@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? |
4a04863
to
9c230ba
Compare
@davidpdrsn I think there's a slight misunderstanding about what @roblabla said : |
There was a problem hiding this 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
tonic/src/client/grpc.rs
Outdated
#[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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
tonic/src/codec/compression.rs
Outdated
); | ||
let mut out_writer = out_buf.writer(); | ||
|
||
tokio::task::block_in_place(|| std::io::copy(&mut gzip_encoder, &mut out_writer))?; |
There was a problem hiding this comment.
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?
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
andtonic::client::Grpc
.Features:
tonic::server::Grpc
ortonic::client::Grpc
respectively:accept_gzip
(client): It enables receiving gzip compressed responses (setsgrpc-accept-encoding
accordingly).accept_gzip
(server): It enables receiving gzip compressed requests. If a server receives a request with an unsupported encoding it responds withUnimplemented
as per the gRPC spec.send_gzip
(client): This will compress requests with gzip. Again server will respond withUnimplemented
if that isn't supported/enabled.send_gzip
(server): This will compress responses if the client supports it.Example usage
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 theEndpoint
(Channel
builder), and for the server you'd call it onServer
. However it turns out to not work great. The issue being that compression settings need to be funneled intotonic::server::Grpc
andtonic::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 liketonic::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:
grpc-accept-encoding
with which encodings are supported.bidirectional_stream::client_enabled_server_enabled
is flaky. It sometimes fails. Have to fix that.Encoding/decoding parts based on #476
Fixes #282