ocaml-cohttp icon indicating copy to clipboard operation
ocaml-cohttp copied to clipboard

Make response type abstract

Open mseri opened this issue 1 year ago • 0 comments

From https://github.com/mirage/ocaml-cohttp/pull/984#issuecomment-1684943089:

Pass a switch to the user provided callback that can be used to start fibers and allocate resources.

It would certainly be useful to scope resources to a particular request. At the moment, any kind of streaming is painful. It's a bit painful with Lwt too (e.g. https://github.com/ocurrent/ocurrent/blob/3c297620ceaa4f7ce676dd1f3121fe010e0cabcc/lib_web/job.ml#L91-L111). The problem is that cohttp requires the user's function to finish before it starts writing the result. So you can't just perform a sequence of writes; you have to provide a stream to do them later.

Giving the user a switch so that they can start a fiber in it is one solution, but it might be simpler to have the callback do the writing itself. If cohttp made the response type abstract then we could have cohttp-eio redefine it so that the callback takes the response stream as an additional argument. respond_string, etc would still work as before, due to partial application.

e.g.

diff --git a/vendor/cohttp/cohttp/src/server.ml b/vendor/cohttp/cohttp/src/server.ml
index cbe08006..5ecc401a 100644
--- a/vendor/cohttp/cohttp/src/server.ml
+++ b/vendor/cohttp/cohttp/src/server.ml
@@ -3,10 +3,11 @@ module type S = sig
 
   type body
   type conn = IO.conn * Connection.t [@@warning "-3"]
+  type response
 
   type response_action =
     [ `Expert of Http.Response.t * (IO.ic -> IO.oc -> unit IO.t)
-    | `Response of Http.Response.t * body ]
+    | `Response of response ]
   (** A request handler can respond in two ways:
 
       - Using [`Response], with a {!Response.t} and a {!body}.
@@ -38,7 +39,7 @@ module type S = sig
 
   val make :
     ?conn_closed:(conn -> unit) ->
-    callback:(conn -> Http.Request.t -> body -> (Http.Response.t * body) IO.t) ->
+    callback:(conn -> Http.Request.t -> body -> response IO.t) ->
     unit ->
     t
 
@@ -48,7 +49,7 @@ module type S = sig
     status:Http.Status.t ->
     body:body ->
     unit ->
-    (Http.Response.t * body) IO.t
+    response IO.t
   (** [respond ?headers ?flush ~status ~body] will respond to an HTTP request
       with the given [status] code and response [body]. If [flush] is true, then
       every response chunk will be flushed to the network rather than being
@@ -64,7 +65,7 @@ module type S = sig
     status:Http.Status.t ->
     body:string ->
     unit ->
-    (Http.Response.t * body) IO.t
+    response IO.t
 
   val callback : t -> IO.conn -> IO.ic -> IO.oc -> unit IO.t
 end

cohttp-lwt can define type response = Http.Response.t * Body.tand avoid any other changes, whilecohttp-eiocould usetype response = Eio.Buf_write.t -> unit` internally.


Perhaps this could be a chance to revise how we do the writing also in the other backends.

mseri avatar Sep 15 '23 14:09 mseri