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

Impl Default for CompressionBody when possible #323

Merged
merged 1 commit into from Feb 24, 2023

Conversation

Ameobea
Copy link
Contributor

@Ameobea Ameobea commented Jan 25, 2023

This allows interop with tonic::transport::Server and tonic::transport::server::Router::serve_with_shutdown which require that the responses implement Default.

EDIT: It turns out that this Default requirement was actually coming from CorsMiddleware from this crate.

I was able to work around this by switching the order of the middleware to put CompressionLayer before CorsLayer. It's up to you all if you still want to merge this, but yeah.

Motivation

When trying to use CompressionLayer in a tonic::transport::Server, I was unable to due to missing Default implementation on the CompressionBody.

error[E0277]: the trait bound `CompressionBody<UnsyncBoxBody<bytes::Bytes, tonic::Status>>: std::default::Default` is not satisfied
   --> control_plane/src/server.rs:158:10
    |
158 |         .serve_with_shutdown(addr, shutdown_signal())
    |          ^^^^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `CompressionBody<UnsyncBoxBody<bytes::Bytes, tonic::Status>>`
    |
    = help: the trait `Service<http::Request<hyper::Body>>` is implemented for `foundation::middleware::request_id::RequestIdMiddleware<S>`
    = note: required for `tower_http::cors::Cors<Compression<Routes, OnlyCompressGrpcWebPredicate>>` to implement `Service<http::Request<hyper::Body>>`
    = note: 2 redundant requirements hidden
    = note: required for `foundation::middleware::request_id::RequestIdMiddleware<foundation::middleware::logging::LoggingMiddleware<tower_http::propagate_header::PropagateHeader<tower_http::cors::Cors<Compression<Routes, OnlyCompressGrpcWebPredicate>>>>>` to implement `Service<http::Request<hyper::Body>>`

Solution

This implements a trivial Default for CompressionBody when the inner Body also implements Default.

 * This allows interop with `tonic::transport::Server` and `tonic::transport::server::Router::serve_with_shutdown` which require that the responses implement `Default`.
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I think this is probably fine but tower-http's compression wont do anything for tonic/gRPC since gRPC has its own compression scheme, which tonic does support.

@Ameobea
Copy link
Contributor Author

Ameobea commented Jan 28, 2023

We have a special situation since we're using grpc-web to serve a gRPC server for use by browser clients.

grpc-web doesn't support the native gRPC compression, and expects you to compress at the HTTP level. So we're using a custom tower_http::compression::Predicate to force compression in the case that content_type.starts_with("application/grpc-web").

@davidpdrsn davidpdrsn merged commit f3d8528 into tower-rs:master Feb 24, 2023
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

2 participants