Lwt unix with connection

[vbmithr]

@paurkedal Could you please review this?

[paurkedal]

Thanks for the quick response!

I think the new API looks good, except I'm wondering about server vs client initiated close. If the server initiates, it is now possible to call close but if the client initiates, I think it will need to wait until the server responds with a close frame.

I'll read through your code more carefully, but I noted that closing of the output channel only occurs if the reason has been specified. You probably meant to close also when it is unspecified?

I tested the new version with the code I'm working on. I only need server-initiated close, and that seems to work correctly now. At least I don't see the lingering TCP sockets any more.

[paurkedal]

I made the following sketch earlier, not suggesting you should use it, but just to say that if I though tracking the close handshake could be left to the client, as I expected the client to already keep track of state:

module Client : sig
  type t

  val connect :
    ?extra_headers:Cohttp.Header.t ->
    ?random_string:(int -> string) ->
    ?ctx:Conduit_lwt_unix.ctx ->
    Conduit_lwt_unix.client ->
    Uri.t -> t

  val send : t -> Websocket.Frame.t -> unit Lwt.t

  val receive : t -> Websocket.Frame.t option Lwt.t

  val close_transport : t -> unit Lwt.t
  (** This closes the channels used by the websocket. No close frame is sent or
      expected by this call; it is up to the client to ensure the that the close
      request and response handshake has taken place before calling this
      function. *)
end

[vbmithr]

What about this? There is a lot of things missing/to improve in this lib, just trying to find a quick solution to your problem without rewriting everything ;)

[paurkedal]

For me this works nicely using the close_transport call.

I think the close call has a race condition, because even if we don't send any request, the server may send a frame (like a ping) right after we send close, which would cause the warning to trigger followed by an unclean close. Maybe https://github.com/paurkedal/ocaml-websocket/commit/4a894fb5a24e777ba11cc81e474922746dc70925 would do?

I appreciate the quick solution. I had a look at alternatives for Lwt users, and though there are several ones available, I think this project is still the best option. Hyper looks very promising, but seems not ready for production use yet (esp. I would need wss support).

[paurkedal]

There is an issue with wscat

wscat.exe: websocket_lwt_unix: Connected to http://localhost:9001
wscat.exe: wscat: Got EOF. Sending a close frame.
Fatal error: exception Websocket.Protocol_error("could not read payload")
Raised at Lwt.Miscellaneous.poll in file "src/core/lwt.ml", line 3068, characters 20-29
Called from Lwt_main.run.run_loop in file "src/unix/lwt_main.ml", line 31, characters 10-20
Called from Lwt_main.run in file "src/unix/lwt_main.ml", line 118, characters 8-13
Re-raised at Lwt_main.run in file "src/unix/lwt_main.ml", line 124, characters 4-13
Called from Dune__exe__Wscat in file "lwt/wscat.ml", line 112, characters 2-49

That must be because it now closes the socket before receiving the close response. I don't quite understand the code after the (* Immediately echo and pass this last message to the user *) comment, but I think the react loop will also need a state variable indicating that the connection was closed on the client side, so that it just calls close_transport instead of attempting a third close reply.

[vbmithr]

Yeah, it’s because there is a race condition between read_frame and the read_frame in close. I’m deleting close.

[vbmithr]

@paurkedal What about now?

[paurkedal]

This looks good to me apart from my note about wscat.