Skip to content

Conversation

@mkhq
Copy link
Collaborator

@mkhq mkhq commented May 11, 2016

Started on a new examples sub-project which can be used to showcase how to use finagle-websocket and as a way to try out the new API. If this does not belong in the main repo, please let me know.

This commit contains a console client that reads a line of text from stdin and sends it to an echo server. The server uses a ServiceFactory to keep track of websocket sessions.

I have a couple of points on the API:

  • It took some time to get used to AsyncStream, but after some trial and error I think it is better compared to the Offer/Broker approach.
  • When creating the client side Request, the SocketAddress is either set to null (from the other examples) or a new SocketAddress {}. Could we swap the input argument and provide a default value instead since this is more interesting to have access to on the server side?
  • In the example, the ServiceFactory is used to get access to the ClientConnection for reporting on when the connection closes. Is there another way of doing this or is it recommended to use a ServiceFactory? An alternative would be to extend the request with an onClose promise as was done before, what are the trade-offs for doing so?

@coveralls
Copy link

coveralls commented May 11, 2016

Coverage Status

Coverage remained the same at 73.529% when pulling b8305a6 on mkhq:examples-asyncstream-client-server into 34d4bd8 on finagle:develop.

@coveralls
Copy link

coveralls commented May 16, 2016

Coverage Status

Coverage remained the same at 73.529% when pulling 43c927b on mkhq:examples-asyncstream-client-server into 34d4bd8 on finagle:develop.

@luciferous
Copy link
Collaborator

Thanks @mkhq! I think it's nice to have examples, but we'll see what @sprsquish thinks.

I want to echo your thoughts about the SocketAddress on the client request. It doesn't really make sense, and I'm not happy with that API either. I have some ideas, but I want to flesh them out a little before presenting them.

The end of the AsyncStream means the same thing as connection closure. Because the stream is persistent, you can do something like this in the Service handler.

request.messages.foreach(_ => ()).ensure { println("Connection closing") }

@sprsquish
Copy link
Contributor

Hey guys, sorry it's been so long. I'm adding @luciferous as an admin collaborator to the repo, @mkhq you're already on the team. I lack sufficient time and motivation to continue this project.

Thanks for picking up the torch so far and bringing the library into the present.

@luciferous
Copy link
Collaborator

@sprsquish Thanks! I'll do my best to attend to these issues regularly. We'll see what happens.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants