cohttp-eio(client): Ensure "Host" header is the first header in requests

[bikallem]

Fixes #934

/cc @mseri

[avsm]

Given that a Host header is essential in HTTP/1.1+, and that we have a dedicated Http.Header module, it makes more sense to specialise the internals of Http.Header to put "Host" first rather than expose a function move_to_first. There is no other header value where move_to_first could be validly used.

[bikallem]

Given that a Host header is essential in HTTP/1.1+, and that we have a dedicated Http.Header module, it makes more sense to specialise the internals of Http.Header to put "Host" first rather than expose a function move_to_first.

Since Http.Header is used in both server and client scenarios, it would be preferable if we could avoid doing it in the server use-cases.

There is no other header value where move_to_first could be validly used.

Indeed, move_to_first is there primarily for other cohttp packages to reuse in implementing this similar functionality. Would it be more amenable if it is moved to Private module perhaps? along with first function?

[bikallem]

I moved first and move_to_first to Header.Private since is primarily package implementation functionality.

[bikallem]

@mseri I believe I have addressed all of your comments.

[avsm]

I'm still not a huge fan of this approach, which doesn't seem to take advantage of what we do know about the HTTP request structure. How about modifying Cohttp.Header to be a GADT (server t and client t) which enforces that the relevant must be defined when constructing one, and can safely cast from client->server without reallocating the structure? This way it should be impossible to construct a Header.t without the right fields in place, and still be efficient.

[rgrinberg]

How about just moving this concern to the serialization of the header field for clients? In other words, a separate Header.serialize_client function that ensures the Host comes first. As far as I can tell, the user shouldn't be able to observe in which position the Host appears with our abstract header type, so there's no need to do any shuffling until we actually serialize it.

EDIT: This PR essentially does this already.

[rgrinberg]

which enforces that the relevant must be defined when constructing one, and can safely cast from client->server without reallocating the structure?

Is that ever possible? A valid client header always requires a Host while a valid server header cannot contain one.

[rgrinberg]

The GADT seems acceptable as well, but I'd like to hear other people's input if we're changing the fundamental Http.Header.t which is meant for interop. On the other hand, changing Cohttp.Header.t should be fine.

[anuragsoni]

FWIW I'm not sure I'm a huge fan of adding an api that will most likely only ever be used for Host header in one very specific scenario. I like @rgrinberg 's proposal for adding a client aware serializer that writes out the Host header in the correct location. Both server and client modules will need to perform certain actions to comply with the RFCs, so I don't mind having serializers that aren't shared across both layers.

I'm not too familiar with designing easy-to-use GADT based apis so I'm interested in learning about what you have in mind.

[avsm]

There are a few more constraints beyond just the Host header, aren't there? E.g. you must have a content-length or be chunked encoding. Or you can have an Upgrade field, but only with HTTP/1.1. Full list here https://en.wikipedia.org/wiki/List_of_HTTP_header_fields

It wouldn't be mad to encode Http.Header.t as a standard ADT

type t =
 | Host of string
 | Proxy_authorization of [`Basic | `Unknown of string ] * string
 | ...

The only reason I didn't do this in the original Cohttp was due to the serialisation/deserialisation cost when you really want to do this lazily (as most apps dont use all the headers). But if there was a way to make such a representation efficient in modern OCaml, it would be nice to have well-typed headers rather than lots of string munging...

[rgrinberg]

An ADT or whatever else we might use internally shouldn't matter much since we're likely to keep the types abstract. We could add additional types that maintain more invariants (but keeping the same internal representation):

module Header : sig
  type t

  module Client : sig
     type t
     val make : host:string -> ... -> t
     val host : t -> string
     ...
  end

  module Server : sig
    type t
    ...
  end
end

In this example I've included them in Http, but we're likely to incubate them in Cohttp first. The downside of course is that there's going to be no polymorphism, therefore the only way to write generic code is to convert from/to Header.t but it doesn't seem like a huge deal to me.

[bikallem]

How about modifying Cohttp.Header to be a GADT (server t and client t)

@avsm Here is something that I am playing with for Http.Header.t using GADT. The implementation seems quite neat to my surprise.
Header2 - https://github.com/bikallem/ocaml-cohttp/blob/host-first2/http/src/http.mli#L438-L480
Header2 - https://github.com/bikallem/ocaml-cohttp/blob/host-first2/http/src/http.ml#L358-L466

[bikallem]

@avsm @mseri I am experimenting with the typed http header API in separate PR (#942). This PR addresses a specific issue and aims for an incremental improvement to the current API. As such I would like to scope this PR to the issue it is trying to address.

@mseri If there are no other objections to the issue this PR is trying to solve, could you please approve.

[mseri]

There are conflicts that need to be resolved

[bikallem]

@mseri rebased on master and fixed the conflicts.

[mseri]

There are new failures appearing:

#=== ERROR while compiling eio.0.5 ============================================#
# context     2.1.3 | linux/x86_64 | ocaml-variants.5.0.0+trunk | git+https://github.com/ocaml/opam-repository.git
# path        ~/work/ocaml-cohttp/ocaml-cohttp/_opam/.opam-switch/build/eio.0.5
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p eio -j 2 --promote-install-files=false @install
# exit-code   1
# env-file    ~/.opam/log/eio-5404-739508.env
# output-file ~/.opam/log/eio-5404-739508.out
### output ###
# (cd _build/default && /home/runner/work/ocaml-cohttp/ocaml-cohttp/_opam/bin/ocamlc.opt -w -40 -g -bin-annot -I lib_eio/core/.eio__core.objs/byte -I /home/runner/work/ocaml-cohttp/ocaml-cohttp/_opam/lib/cstruct -I /home/runner/work/ocaml-cohttp/ocaml-cohttp/_opam/lib/fmt -I /home/runner/work/ocaml-cohttp/ocaml-cohttp/_opam/lib/hmap -I /home/runner/work/ocaml-cohttp/ocaml-cohttp/_opam/lib/lwt-dllist -no-alias-deps -open Eio__core__ -o lib_eio/core/.eio__core.objs/byte/eio__core__Debug.cmo -c -impl lib_eio/core/debug.ml)
# File "lib_eio/core/debug.ml", line 32, characters 16-25:
# 32 |     | exception Unhandled -> default_traceln
#                      ^^^^^^^^^
# Error: This variant pattern is expected to have type exn
#        There is no constructor Unhandled within type exn

Is it expected?

[talex5]

There are new failures appearing [ on ocaml-variants.5.0.0+trunk ]

Yes, the OCaml API changed. Eio will be updated once OCaml 5 beta1 is out.

[bikallem]

@mseri I have updated the cohttp-eio ci to use ocaml 5.0 beta1 and eio >= 0.6. Waiting for ocaml/opam-repository#22247 to merge. After which the ci should be green, 🤞.

[bikallem]

@mseri cohttp-eio is now building ok again.

[mseri]

Thanks