Make server response type abstract and allow streaming in cohttp-eio #1024
Conversation
|
Looks good to me! I would even suggest making We might even make val respond :
?headers:Http.Header.t ->
?flush:bool ->
?status:Http.Status.t ->
_ Eio.Flow.source ->
response IO.tSo that the simplest form becomes |
|
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? |
talex5
force-pushed
the
abstract-response
branch
from
ebdbff2
to
34f881c
Compare
|
|
||
| type t = { | ||
| conn_closed : conn -> unit; | ||
| handler : conn -> Http.Request.t -> body -> response_action IO.t; | ||
| handler : conn -> Http.Request.t -> body -> IO.ic -> IO.oc -> unit; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Yes, although not when using the normal response functions. So it's certainly possible to write code that works with both.
|
AFAIR this looks fine to me and we can merge it. |
|
@talex5 can you have a look at the conflict? I think after that we could merge |
This will allow cohttp-eio to use a different type in future, which should make streaming easier.
Makes next commit easier to read: - Use `respond` helpers in example. Avoids it having to change. - Remove useless `IO.t` and `>>=`. - Move some functions later in the file, and they'll need to use `write`.
This changes the response type to `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.
|
I rebased this. The conflict was due to #1025. If I understand the logic there correctly, this is what cohttp-lwt now does:
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 For cohttp-eio, I modified the (abstract) |
|
Thanks. I think it may be worth revising the api for cohttp-lwt but it could be done in a future 7.0 release |
CHANGES: - bump minimum dune version to 3.8 (@avsm) - cohttp-eio: Use system authenticator in example. - http, cohttp: remove the scheme field from requests. This means that [Request.uri] no longer returns the same URI as was to create the request with [Request.make] (@rgrinberg 1086) - cohttp-eio: Remove unused `Client_intf` module (talex5 mirage/ocaml-cohttp#1081) - cohttp-eio: Make server response type abstract and allow streaming in cohttp-eio (talex5 mirage/ocaml-cohttp#1024) - cohttp-{lwt,eio}: server: add connection header to response if not present (ushitora-anqou mirage/ocaml-cohttp#1025) - cohttp-curl: Curl no longer prepends the first HTTP request header to the output. (jonahbeckford mirage/ocaml-cohttp#1030, mirage/ocaml-cohttp#987) - cohttp-eio: client: use permissive argument type for make_generic - cohttp-eio: Improve error handling in example server (talex5 mirage/ocaml-cohttp#1023) - cohttp-eio: Don't blow up `Server.callback` on client disconnections. (mefyl mirage/ocaml-cohttp#1015) - http: Fix assertion in `Source.to_string_trim` when `pos <> 0` (mefyl mirage/ocaml-cohttp#1017) - cohttp: `Cohttp.Request.make_for_client` no longer allows setting both `~chunked:true` and `~body_length`. - cohttp-lwt-unix: Don't blow up when certificates are not available and no-network requests are made. (akuhlens mirage/ocaml-cohttp#1027) + Makes `cohttp-lwt.S.default_ctx` lazy.
This fixes #998.
The first commit adds an abstract
responsetype and then usestype response = Http.Response.t * Body.tin 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
responsetype in cohttp-eio totype response = writer -> unit. This allowshandlers 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_stringetc will continue to work as before.This also exposes a more polymorphic version of the
respondfunction that accepts sub-types ofFlow.source, so that callers don't need to cast the body./cc @mefyl