Usage with Mirage

[andreas]

Currently websocket-lwt depends on cohttp-lwt-unix, which makes it incompatible with Mirage. Do you have any thoughts on what it would take to implement a version that's compatible with Mirage, i.e. using cohttp-lwt and mirage-conduit?

[andreas]

I've looked further into this, and it appears to be quite easy, actually. Here's the diff between lwt and a local copy I've made Mirage-compatible:

------ lwt/websocket_lwt.ml
++++++ mirage/websocket_mirage.ml
@|-23,1 +23,1 ============================================================
-|module Lwt_IO = IO(Cohttp_lwt_unix.IO)
+|module Lwt_IO = IO(Cohttp_lwt.IO)
@|-26,2 +26,2 ============================================================
-|module Request = Cohttp.Request.Make(Cohttp_lwt_unix.IO)
+|module Request = Cohttp.Request.Make(Cohttp_lwt.IO)
-|module Response = Cohttp.Response.Make(Cohttp_lwt_unix.IO)
+|module Response = Cohttp.Response.Make(Cohttp_lwt.IO)
@|-35,1 +35,1 ============================================================
-|    flow: Conduit_lwt_unix.flow;
+|    flow: Conduit_mirage.Flow.flow;
@|-95,1 +95,1 ============================================================
-|    | Vchan of Conduit_lwt_unix.vchan_flow
+|    | Vchan of Conduit_mirage.vchan_flow
@|-99,3 +99,3 ============================================================
-|    | Conduit_lwt_unix.TCP tcp_flow ->
+|    | Conduit_mirage.TCP tcp_flow ->
-|      TCP (tcp_flow.Conduit_lwt_unix.ip, tcp_flow.Conduit_lwt_unix.port)
+|      TCP (tcp_flow.Conduit_mirage.ip, tcp_flow.Conduit_mirage.port)
-|    | Conduit_lwt_unix.Domain_socket { path; _ } ->
+|    | Conduit_mirage.Domain_socket { path; _ } ->
@|-103,1 +103,1 ============================================================
-|    | Conduit_lwt_unix.Vchan flow ->
+|    | Conduit_mirage.Vchan flow ->
@|-110,1 +110,1 ============================================================
-|  let open Conduit_lwt_unix in
+|  let open Conduit_mirage in
@|-144,1 +144,1 ============================================================
-|    ?(ctx=Conduit_lwt_unix.default_ctx)
+|    ?(ctx=Conduit_mirage.default_ctx)
@|-155,1 +155,1 ============================================================
-|    Conduit_lwt_unix.connect ~ctx client >>= fun (flow, ic, oc) ->
+|    Conduit_mirage.connect ~ctx client >>= fun (flow, ic, oc) ->
@|-214,1 +214,1 ============================================================
-|    ?(ctx=Conduit_lwt_unix.default_ctx) ~mode react =
+|    ?(ctx=Conduit_mirage.default_ctx) ~mode react =
@|-256,1 +256,1 ============================================================
-|  Conduit_lwt_unix.serve ?on_exn ?timeout ?stop ~ctx ~mode begin fun flow ic oc ->
+|  Conduit_mirage.serve ?on_exn ?timeout ?stop ~ctx ~mode begin fun flow ic oc ->
@|-273,1 +273,1 ============================================================
-|    ?on_exn ?check_request ?(ctx=Conduit_lwt_unix.default_ctx) ~mode react =
+|    ?on_exn ?check_request ?(ctx=Conduit_mirage.default_ctx) ~mode react =

------ lwt/websocket_cohttp_lwt.ml
++++++ mirage/websocket_cohttp_mirage.ml
@|-21,1 +21,1 ============================================================
-|module Lwt_IO = Websocket.IO(Cohttp_lwt_unix.IO)
+|module Lwt_IO = Websocket.IO(Cohttp_mirage.IO)
@|-63,1 +63,1 ============================================================
-|          | Conduit_lwt_unix.TCP tcp ->
+|          | Mirage_conduit.TCP tcp ->

Though we can certainly duplicate a bit of code, the question is whether this warrants extracting the common code to a functor.

@vbmithr, how would you prefer to approach this? I'm happy to help with the implementation.

Finally, Websocket_cohttp_lwt does not reveal that Frame is an alias of Websocket.Frame. This makes it hard to use the library in an IO-agnostic manner since Websocket.Frame and Websocket_cohttp_lwt.Frame are not considered equal. Any concerns about simply having module Frame = Websocket.Frame in Websocket_cohttp_lwt (like for Websocket_async)?

[vbmithr]

I just happened to look into this right now. I'm trying to see what I can.

[vbmithr]

Conduit_mirage interfaces are not compatible with Conduit_lwt_unix unfortunately. Straightforword functorization is not possible. It would be nice if Conduit would be more homogenous.
For the last question, yes sure, it should actually been this way.

[vbmithr]

Ok, I have refactored to the maximum. Either copy Websocket_lwt_unix to make a Mirage version or better, try to put all common parts in a functor with an argument that would abstract out the if it's Mirage or UNIX… this would probably be better done in Conduit itself…

[andreas]

Thanks for the refactoring, @vbmithr!

So my initial diff was terribly wrong, and this turned out be a much bigger rabbit hole than I anticipated 😅

After looking more closely at the code from a perspective of adding a Mirage-compatible websocket server without duplicating code, I made the following observations:

• The current Lwt implementation is not using Cohttp_lwt_unix.Server to handle reading requests and writing responses, but rather works at the connection level using Conduit_lwt_unix. This appears to make some things more complicated, since the server-loop has to be reimplemented.
• Websocket_cohttp_lwt is not used for upgrading requests in Websocket_lwt_unix, so the upgrade logic appears to be implemented twice (slightly differently).
• Handling clients with Websocket_lwt_unix requires a function Connected_client.t -> unit Lwt.t, while Websocket_cohttp_lwt handles clients via a function for receiving frames (Frame.t -> unit) and another function for sending frames (Frame.t option -> unit). This means the Connected_client abstraction is not reused.

I might be missing something, or maybe there's a reason why the API and implementation has ended up like this. I'd be curious to learn if you have the time to elaborate. I've started dabbling with implementing some changes to address the above, but it's still unfinished. Unfortunately these changes are quite intrusive and can probably not be backwards-compatible 😞

[vbmithr]

On 10/24/18 7:27 PM, Andreas Garnaes wrote: Unfortunately these changes are quite intrusive and can probably not be backwards-compatible 😞

I don't care about backward compat. If your changes are good I will merge them. Best, Vincent

[vbmithr]

On 10/24/18 7:27 PM, Andreas Garnaes wrote: I might be missing something, or maybe there's a reason why the API and implementation has ended up like this.

I have not written the cohttp lwt part. They guy that wrote it probably needed something for him and did not try to be compatible with the rest of the code, I guess.

[andreas]

A quick follow-up on this issue...

I've worked on adding "response actions" to Cohttp (merged PR), which exposes the underlying input/output channels to the request handler. For the websocket server part of this repo, it means:

1. All the HTTP request handling logic can be removed.
2. With light functorization, the library can be agnostic to Lwt/Async.

Instead of trying to make modifications directly to this repo, I've included an edited version of websocket handling logic in graphql-cohttp with the above ideas in mind (work-in-progress PR). I think it's turned out rather well, but there are more details I'd like to iron out before trying to contribute upstream.

If you have any input or ideas, I'd be happy to hear.