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

Streaming JSON Requests #667

Closed
vkostyukov opened this issue Nov 8, 2016 · 6 comments
Closed

Streaming JSON Requests #667

vkostyukov opened this issue Nov 8, 2016 · 6 comments

Comments

@vkostyukov
Copy link
Collaborator

Right now (as per 0.11) it's possible to stream JSON objects over the HTTP response (chunked) via AsyncStream[CaseClass]. Although, there is near zero support for the inbound streaming (streaming requests). This issue is related to #336.

I assume, for each JSON library we support, we can provide meanings for converting AsyncStream[Buf] into AsyncStream[CaseClass]. This might require a new type class for decoding since the Decode abstraction we have goes from Buf to Try[A]. Technically, we could utilize that and build something close to Netty's replying decoder (return Throw when we need more data), but in that case, it would be impossible to distinct streaming and non-streaming decoders (in case if we need too).

Anyways, there are lots of open questions here, but it seems totally possible to me (with zero API changes).

NOTE: @travisbrown has kindly agreed to find some time and work on iteratees support for Circe. This would mean that for Circe there will be two streaming APIs via AsyncStream and via iteratees (see #557).

@vkostyukov vkostyukov added this to the Finch 1.0 milestone Nov 8, 2016
@vkostyukov
Copy link
Collaborator Author

I think the path forward here is to push this kind of functionality out of the finch-core and use something like fs2 and iteratee instead. There is a PR that introduces iteratee support in Finch (see #557) - this is what we need to be focusing on.

I still want to keep the very basic AsyncStream support in finch-core for the sake of simplicity. Although I'm not 100% sure that Endpoint[AsyncStream[User]] (as a streaming JSON response) is the right pattern here.

@sergeykolbasov
Copy link
Collaborator

sergeykolbasov commented Jul 12, 2017

I've picked it up and made some good progress. My approach is following:

  • Get AsyncStream[Buf] from request
  • Build Enumerator[Future, Buf] from it
  • Use circe-streaming and transform Buf into Json and then Json into some A
  • Use Iteratee.fold to build AsyncStream[A]

I guess it'll simplify life of potential users by hiding Iteratee/Enumeratee stuff under the hood.

Have to think about API, probably it's possible to reuse asyncBody with type parameter. I'll make PR soon.

@vkostyukov
Copy link
Collaborator Author

Thanks @imliar! This is really cool!

One thought: I think we shouldn't try sticking to AsyncStream[_] API so hard. In fact, I'd prefer us using Circe/Iteratee abstractions instead (I honestly don't think AsyncStream[_] is a great tool for this job). I know we already support serving AsyncStream[Foo] (as a streaming JSON), but I really think we should reconsider that (and probably cut the support). Instead, we should probably support serving Iteratee (or something).

It thinks there is nothing wrong with just AsyncStream[Buf], but this is probably as far as we should go with async-streams.

As far as the JSON streaming goes, we should probably build everything within finch-circe for now (using both circe-streaming and iteratee). We can provide something like jsonAsyncBody[A] or asyncJsonBody[A] (that reuses asyncBody from there for now). What do you think @imliar?

@sergeykolbasov
Copy link
Collaborator

@vkostyukov

After dealing with Iteratee/Enumeratee for some time I have a feeling that it's complicated thing, so with exposing em outside this complexity will be shifted to finch users. Not sure about it.

Why AsyncStream can't be used for this in your oppinion?

@vkostyukov
Copy link
Collaborator Author

We've been having lots of issues w.r.t. resource management in AsyncStream (also memory leaks just to mention a few). Speaking of performance, I'm quite confident it's way slower than Iteratee/Enumeratee abstractions (we can do some benchmarking if you will).

I think it's fine to expose Iteratee/Enumeratee powered endpoints withing a Circe context since it's what Crice uses itself. We'll see how much adoption it gets and then make a final decision whether or not we need to stick with AsyncSteam anymore. An ideal end state, in my minds eye, is to have AsyncStream[Buf] support within finch-core, but provide more fine-grained abstractions for each/some of the JSON libraries (Iteratee/Enumeratee for finch-circe is a good start).

I think @travisbrown may have some insight on AsyncStream vs Iteratee/Enumeratee performance.

@vkostyukov
Copy link
Collaborator Author

I think we can close this now, after almost a year (h/t @imliar).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants