Make server response type abstract and allow streaming in cohttp-eio

[talex5]

This fixes #998.

The first commit adds an abstract response type and then uses type response = Http.Response.t * Body.t in cohttp-lwt and cohttp-eio to keep the final types the same (cohttp-async already defined this type anyway and so didn't need any updates).

The second is just a few trivial changes to cohttp-eio to make the third commit diff smaller.

The third commit changes the response type in cohttp-eio to type response = writer -> unit. This allows
handlers to write the response inside the function, rather than returning a request to cohttp to write it later. That's useful because it allows e.g. streaming from an open file and then closing it afterwards.

Partial application means that code using respond_string etc will continue to work as before.

This also exposes a more polymorphic version of the respond function that accepts sub-types of Flow.source, so that callers don't need to cast the body.

/cc @mefyl

[mefyl]

Looks good to me!

I would even suggest making Cohttp_eio.Server.respond's status argument default to OK, just like Cohttp.Response.make, so that one need only replace (Cohttp.Response.make (), body) with Cohttp_eio.Server.respond ~body () to adapt to this change.

We might even make body positional since it's required anyway, making the signature:

val respond :
  ?headers:Http.Header.t ->
  ?flush:bool ->
  ?status:Http.Status.t ->
  _ Eio.Flow.source ->
  response IO.t

So that the simplest form becomes Cohttp_eio.Server.respond body.

[talex5]

That probably is a better API, but it's trying to implement the signatures in the shared cohttp package, so they would have to be extra functions with different names if we did that. Maybe something for a separate PR?

[mseri]

My only concern is that the change to old-style response was done to align the APIs, with this they APIs are diverging again (at least in the expectations and signatures)

[talex5]

Yes, although not when using the normal response functions. So it's certainly possible to write code that works with both.

[mseri]

What is the status of this? I'd like to make a RC release of cohttp in the next couple of weeks. @mefyl and @talex5 do you think we should merge it as is or it needs further discussion?

[mefyl]

AFAIR this looks fine to me and we can merge it.

[mseri]

@talex5 can you have a look at the conflict? I think after that we could merge

[talex5]

I rebased this. The conflict was due to #1025. If I understand the logic there correctly, this is what cohttp-lwt now does:

1. The user's handler uses e.g. respond* to create an HTTP 1.1 response, possibly missing the connection header, and returns it.
2. cohttp-lwt then notices that this isn't appropriate for the request and creates a fixed-up response with the correct header for the request (though still always HTTP 1.1, by the look of it).

The seems a bit odd. It would perhaps make more sense to create the response correctly in the first place, but that would require passing the request to respond, etc, which would be an API change.

For cohttp-eio, I modified the (abstract) writer type to include the request, so respond can build a correct response by itself. The test is simplified a bit because (as with Lwt's responders) the response is always HTTP 1.1, so Http.Response.is_keep_alive is always true when the header is missing.

[mseri]

Thanks. I think it may be worth revising the api for cohttp-lwt but it could be done in a future 7.0 release