Open andreas opened 5 years ago
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
)?
I just happened to look into this right now. I'm trying to see what I can.
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.
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…
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:
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).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 😞
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
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.
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:
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.
Currently
websocket-lwt
depends oncohttp-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. usingcohttp-lwt
andmirage-conduit
?