server: modernize websockets

[luciferous]

Move off Offer/Broker, preferring AsyncStream.

In this model, servers receive an AsyncStream representing the frames of an incoming WebSocket client. The server responds with an AsyncStream of frames to be written to the client. When the response ends, the dispatcher knows to close the transport. This has the effect of discarding incoming frames.

Still working on (highest to lowest priority):

• [x] Update tests
• [x] Docs and comments
• [x] Get feedback for API for Request and Response

Related: #10

[luciferous]

@sprsquish

[luciferous]

Bare bones test in Scala REPL.

import com.twitter.finagle.Websocket
import com.twitter.finagle.Service
import com.twitter.util.Future
import com.twitter.finagle.websocket._
val echo = new Service[Request, Response] {
  def apply(req: Request) = Future.value(Response(req.messages))
}
Websocket.serve(":3000", echo)

In another terminal.

$ wscat -c ws://localhost:3000/
> hello

< hello
> world

< world

[sprsquish]

This is great. I like the direction this is going. I think it'd be good to handle ping/pong frames, too. I think you capture this in the improve API bullet, but Request should have the URI and headers.

Thanks for this, it's way nicer than my solution. :)

[luciferous]

@sprsquish thanks for the feedback – I really think your connection-oriented API is something to work towards though: Service[Req, Rep] isn't intuitively "streaming". We can talk more about what I mean by this if it's not clear.

OK, I was hoping you would have some opinions on the API. I'll expose the Ping/Pong frames. Let me know of other thoughts for improvement as they come to you.

[sprsquish]

When I think of a socket, I generally think of something I can write to and read from. To that end, I think it'd be nice to either be able to provide a writeable request or receive a writeable socket. Reading from an AsyncStream is intuitive, writing to one is less so.

I think receiving something that's read/write-able after the connection's been established makes more sense. Any ideas for how that API might look?

[luciferous]

I have a few ideas, but I'd like to put them in another PR or discuss them in chat. Here are the types I'm thinking of:

sealed trait App[+T]
case object Read extends App[Frame]
case class Write(frame: Frame) extends App[Unit]
[...]

def toService(app: App[Unit]): Service[Request, Response]

This PR is focused on providing just the "plumbing" layer. I like your read/writable socket API, let's keep chatting about that after we figure out this basic stuff first.

[luciferous]

Deferring Websocket.withHttp(httpService).

[luciferous]

What use cases did you have in mind by exposing the Ping/Pong frames to the user?

[vkostyukov]

Just wanted to add my 5 cents.

I really like the API (and implementation) @luciferous suggested here. I think it should be easy to reason about WebSockets in terms of AsyncStream.

I'm personally eager to see this merged and finally build a first class WebSockets support in Finch. People keep asking about that, but I don't have a good answer yet.

[sprsquish]

@luciferous A previous PR had implemented keep alive via ping/pong. My thinking was that it'd be nicer to provide a KeepAlive and a PongResponder filter that could be composed onto the service. That feels like it fits the Finagle aesthetic better than if we were to try and thread all that logic through the client/server.

@vkostyukov Always glad to have more input! Thanks for providing feedback.

[luciferous]

@sprsquish Ok, that seems reasonable. It does make the API a little bit low level, but maybe that is the right choice if it will be a nice stable base to build a higher level socket-like API on top.

[sprsquish]

@luciferous yea, that's what I was going for. It makes it similar to other Finagle libraries. You get raw(-ish) protocol messages. What gets built on top of those is up to the user. My goal is to be simple, powerful, and low maintenance. :)

[cquiroz]

Would WebSockets be able to run in the same port than Finch Rest endpoints? I'd like to have a single server that serves both regular end points and websockets

[sprsquish]

I created the develop branch and made it the default. I'll merge this into there manually when we're all good with it.

:shipit: from me. I like where this is going. @vkostyukov, @mkhq, any thoughts?

[mkhq]

Really nice work! I added the keepAlive to the existing code, this way is however much better.

I have a comment on the Request. We are using the remoteAddress, would it be possible to include that also?

[luciferous]

@mkhq Added remoteAddress.

[luciferous]

@cquiroz Not currently. I think it's worth further discussion. Design ideas welcome.

[luciferous]

@sprsquish When this is ready, let me know if you want me to squash into a single commit.

[coveralls]

Coverage increased (+4.3%) to 70.946% when pulling 0372829431aef546e08c22a7d2ee330667adf032 on luciferous:neuman/server into d549afcc8ee0929d7397d160da9ec34aa888257e on finagle:master.

[sprsquish]

:+1: looks great.

Final comments @vkostyukov, @mkhq?

[luciferous]

@sprsquish This PR is un-squashed right now, but I think it'd be good to git merge --squash into develop to keep the history clean. I think the PR description works well enough (with TODO list omitted), please feel free to use it and add to it where you feel it's necessary.

[vkostyukov]

LGTM! Thanks @luciferous, @sprsquish!

[sprsquish]

merged. thanks @luciferous for the code and everyone else for the feedback!