FollowRedirect: Move Policy::clone_body to a separate trait

[tesaguri]

Feature Request

Motivation

FollowRedirect tries to clone the request bodies using Policy::clone_body function. But I believe that there is usually no more than one way to clone a body for each body type, so ideally clone_body should be be implemented once for a body type, rather than adding .and::<_, _, ()>(clone_body_fn(...)) to every instance of impl Policy. Additionally, removing clone_body from Policy<B, E> lets us remove the B parameter, which in turn removes the B parameter of PolicyExt methods (which might also make it possible to bring the extension trait methods back to Policy trait (#79 (comment)), though I haven't experimented with it yet).

Also, currently, you have to manually implement Policy::<B, _>::clone_body even if B implements Clone. It would be great if FollowRedirect automatically clones the body when B: Clone.

Proposal

Introduce TryClone (or whatever) trait that tries to clone self (request body):

pub trait TryClone: Sized {
    fn try_clone(&self) -> Option<Self>;
}

// User code

pub enum MyBody {
    Stream(/* ... */),
    Full(/* ... */),
}

impl TryClone for MyBody {
    fn try_clone(&self) -> Option<Self> {
        match self {
            MyBody::Stream(/* ... */) => None,
            MyBody::Full(/* ... */) => Some(MyBody::Full(/* ... */))
        }
    }
}

And make FollowRedirect use this trait instead of Policy::clone_body function and remove that function.

When #![feature(stabilization) (rust-lang/rust#31844) lands, we can implement the trait for any B where B: Clone:

pub trait TryClone: Sized {
    fn try_clone(&self) -> Option<Self>;
}

impl<T> TryClone for T {
    default fn try_clone(&self) -> Option<Self> {
        None
    }
}

impl<T: Clone> TryClone for T {
    fn try_clone(&self) -> Option<Self> {
        Some(self.clone())
    }
}

Drawbacks

In user code, TryClone cannot be implemented for types from third party crates and, without specialization, you would end up being unable to clone, for example, an http_body::Full. But tower_http can implement TryClone for http_body's body types in the meantime.

Even with specialization, the trait cannot be implemented for non-Clone third-party body types. However, I expect its impact to be small in practice because few (if any) body types out there expose interfaces that let third party code implement clone_body. For example, hyper::Body internally has Once variant which could in theory be cloned whereas hyper::Body itself is not Clone, but in fact there is no way for third party code to reference the variant and clone it.

Though, it should be noted that if hyper adds GenericBody and EitherBody types (hyperium/hyper#2345 (comment)) and makes their variants public, they would be affected by this drawback. But if they live under tower_http instead (EitherBody surely belongs there at least), we can simply implement TryClone for them in tower_http.

Also, this suggestion relies on specialization feature (instead of min_specialization as it stands), whose path towards stabilization is still very unclear.

Alternatives

Keep the current API as-is. We may well want to wait until specialization feature stabilizes. Also, we can apply the specialization trick in the proposal even while keeping the API in its current form, by making TryClone trait private and using TryClone::try_clone as a fallback for Policy::clone_body.

[davidpdrsn]

I think this is a great idea!

Especially

Additionally, removing clone_body from Policy<B, E> lets us remove the B parameter, which in turn removes the B parameter of PolicyExt methods (which might also make it possible to bring the extension trait methods back to Policy trait (#79 (comment)), though I haven't experimented with it yet).

would be a very nice improvement!

[tesaguri]

I did a quick experiment on implementing a part of the proposed API and I realized some oversights in the proposal.

removing clone_body from Policy<B, E> lets us remove the B parameter

This turned out to be non-trivial because Policy::on_request takes an &mut Request<B>. We can still achieve this by making on_request generic over B, but this would make Policy no longer object safe.

Also, if we are going to adopt this API prior to the stabilization of specialization, we need to consider a way to make FollowRedirect<impl Service<Request<B>>, P> a Service when B is a third-party type. The obvious implementation would be like the following:

impl<ReqBody, ResBody, S, P> Service<Request<ReqBody>> for FollowRedirect<S, P>
where
    ReqBody: Body + Default + TryClone,
    // ...

But this would not work when ReqBody does not implement TryClone, like hyper::Body. So I have another idea. First, create types like the following:

// Naming is difficult...
pub enum WithClone {}
pub enum WithTryClone {}
pub enum NoClone {}

pub(crate) trait CloneStrategy<B> {
    fn try_clone(body: &B) -> Option<B>;
}

impl<B: Clone> CloneStrategy<B> for WithClone {
    fn try_clone(body: &B) -> Option<B> {
        Some(body.clone())
    }
}

impl<B: TryClone> CloneStrategy<B> for WithTryClone {
    fn try_clone(body: &B) -> Option<B> {
        body.try_clone()
    }
}

impl<B> CloneStrategy<B> for NoClone {
    fn try_clone(body: &B) -> Option<B> {
        None
    }
}

Then add a new type parameter to FollowRedirect (which might default to NoClone) and provide constructors for FollowRedirect<S, P, WithClone>, FollowRedirect<S, P, WithTryClone> and FollowRedirect<S, P, NoClone>, and use the CloneStrategy internally when cloning a body.

The above idea solves the problem of trait bounds, but it comes with complexity cost with additional type parameter and constructors. It may simplify the use case with http_body::Full, but I'm not sure this benefit outweighs the cost.

[davidpdrsn]

@Nehliin @MarcusGrass and I (all work at embark) and have talked about this a bit. We think given the number of unanswered questions here not gonna include this in tower-http 0.2.

It feels having some answer to cloning requests would be useful for this as well, and also useful in general. So at some point we should research that.