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(body): add Body::wrap_body #2913

Closed
wants to merge 1 commit into from
Closed

Conversation

davidpdrsn
Copy link
Member

This adds Body::wrap_body which creates a Body from any impl http_body::Body. This is required in axum for tokio-rs/axum#1154.

Note that I made the PR against 0.14.x because the Body struct is undergoing some big changes on master. If you want me to make this PR against master instead and backport this to 0.14.x then just let me know 😊

@sfackler
Copy link
Contributor

Wouldn't BoxBody suffice for axum?

@davidpdrsn
Copy link
Member Author

davidpdrsn commented Jul 11, 2022

Wouldn't BoxBody suffice for axum?

No, I wish 😞

I want to support impl FromRequest for Request<hyper::Body> and also fix tokio-rs/axum#1110. That essentially requires being able to convert an impl http_body::Body into a hyper::Body.

I'll try an elaborate. This is a simplified version of what we have in axum today (everything is generic over the request body):

#![allow(warnings)]

use http::{Request, Response};
use http_body::combinators::UnsyncBoxBody;
use hyper::{body::Bytes, Body};
use std::{
    collections::HashMap,
    convert::Infallible,
    future::Future,
    pin::Pin,
    task::{Context, Poll},
};
use tower::{util::BoxCloneService, Layer, Service, ServiceExt};

type BoxBody = UnsyncBoxBody<Bytes, hyper::Error>;
type BoxFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a>>;
type BoxError = Box<dyn std::error::Error + Send + Sync>;

struct Route<B> {
    inner: BoxCloneService<Request<B>, Response<BoxBody>, Infallible>,
}

impl<B> Clone for Route<B> {
    fn clone(&self) -> Self {
        Self {
            inner: self.inner.clone(),
        }
    }
}

impl<B> Service<Request<B>> for Route<B> {
    type Response = Response<BoxBody>;
    type Error = Infallible;
    type Future = BoxFuture<'static, Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        todo!()
    }

    fn call(&mut self, req: Request<B>) -> Self::Future {
        todo!()
    }
}

struct Router<B> {
    routes: HashMap<String, Route<B>>,
}

impl<B> Router<B> {
    fn new() -> Self {
        Router {
            routes: HashMap::new(),
        }
    }

    pub fn layer<L, NewReqBody, NewResBody>(self, _layer: L) -> Router<NewReqBody>
    where
        L: Layer<Route<B>>,
        L::Service:
            Service<Request<NewReqBody>, Response = Response<NewResBody>> + Clone + Send + 'static,
        <L::Service as Service<Request<NewReqBody>>>::Error: Into<Infallible> + 'static,
        <L::Service as Service<Request<NewReqBody>>>::Future: Send + 'static,
        NewResBody: http_body::Body<Data = Bytes> + Send + 'static,
        NewResBody::Error: Into<BoxError>,
    {
        todo!()
    }
}

impl<B> Service<Request<B>> for Router<B> {
    type Response = Response<BoxBody>;
    type Error = Infallible;
    type Future = BoxFuture<'static, Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        Poll::Ready(Ok(()))
    }

    fn call(&mut self, req: Request<B>) -> Self::Future {
        todo!()
    }
}

#[test]
fn a_test() {
    use http_body::Limited;
    use tower_http::limit::RequestBodyLimitLayer;

    let router: Router<Limited<Body>> = Router::new().layer(RequestBodyLimitLayer::new(1024));
}

We want Router (and thus Route) to not be generic to solve tokio-rs/axum#1110 so we need to decide on a request body in

struct Route<B> {
    inner: BoxCloneService<Request<B>, Response<BoxBody>, Infallible>,
}

If we pick hyper::Body that forces Router::layer to be:

pub fn layer<L, NewResBody>(self, _layer: L) -> Router
where
    L: Layer<Route>,
    // this says that the service the layer produces must accept `Request<Body>`
    L::Service:
        Service<Request<Body>, Response = Response<NewResBody>> + Clone + Send + 'static,
    <L::Service as Service<Request<Body>>>::Error: Into<Infallible> + 'static,
    <L::Service as Service<Request<Body>>>::Future: Send + 'static,
    NewResBody: http_body::Body<Data = Bytes> + Send + 'static,
    NewResBody::Error: Into<BoxError>,
{
    ...
}

and we are forced to implement Service as

impl Service<Request<Body>> for Route { ... }

impl Service<Request<Body>> for Router { ... }

However those two (fn layer and the Service impls) don't work with each other. If we do

let router = Router::new().layer(RequestBodyLimitLayer::new(1024));

Then RequestBodyLimit will require the inner service (a Route) to accept Request<Limited<Body>> but Route only accepts Request<Body>. So it doesn't work.

If we try to make the Service impl more general we also run into issues:

impl<B> Service<Request<B>> for Route {
    type Response = Response<BoxBody>;
    type Error = Infallible;
    type Future = BoxFuture<'static, Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        Poll::Ready(Ok(()))
    }

    fn call(&mut self, req: Request<B>) -> Self::Future {
        // but this doesn't work either because `Route` contains a
        // `BoxCloneService<Request<Body>, Response<BoxBody>, Infallible>`
        // which we must call with a `Request<Body>` but we have no way to convert
        // `B` into a `Body`
        todo!()
    }
}

So that is why Body::wrap_body is needed because it allows me to convert the generic B in these Service impls into Body.

You're right that I could "just" use BoxBody for axum and then all this goes away but then I cannot impl FromRequest for Request<Body>. Again because that requires converting a BoxBody into a Body. I'm worried its gonna be a source of confusion if this doesn't work.

I hope you're able to see some magic trick that makes this all work without Body::wrap_body but I don't see it 😞

@davidpdrsn davidpdrsn force-pushed the david/body-wrap-body branch from 96eb6c6 to 355b6fc Compare July 11, 2022 15:33
@davidpdrsn davidpdrsn force-pushed the david/body-wrap-body branch from 355b6fc to 5be673f Compare July 11, 2022 15:35
@davidpdrsn
Copy link
Member Author

axum is moving to its own body type (tokio-rs/axum#1584) so we don't actually need this anymore. Things have also changed for hyper 1.0 so I'll close this for now.

@davidpdrsn davidpdrsn closed this Mar 20, 2023
@davidpdrsn davidpdrsn deleted the david/body-wrap-body branch March 20, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-body Area: body streaming.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants