Make Response more generic #1164

Twey · 2017-05-04T14:32:20Z

The commit messages match the guidelines in https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md#git-commit-guidelines

seanmonstar · 2017-05-04T15:34:18Z

What was the exact problem you're trying to solve here, so I can have some more context?

Twey · 2017-05-04T16:39:27Z

Essentially, I have some code where I transform the response body but want to leave the rest of the response info intact for upstream consumption. Specifically, I'm a) asserting that a particular API call always returns a body, and don't want my callers to always have to unwrap(); b) deserializing the body of the API response from JSON. So combined I have a transformation from Request<Option<Body>> into Request<DeserializedType>. I had, essentially, my own local copy of Response, to which I transformed Response as soon as possible, but this seemed a bit silly, and I noticed that Response had been generalized over the body type: it just lacked tools to actually work with the generalized type (e.g. with_body requiring that the new body be of the same type as the old body) that I needed. Since that seemed to be the direction the codebase was going anyway, I thought I'd contribute them. Separating sums and products is good API hygiene anyway, and happens to help solve my problem; I'm not sure this is precisely the right API to do that (I modified the existing API as little as possible, but it might be nice to avoid propagating Options everywhere), but seems like a reasonable step.

seanmonstar · 2017-05-04T22:54:37Z

I've been thinking about adding 1 piece here, that would do what you want, a split method, and I think making with_body generic over a new body type would then allow you to do what you, right?

impl<B: Default> Response<B> {
    pub fn split(self) -> (Response<B>, B);
}

impl<B> Response<B> {
    pub fn with_body<BB>(self, body: BB) -> Response<BB>;
}

With just those changes, could you achieve what you want? I think I'd prefer having a Default bound and returning a default into of Option, but maybe I'm wrong.

Twey · 2017-05-05T11:47:36Z

Yes, this is similar to the method I've called take_body, but with less type information. That method could be renamed split.

However, that approach alone, keeping the Option internal to the Response, has two issues: 1) it's now impossible to know whether the response had no body or whether it had a body that was equal to B::Default (and generating B::Default and comparing the body to it may not be cheap [or ergonomic — e.g. deciding whether a Body is Body::default() requires creating another Future layer and then flattening it out]), and 2) it's impossible to talk in the type system about responses that definitely have (or do not have — although this can be written Response<!> in new Rust) a body. 1) is an instance of the general principle that results in Rust functions returning Result or Option a lot: the idea is that the user should be able to decide what to do with the information, rather than the library making a guess at how they want to handle the special case. 2) is directly part of my use case.

It's a simple translation from .body() to .into_body().unwrap_or_default() (the name body() in the current codebase seems to be inconsistent: similarly-named functions, like headers() or version(), do not consume the Response) to get the same effect for the runtime API, but gives the user much more control. The part I mostly dislike about this patch is that there are a lot of instances of <B: Stream> Response in the codebase, which become <B: Stream> Response<Option>. It seems like instead we should push the type information further down into the code, so the Option becomes less necessary (obviously we have it when the Response has just come off the wire, but we can quickly project out of it, e.g. fn check_body(Response<Option>) -> Either<Response, Response<()>>, and then track the presence or absence of the body through the codebase). But that's a slightly larger project that I don't have time for right now — I'm using hyper in anger at work :) Maybe I'll have some time to play with it on the weekend.

Make methods on Response more generic over the body type, including methods to extract and modify the body (including changing body type) and moving the optionality of the body into the type parameter, and remove reliance on Body in several places.

seanmonstar · 2017-05-09T22:34:11Z

I'm not ignoring this, I've come back to re-read a few times now. I'm going to try out the PR locally and fiddle. I'm just afraid of being harder to use, but maybe it's required.

seanmonstar · 2017-05-10T00:29:45Z

Ok, thinking about this, the general idea seems like the right way to go. I mostly didn't like res.set_body(Some(foo)), since it messed with Into<Body>, and it just looked weird using Some. But!

hyper's default Body can easily encapsulate being an Option. It could easily have a Into<Option> implementation, and then we'd still have the nicer default API, and still be able to tell if the body was empty or not. Whatcha think?

Twey · 2017-05-10T01:13:16Z

Yes, absolutely! That sounds great. In fact we already have the notion of an ‘empty’ body.
There's a similar argument to be made for Request, of course!

Twey · 2017-05-10T01:52:52Z

Is Into<Option<…>> worth it? Perhaps we just want to expose an alias pub type Body = Option<TokioBody>;?

EDIT: Scratch that — I guess you explicitly want to be able to say things like Body::empty() instead of None, right?

seanmonstar · 2017-05-10T05:06:58Z

After trying out a bunch of impls, I don't think what I said can actually be done properly.

I'm now back to thinking that the body: Option should stay. For the case where it's expensive to create the Default, that can be fixed by defining a cheap "empty" state. If the type is from another crate, a user can define a new type over it with a cheap default. For the case that you cannot tell if there is no body, again, being your own type you can add methods to ask if there is no body. We can add one to Body::is_empty, too.

I could see Response having this adjusted method:

impl<B: Default> Response<B> {
    pub fn split(self) -> (Response<Empty>, B);
}

pub struct Empty(());

Or possibly -> (Response<Empty>, Option)...

Twey · 2017-05-15T16:23:41Z

What's wrong with the impls?

Unfortunately, that approach doesn't fix the primary issue, which is that there's no information in the type system about whether or not the response has a body, so the user has to check or unwrap() every time they want to use it.

Re split(): the proper return type here is (Response<!>, B) (or (Response<!>, Option) without the Default constraint that throws away information). Since ! has no values, the only value the body could have is None, which is accurately reflected in the type system. But there's still no way to say that the body definitely has a body so long as the optionality of the body is hidden inside the implementation.

seanmonstar · 2017-06-17T22:59:22Z

The issue I ran into is that libstd already defines a blanket Into<Option<T>> for T, so we can't implement custom ones for different body types. Maybe we could when specialization stabilizes, if that is indeed how we'd want to declare that a body exists or not.

I'm sorry for the long silence. I actually am quite interested in this problem, and you're work in this PR has caused a few (even recently) discussions among some of us. Some of the tricky part is how to define that a message has no body. ! isn't stable, so we can't rely on it yet. Does that mean we define a NoBody type in hyper? There's some reluctance to do that, as well. So then, do we just say that Response<Foo> means there is definitely a Foo, and Response<Option<Foo>> means there might be? That might be the best solution, even though I find that it makes the default case to be worse. Specifically, when you receive a Response from a client request, it might have a body, it might not, but having to check an option feels bad, especially when in the Som(Body) case, that Body could still end up being empty...

Twey · 2017-06-18T18:48:14Z

Would we want custom ones?

As for ! — there is the void crate for just such a case.

If Body already has the possibility of being empty, surely it should have an impl for Into<Option<T>> where T is some kind of non-empty type. But this should be largely immaterial — the only case where it matters whether the body could be empty or not (because we need to be able to wrap up non-body responses) is when it comes off the wire, in which case we already fix the body type.

seanmonstar · 2017-07-13T18:21:24Z

If Body already has the possibility of being empty, surely it should have an impl for Into<Option<T>> where T is some kind of non-empty type.

The problem is that the blanket impl From<Option<T>> for T in std means we cannot customize (yet, probably can with specialization).

We can't do this in hyper 0.11, but while trying to design an http crate that will have Request<T> and Response<T> that would replace hyper's types, we have killed the internal Option, so this should be the case eventually.

I'm going to close the PR cause we can't merge, but I want to state again: thank you! This PR has been a huge help, even though it hasn't been merged.

Twey · 2017-07-14T12:00:58Z

I'm still thinking about this! I've actually put it on my work schedule for this/next week, since we currently have an internal hyper fork with this feature and we'd like to get rid of it.

Feel free to close, but would you be happy with a similar PR with alternative API suggestions?

seanmonstar · 2017-07-14T23:10:07Z

As I mentioned, we're planning to extract these types into a plain http crate, and the design we currently have is essentially this:

struct Request<B> {
    // head stuff
    body: B,
}

impl<B> Request<B> {
    pub fn body(&self) -> &B { &self.body }
    pub fn body_mut(&mut self) -> &mut B { &mut self.body }
}

Granted, once hyper is using such a type, we probably need some bounds on B when using with a Service.

Did you have other thoughts besides this? (Oh, Request and Response would be same with regards to their body).

mikedilger · 2017-07-20T22:59:51Z

FYI I'm looking forward to the solution for this as well. My use case is a server with plugins. Consider a flate plugin that takes the Response prepared by a handler and modifies the body stream by compressing it.

As far as I can tell, I can't do this direclty in hyper 0.11. I have to instead define my handlers to return a different internal-to-my-crate type that looks like Response, and then after all the plugins, turn it into a hyper Response.

Twey force-pushed the master branch from 9fdad06 to 66fc57e Compare May 4, 2017 14:51

Twey force-pushed the master branch from 66fc57e to 071bd5a Compare May 4, 2017 16:26

Twey force-pushed the master branch from 071bd5a to 5ab4bc8 Compare May 5, 2017 12:21

Twey force-pushed the master branch from 5ab4bc8 to 23820e4 Compare May 5, 2017 14:50

carllerche mentioned this pull request May 15, 2017

Initial implementation of Request hyperium/http#22

Merged

2 tasks

seanmonstar mentioned this pull request Jun 17, 2017

Determine if Response body is populated #1216

Closed

seanmonstar closed this Jul 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Response more generic #1164

Make Response more generic #1164

Twey commented May 4, 2017

seanmonstar commented May 4, 2017

Twey commented May 4, 2017

seanmonstar commented May 4, 2017

Twey commented May 5, 2017 •

edited

Loading

seanmonstar commented May 9, 2017

seanmonstar commented May 10, 2017

Twey commented May 10, 2017

Twey commented May 10, 2017 •

edited

Loading

seanmonstar commented May 10, 2017

Twey commented May 15, 2017 •

edited

Loading

seanmonstar commented Jun 17, 2017

Twey commented Jun 18, 2017

seanmonstar commented Jul 13, 2017

Twey commented Jul 14, 2017

seanmonstar commented Jul 14, 2017

mikedilger commented Jul 20, 2017

Make Response more generic #1164

Make Response more generic #1164

Conversation

Twey commented May 4, 2017

seanmonstar commented May 4, 2017

Twey commented May 4, 2017

seanmonstar commented May 4, 2017

Twey commented May 5, 2017 • edited Loading

seanmonstar commented May 9, 2017

seanmonstar commented May 10, 2017

Twey commented May 10, 2017

Twey commented May 10, 2017 • edited Loading

seanmonstar commented May 10, 2017

Twey commented May 15, 2017 • edited Loading

seanmonstar commented Jun 17, 2017

Twey commented Jun 18, 2017

seanmonstar commented Jul 13, 2017

Twey commented Jul 14, 2017

seanmonstar commented Jul 14, 2017

mikedilger commented Jul 20, 2017

Twey commented May 5, 2017 •

edited

Loading

Twey commented May 10, 2017 •

edited

Loading

Twey commented May 15, 2017 •

edited

Loading