Direct-style websockets with Eio#130
Conversation
talex5
left a comment
talex5
left a comment
There was a problem hiding this comment.
I don't know anything about web-sockets, but the Eio stuff looks reasonable to me. I suspect you don't need most of the Buf_write.flush calls, though. The buffer gets flushed automatically when you block; flush just makes the caller wait until the data has finished flushing but doesn't speed it up (any more than calling Fiber.yield () would, anyway).
|
Is there anything holding back this PR for merge? Anything I can perhaps do to help push it over the line? |
Thanks for the interest and offer for help @ryanslade. I would say this PR is blocked on landing a proper release of cohttp-eio. Right now, it is an |
|
Hi. Cohttp 6.0.0~beta1 is out and it has |
|
I tried to build this PR with cohttp-eio.6.0.0~alpha2 but stuck with compilation error.
@patricoferris have things become better in last year? |
|
Quickly bump to work with cohttp-eio.6.0.0~beta2 should make this work with the |
|
Are there any updates about this? It looks like cohttp-eio is out of beta. Depending on how much time I end up having over the next couple weeks, I might be able to try to spend some time with it, if that could be useful. |
copy
left a comment
copy
left a comment
There was a problem hiding this comment.
I've tested this and it works well.
I left a few suggestions. Otherwise, this looks good to merge to me.
|
|
||
| let write_frame_to_buf ~mode buf fr = | ||
| let open Frame in | ||
| let content = Bytes.unsafe_of_string fr.content in |
There was a problem hiding this comment.
This is later written to (the xor call below), so it should use Bytes.to_string, at least in the client case.
| (public_name websocket-async) | ||
| (modules websocket_async) | ||
| (optional) | ||
| (libraries websocket logs-async cohttp-async)) |
There was a problem hiding this comment.
Could you remove this change?
| match read_exactly ic payload_len with | ||
| | None -> proto_error "could not read payload (len=%d)" payload_len | ||
| | Some payload -> | ||
| let payload = Bytes.unsafe_of_string payload in |
There was a problem hiding this comment.
I don't think Bytes.unsafe_of_string is safe here.
| @@ -0,0 +1 @@ | |||
| (vendored_dirs ocaml-cohttp) | |||
There was a problem hiding this comment.
This shouldn't be needed any more.
| process incoming websocket frames, and returns ([response_action], | ||
| [push_frame]) where [response_action] is used to produce a | ||
| {!Cohttp_lwt.Server.t} and [push_frame] is used to send websocket | ||
| frames to the client. *) |
There was a problem hiding this comment.
I realise this matches the Lwt API, but maybe this is an opportunity to come up with a better signature. The current implementation requires us to write odd constructions to get a handle of the push_frame inside of the incoming_handler.
May I suggest:
type connection
val send : connection -> Frame.t -> unit
val recv : connection -> Frame.t
val close : connection -> unit
val upgrade_connection :
Http.Request.t ->
(connection -> unit) ->
Cohttp_eio.Server.response_action
(** [upgrade_connection req callback] takes [req], a connection request, and
[callback], a function that will be called when the websocket upgrade is
successful is established and returns [response_action] which is used to
produce a {!Cohttp_eio.Server.t}. *)| | None -> raise End_of_file | ||
| | Some hdr -> read_frame ic oc mode hdr | ||
|
|
||
| module Connected_client = struct |
7 participants
This PR adds direct-style websockets using eio and cohttp-eio (yet unreleased, but it has been merged in the main branch of mirage/ocaml-cohttp). It is WIP and probably will change, but I thought I would open this PR in case I don't have time to come back to it later.
cc: @bikallem @talex5 who might be able to comment on the core IO implementation a little :))