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

Make response type abstract #998

Closed
mseri opened this issue Sep 15, 2023 · 0 comments · Fixed by #1024
Closed

Make response type abstract #998

mseri opened this issue Sep 15, 2023 · 0 comments · Fixed by #1024
Labels

Comments

@mseri
Copy link
Collaborator

mseri commented Sep 15, 2023

From #984 (comment):

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant