-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
Conversation
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! |
Np, thanks for the update 👍 |
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. |
Glad that it's helpful 😄 Let me know if I can help in any way. |
Hello! I'm looking at using websockets in a project and this PR looks like a promising contribution. What's the status here? |
Hi @seliopou,
I've been looking into using
httpaf
forocaml-graphql-server
, but that requires a supporting websocket library. After stumbling onwebsocketaf
, I went down the rabbit hole of makingwebsocketaf
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
todune
, and introducedbigstringaf
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 fromhttpaf
, though it does not validate the frame yet, i.e. it operates on a parser of typeunit 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):mode
, which determines whetherWsd
is acting as a client or server. This takes care of masking payloads appropriately, and means the optional argument?mask
is no longer required forWsd.schedule
andWsd.send_bytes
.Wsd.when_ready_to_write
, similar toHttpaf.Body.when_ready_to_write
, which is used when a server connection yields writing (Server_connection.yield_writer
).next
,report_result
).Client
I tried to complete the client code following the approach you had already set up: transitioning from
Uninitialized
toHandshake
toWebsocket
(ed99164).The most tricky part was transitioning from
Handshake
toWebsocket
: you need change the state toWebsocket
and then close theHttpaf.Client_connection.t
in this particular order to trigger theHttpaf.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
toHandshake
toWebsocket
:Server_handshake
is the initial websocket handshake, and is essentially aHttpaf.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 usesReader
to read frames, andWsd
to send frames.Server_connection
is responsible for composing the behavior ofServer_handshake
andServer_websocket
.As with the client code, the trickiest part is transitioning from
Handshake
toWebsocket
inServer_connection
. This is challenging because this transition should happen after the handshake response has been written to the wire. There is currently no hook inhttpaf
for this without writing a non-empty response, in which caseHttpaf.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:flush
function toHttpaf.Server_connection
.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 ofhttpaf-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:You can then use a client, e.g.
wscat
from NPM, as follows:examples/lwt/wscat
reads lines from stdin as text frames and sends to a websocket server. Received frames are written to stdout.Note that the two examples,
wscat
andecho_server
, does not work together, sincehttpaf
strictly disallows a body for101
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! 🙏