-
Notifications
You must be signed in to change notification settings - Fork 177
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
cohttp-eio: client: use permissive argument type for make_generic #1026
cohttp-eio: client: use permissive argument type for make_generic #1026
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me to relax this.
(this wasn't spotted before because make ~https
can be used for TLS support in most cases)
cohttp-eio/src/client.mli
Outdated
@@ -24,6 +24,6 @@ val make : | |||
- URIs of the form "httpunix://unix-path/http-path" connect to the given | |||
Unix path. *) | |||
|
|||
val make_generic : (sw:Switch.t -> Uri.t -> _ Eio.Net.stream_socket) -> t | |||
val make_generic : (sw:Switch.t -> Uri.t -> Eio.Flow.two_way_ty r) -> t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val make_generic : (sw:Switch.t -> Uri.t -> Eio.Flow.two_way_ty r) -> t | |
val make_generic : (sw:Switch.t -> Uri.t -> _ Eio.Flow.two_way) -> t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talex5 Thanks for your feedback. I updated my patch.
Currently, Cohttp_eio.Client.make_generic takes a function that returns a value of type `_ Eio.Net.stream_socket` as an argument. This type is too strict and can be relaxed to `_ Eio.Flow.two_way`. The difference between these two types is actually important when we use cohttp-eio with ocaml-tls. Consider the following code: ``` let authenticator = Ca_certs.authenticator () |> Result.get_ok let connect_via_tls url socket = let tls_config = Tls.Config.client ~authenticator () in let host = Uri.host url |> Option.map (fun x -> Domain_name.(host_exn (of_string_exn x))) in Tls_eio.client_of_flow ?host tls_config socket let connect net ~sw url = (* NOTE: Do something different than `Cohttp_eio.Client.tcp_address` here and get `addr` *) let socket = Eio.Net.connect ~sw net addr in connect_via_tls url socket let () = Eio_main.run @@ fun env -> Cohttp_eio.Client.make_generic (fun ~sw url -> let flow = connect (Eio.Stdenv.net env) ~sw url in flow (* <<---- TYPE ERROR HERE! Error: This expression has type Tls_eio.t = [ `Flow | `R | `Shutdown | `Tls | `W ] Eio_unix.source but an expression was expected of type [> [> `Generic ] Eio.Net.stream_socket_ty ] Eio_unix.source Type [ `Flow | `R | `Shutdown | `Tls | `W ] is not compatible with type [> ([> `Generic ] as 'a) Eio.Net.stream_socket_ty ] = [> `Close | `Flow | `Platform of 'a | `R | `Shutdown | `Socket | `Stream | `W ] The first variant type does not allow tag(s) `Close, `Platform, `Socket, `Stream *)) ``` This code won't compile if `make_generic` expects `_ Eio.Net.stream_socket`, but it will compile if `_ Eio.Flow.two_way`. This patch solves the above problem by changing the interface of `make_generic` to `(sw:Switch.t -> Uri.t -> _ Eio.Flow.two_way) -> t`. The implementation of `make_generic` is effectively an identity function, so we don't need to change the implementation.
f8919f2
to
539db5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Currently, Cohttp_eio.Client.make_generic takes a function that returns a value of type
_ Eio.Net.stream_socket
as an argument. This type is too strict and can be relaxed toEio.Flow.two_way_ty r
. The difference between these two types is actually important when we use cohttp-eio with ocaml-tls. Consider the following code:This code won't compile if
make_generic
expects_ Eio.Net.stream_socket
, but it will compile ifEio.Flow.two_way_ty r
.This patch solves the above problem by changing the interface of
make_generic
to(sw:Switch.t -> Uri.t -> Eio.Flow.two_way_ty r) -> t
. The implementation ofmake_generic
is effectively an identity function, so we don't need to change the implementation.