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

Fix client. Add server. Add Lwt support. #1

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

andreas
Copy link
Collaborator

@andreas andreas commented Jan 5, 2019

Hi @seliopou,

I've been looking into using httpaf for ocaml-graphql-server, but that requires a supporting websocket library. After stumbling on websocketaf, I went down the rabbit hole of making websocketaf actually work (though there are still some deficiencies that need to be addressed!).

The initial state of the repo appeared to outline the direction you wanted to take the library in. I've tried to continue in that direction, for the most part keeping the signatures that were already in place, and further taking inspiration from httpaf. I've attempted to match the code style too (though you be the judge of that).

Broadly speaking, there are four areas of changes listed below.

Core

I've switched from jbuilder to dune, and introduced bigstringaf as a dependency (4efc11a, a5e591c).

The frame parsing and serialization logic in Websocket.Frame had a number of subtle bugs, primarily around the bit fiddling (e3d7a1e, bba1257). I've added a couple of tests which helped sort that out, though even more tests would be helpful indeed (8382d0e).

I've fixed up Reader, taking inspiration from httpaf, though it does not validate the frame yet, i.e. it operates on a parser of type unit Angstrom.t rather than (unit, 'error) result Angstrom.t (
d45840f). This could be a future improvement.

I've updated Wsd in a couple of ways (e0cf9bb):

  • Added a mode, which determines whether Wsd is acting as a client or server. This takes care of masking payloads appropriately, and means the optional argument ?mask is no longer required for Wsd.schedule and Wsd.send_bytes.
  • Added a function Wsd.when_ready_to_write, similar to Httpaf.Body.when_ready_to_write, which is used when a server connection yields writing (Server_connection.yield_writer).
  • Added functions for interacting with the "I/O loop" (next, report_result).

Client

I tried to complete the client code following the approach you had already set up: transitioning from Uninitialized to Handshake to Websocket (ed99164).

The most tricky part was transitioning from Handshake to Websocket: you need change the state to Websocket and then close the Httpaf.Client_connection.t in this particular order to trigger the Httpaf.Body.when_ready_to_write-hook which will continue the IO loop. This was quite the puzzle for me 😅

Server

Server support was needed for my own purposes, so I added that (65185b0). It follows the same approach as the client connection code by transitioning from Uninitialized to Handshake to Websocket:

  • Server_handshake is the initial websocket handshake, and is essentially a Httpaf.Server_connection.t. It could be arguably be omitted since it does not add additional functionality, however further improvements could change that.
  • Server_websocket handles the connection once its been upgraded to the websocket protocol. It uses Reader to read frames, and Wsd to send frames.
  • Server_connection is responsible for composing the behavior of Server_handshake and Server_websocket.

As with the client code, the trickiest part is transitioning from Handshake to Websocket in Server_connection. This is challenging because this transition should happen after the handshake response has been written to the wire. There is currently no hook in httpaf for this without writing a non-empty response, in which case Httpaf.Body.flush can be used. The current code writes a single space in the response body, but a better solution should replace this. My ideas are:

  • Add a flush function to Httpaf.Server_connection.
  • Count the number of bytes written in Server_handshake to determine when the response has been fully transferred.

What do you think? Maybe you have better ideas.

Lastly I should mention that the HTTP handshake request is currently not validated by the server, but obviously it should be (Server_connection.passes_scrutiny) -- I've considered it future work.

Lwt support

To actually test the client and server, I've implemented Lwt support as a new OPAM package, websocketaf-lwt (db0c615). It's basically copy/paste of httpaf-lwt. Error reporting is missing though (future work).

Finally, two examples are included (9ada40d):

  • examples/lwt/echo_server runs a websocket server and echoes frames back to clients. To run:

    dune exec examples/lwt/echo_server.ml
    

    You can then use a client, e.g. wscat from NPM, as follows:

    wscat localhost:8080
    
  • examples/lwt/wscat reads lines from stdin as text frames and sends to a websocket server. Received frames are written to stdout.

    dune exec examples/lwt/wscat.exe echo.websocket.org
    

Note that the two examples, wscat and echo_server, does not work together, since httpaf strictly disallows a body for 101 responses (other clients typically are more lenient).

Summary

I realise this is a massive PR, which is always unfortunate. I felt like I had to get to a working example -- interacting with external websocket clients and servers -- to validate that the code actually worked. If you would consider it helpful, I can split this PR into multiple PRs. Some of the changes are probably less contentious than others.
I know @anmonteiro has done further work to address some of the limitations listed above. I expect he'll follow up with another PR, or we can combine our changes if you prefer.

Let me know how to best proceed. Thanks! 🙏

@seliopou
Copy link
Member

Thanks for sending this over. Integrating this with httpaf properly requires some support for upgrading connections, which I hope to implement in next couple weeks. Once I get something working, I'll ping this issue again to see how easy it will be to adapt to it.

Thanks again!

@andreas
Copy link
Collaborator Author

andreas commented Apr 1, 2019

Np, thanks for the update 👍

This was referenced Apr 7, 2019
@seliopou
Copy link
Member

seliopou commented Apr 7, 2019

This is great, thanks for the clear description of the changes and the organized patches. It's making it very easy to evaluate your contributions and get them merged in.

I thought a good way to proceed was to first get master to actually build with the obvious fixes, and then start looking at the other parts. So to that end I've been cherry-picking some of the commits these commits in separate PRs to start moving master to that state.

@seliopou seliopou mentioned this pull request Apr 7, 2019
@andreas
Copy link
Collaborator Author

andreas commented Apr 20, 2019

Glad that it's helpful 😄 Let me know if I can help in any way.

@rbjorklin
Copy link

Hello! I'm looking at using websockets in a project and this PR looks like a promising contribution. What's the status here?

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.

3 participants