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

Stream endpoint support for servant #836

Merged
merged 11 commits into from Dec 3, 2017

Conversation

Projects
None yet
5 participants
@gbaz
Copy link
Contributor

commented Oct 18, 2017

This addresses #271.

I'm adding it here rather than servant-contrib because I think it is basically a correct and complete solution in sufficient generality to be in core. I especially want that so that it is easier for downstream things like servant-cassava and servant-swagger can add support for it easily. The big advantage of this over the one-off solutions in that thread is that it lets you have fully typed streaming endpoints whose types capture both the data being streamed back and the delimiting (or "framing") strategy for the sequence as a whole.

I'm happy with the general approach, and wanted to submit the PR early to get some feedback.

That said, here's my caveats/todos thus far:

  1. The names for framing strategies etc could be bikeshedded arbitrarily. I'm not sure if there's better industry standard terminology to use.
  2. More basic strategies like Netstrings could be added.
  3. The MkLink/HasLink instances haven't been added yet.
  4. Client/consumer code could be added, which might require extending framing strategies with parsing tools as well. Question: Would we then want two typeclasses (as with mimerender/unrender) or is leaving it as one fine?
  5. There's no provision at the moment for attaching custom headers. This can be added just as with Verb, it just hasn't been yet.

Note: one choice I made was to only take a single content-type rather than a list. This is because the framing strategies and content types tend to have a mutual interaction, and it would take a great deal more type-level machinery to capture that. Furthermore, I don't know of this case existing in the real world -- streaming services tend not to be encoding-polymorphic.

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

tl;dr: I'm pretty happy with the approach and personally agree that this could be added to the core packages, given how generic it is.


To address specific points of your comment:

The big advantage of this over the one-off solutions in that thread is that it lets you have fully typed streaming endpoints whose types capture both the data being streamed back and the delimiting (or "framing") strategy for the sequence as a whole.

This is indeed very nice. It basically mirrors what we have for endpoints that return non-streaming responses, and therefore follows the principle of least surprise.

More basic strategies like Netstrings could be added.

If it's such a common one that users are likely to need it, it would be nice to have it.

Client/consumer code could be added, which might require extending framing strategies with parsing tools as well. Question: Would we then want two typeclasses (as with mimerender/unrender) or is leaving it as one fine?

I think we would indeed at least want support for -client either in this PR or soonish (before the next release or in the next one, say), unless it represents an unreasonable amount of work (servant-client might be less prepared for getting streaming support). And I think we probably would want to mirror the MimeRender/MimeUnrender separation by having two separate typeclasses, if only because it's nice that when you're just writing clients to an API, you don't even have to think about anything server-related (in this case, how the server streams the data your way), even if the code might not be that hard to write. It would be nice to have the Stream machinery align with the rest in this regard don't you think? Not to mention that some users might have some code around for going one way but not the other, even though this is a pretty arbitrary claim not backed by anything.

There's no provision at the moment for attaching custom headers. This can be added just as with Verb, it just hasn't been yet.

That would be nice to have too. I believe we could e.g stream zip archives and use the Content-Disposition header to have the client browser download it, for example, without too much work. But I don't think this instance represents much work anyway. I could add it if you don't get around to it.

Note: one choice I made was to only take a single content-type rather than a list. This is because the framing strategies and content types tend to have a mutual interaction, and it would take a great deal more type-level machinery to capture that. Furthermore, I don't know of this case existing in the real world -- streaming services tend not to be encoding-polymorphic.

Sounds reasonable to me. I think this is a situation where "let's think about it when a user asks for it" applies.


I have a question leading to another (not necessarily requests, just questions for now really) that I think have not been addressed by your post: do you have any plans for providing "builders" of StreamGenerator values? So that upon seeing the docs, users could just use a builtin function to produce their first streaming response with this without having to care about what a StreamGenerator actually is just yet. They could then go back to the docs, read on and learn more about StreamGenerator and how to write their own, or use existing fancier ones -- any plans for those? The usual streaming libraries come to mind,

I know this is an early patch, but I'll mention it anyway so that we don't forget later: we probably want a little bit more documentation before this is released, and probably a dedicated section in the -server bit of the tutorial. The latter could be added in another PR (and I might be able to do it if you really don't feel like taking care of it), but we definitely want some more haddocks before merging, so that this part is at least already taken care of.

Finally, just a suggestion for the "polish" phase (i.e for later): I think we might want to provide GetStream, PostStream, ... synonyms to make this even simpler to approach/use. Less characters to type, one less type parameter to worry about, and that's after all what we do for endpoints with non-streaming responses.

-- * Streaming endpoints, distinguished by HTTP method
module Servant.API.Stream,

-- * Authentication

This comment has been minimized.

Copy link
@phadej

phadej Oct 19, 2017

Member

indentation off

instance ( MimeRender ctype a, ReflectMethod method, Framing framing ctype
) => HasServer (Stream method framing ctype a) context where

type ServerT (Stream method framing ctype a) m = m (StreamGenerator a)

This comment has been minimized.

Copy link
@phadej

phadej Oct 19, 2017

Member

I think we should have something more low-level as a base primitive, maybe as low as a conversion from a to StreamingBody

You will be able to make StreamGenerator an instance of above class

This comment has been minimized.

Copy link
@alpmestan

alpmestan Oct 19, 2017

Contributor

Could you give a use case?

@phadej

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

I have to think a bit. I like this.

To rephrase myself: I'd like to have something servant which would let me return Conduit or Pipe or Machine (by writing plumbing instance).

It feels that StreamGenerator is another Source we can read chunks of to put on wire.

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2017

All of @alpmestan's comments I agree with, and am basically happy to do, over some course of time. I'll also add some sort of ArbitraryBoundaryStrategy which holds any old function on bytestrings, as it was pointed out to me there are some weird framing protocols that do their own escaping and transformations of the payload (jabber, applescript, maybe others). So we might as well have an escape hatch for them, just for generality's sake.

I can see what @phadej wants in a sense, but I'm not quite sure if I can make the typeclass instances work out although I'm happy to give it a shot. It would certainly be nice to be polymorphic over that end result, and it would make writing adapters easier. But without more deps in the tree, I don't think any of those adapters could live in the base packages regardless, so we'd still want a StreamGenerator or something.

So what I'll provide, if I can make it work, won't be a more low-level primitive (we want to abstract over the ability to handle different framings, etc ourselves) but it should nonetheless allow passing back conduits or pipes of a directly.

gbaz added some commits Oct 19, 2017

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2017

First round of changes are done. Netstring is added. Framing now has FramingRender and FramingUnrender. We now are able to be polymorphic in the output type. All Conduit or whatever needs to implement is the new ToStreamGenerator typeclass, which should be pretty straightforward. As to where those instances live, I defer that to later :-) And if someone wants to return a StreamGenerator directly, that works too.
I also added the Header instance. Note how I did it -- with a single helper function and passing in a splitHeaders function. This same refactor could be applied to consolidate methodRouterHeaders, methodRouter and processMethodRouter into a single function and remove some code duplication there, if people like it and want to do it.

The MkLink stuff is also added.

Here's what's undone:

  • servant-client
  • documentation
  • tests
  • polish with type synonyms for StreamGet etc.

I took a quick look at servant-client but haven't wrapped my head around it yet, so am not sure exactly how hard it is to implement...

@phadej

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

@gbaz could you submit splitHeaders as a separate PR before this, I'm happy to merge it right away

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Great, nice to see this shape up so quickly.

Regarding servant-client, let me quickly explain how it works.
Here you can find the HasClient class, which is used to traverse API types and add arguments to the client functions as it encounters Capture, ReqBody etc while updating a Request.

The said Request type is defined here. In particular, you can see that it does not have any room for storing anything about the response body, all it knows about it is the content types the client function will accept.

A Request is then turned into a Response using runRequest, which is a typeclass method to allow for the use of different http client libraries.

A Response, with our current representation, has to be a lazy bytestring. I imagine you might want to add a constructor to represent streaming responses? Or support that some other way. But that's not enough, because runReader implementations would have absolutely no way to know when they should present the response as a stream and when they should not. I therefore think we might want to add a flag in Request to precisely inform runReader implementations about whether a given request should yield a streaming response or not.

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

I committed a hasclient instance for stream, but it works on strictly produced bodies. I'm playing with making it lazy, but the issue is because we need to manage opening and closing the connection explicitly, we can't pass through lazy IO anywhere, and instead need to make endpoints for these things weird continuation creatures themselves, which is a bit unfortunate.

terminate :: Proxy strategy -> Proxy a -> ByteString
header :: Proxy strategy -> Proxy a -> ByteString
boundary :: Proxy strategy -> Proxy a -> BoundaryStrategy
trailer :: Proxy strategy -> Proxy a -> ByteString

This comment has been minimized.

Copy link
@3noch

3noch Oct 21, 2017

Could Proxy be polymorphic here (e.g. proxy strategy instead)?

This comment has been minimized.

Copy link
@phadej

phadej Oct 22, 2017

Member

then it should be proxy strategy -> proxy2 a -> .... I have no strong opinion on this.

This comment has been minimized.

Copy link
@phadej

phadej Oct 22, 2017

Member

Addition to above: I don't think users will be calling these methods often, compared to e.g. safeLink or serve.

This comment has been minimized.

Copy link
@gbaz

gbaz Oct 23, 2017

Author Contributor

I see Proxy pretty widespread throughout the codebase, and these are mainly internal usage functions, in terms of the callers, so I don't see any benefit to moving to a more polymorphic Proxy starting here tbh. If the servant maintainers wanted to do it everywhere, that would be fine. But this isn't a place I'd think to start doing it in particular...

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2017

Ok now I have a proper streaming client thing that typechecks. I'd like to add some end-to-end tests to see that the server and client talk as I think they should. Can someone point me to the right place in the repo to do that sort of thing?

ServantError (..))

class (Monad m) => RunClient m where
-- | How to make a request.
runRequest :: Request -> m Response
streamingRequest :: Request -> m StreamingResponse

This comment has been minimized.

Copy link
@alpmestan

alpmestan Oct 25, 2017

Contributor

The name is a little bit confusing, as it's the response that's streamed to the client, not the request streamed to the server. Don't you think?

This comment has been minimized.

Copy link
@gbaz

gbaz Nov 4, 2017

Author Contributor

what name do you suggest?

This comment has been minimized.

Copy link
@gbaz

gbaz Nov 4, 2017

Author Contributor

streamedRequest? that has the same "which direction" problem in reverse imho.

This comment has been minimized.

Copy link
@alpmestan

alpmestan Nov 4, 2017

Contributor

Hmm. runRequestStreaming, streamingResponse, runStreaming? They all kind of suffer from the "which direction" problem though. So forget what I said. Unless someone comes up with a better suggestion, I'm fine with that name for now =)

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2017

This is looking pretty good! I just have one nitpick on naming, but am very happy with the PR. I also saw there's a call to error left somewhere, but I assume it's meant to go by the time we merge this.

Regarding tests, I'd probably recommend a dedicated test tree in servant-client's test suite: https://github.com/haskell-servant/servant/blob/master/servant-client/test/Servant/ClientSpec.hs

This brings in both servant-server and servant-client and tests many clients against matching servers. I would however recommend doing the streaming testing in another module, unless you see a good reason not to.

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2017

Ok, I have tests now, and I made them pass, so I feel much better. That leaves three things.

  1. docs (where, how?)
  2. that name
  3. fixing that one error call in HasClient. I haven't thought through what's even possible to do there that's better -- by the time we see the status we're beginning to deliver results already, and arguably just throwing a decent exception [suggestions on which?] is cleaner than adding another Either layer to an already complicated type.

And then... I think its done!

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2017

Ugh, the build fails on 7.8.4 with:

src/Servant/API/Stream.hs:22:18:
    Could not find module Data.Bifunctor
    Perhaps you meant Data.Functor (from base)
    Use -v to see a list of the files searched for.

Other than that, I'm very happy with this PR!

@phadej

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I tag this for 0.13, let's have 0.12 soon.

@phadej phadej added this to the 0.13 milestone Nov 6, 2017

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2017

Regarding the docs, we usually like to have reasonable haddocks for combinators with an example or two. And then we'll want to augment the tutorial, whose code and text lives at https://github.com/haskell-servant/servant/tree/master/doc/tutorial

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

I don't see anything to be done more on the haddocks tbh, the Stream haddocks look just about as complete as the Verb ones?

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2017

I didn't say they aren't (didn't look again at the haddocks recently), just stated again what the "standard" is. :) But again, the tutorial update can come in another PR.

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

Ok, and here's a first pass at a tutorial change: #852

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2017

Great, gave it a review. It's pretty close to mergeable. We'll probably end up merging this and the tutorial PR as soon as @phadej is done branching off the 0.12 release, which means well have the Stream stuffs in the next one (which doesn't mean it won't ship for another 6 months, don't worry, frequent releases actually make our lives easier).

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2017

@phadej Any objection to merging this and the accompanying tutorial PR?

@phadej phadej merged commit cbd3862 into haskell-servant:master Dec 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@domenkozar

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

@gbaz have you used this with pipes/conduit - do you have an example how to integrate? Thanks!

@gbaz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

No I don't have integration code lying around. The basic idea is to just give an instance for BuildFromStream and ToStreamGenerator to the desired iteratee-like type (i.e. Pipe or Conduit or whatever). After that, the conversion should be handled for you. Ideally someone who wanted to integrate with those ecosystems could write and upload a supporting package giving the missing instances. (And the instances themselves might be a bit subtle to write -- but on the other hand, I imagine you can get pretty far by just following the types).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.