Skip to content
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

Merged

Conversation

ushitora-anqou
Copy link
Contributor

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_ty r. 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_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 of make_generic is effectively an identity function, so we don't need to change the implementation.

Copy link
Contributor

@talex5 talex5 left a 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)

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

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.
@ushitora-anqou ushitora-anqou force-pushed the change-eio-client-make-generic-intf branch from f8919f2 to 539db5c Compare April 4, 2024 10:07
Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mseri mseri merged commit 5efbcec into mirage:master Apr 24, 2024
15 of 19 checks passed
@ushitora-anqou ushitora-anqou deleted the change-eio-client-make-generic-intf branch April 25, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants