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

Cohttp expose closefn function #1036

Open
wants to merge 3 commits into
base: v5-backports
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions cohttp-async/src/server.ml
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ let handle_client handle_request sock rd wr =
Body.drain body >>| fun () -> Ivar.fill_if_empty finished ()
| `Response (req, body, (res, res_body)) ->
(* There are scenarios if a client leaves before consuming the full response,
we might have a reference to an async Pipe that doesn't get drained.
we might have a reference to an async Pipe that doesn't get drained.

Not draining or closing a pipe can lead to issues if its holding a resource like
a file handle as those resources will never be closed, leading to a leak.
Not draining or closing a pipe can lead to issues if its holding a resource like
a file handle as those resources will never be closed, leading to a leak.

Async writers have a promise that's fulfilled whenever they are closed,
so we can use it to schedule a close operation on the stream to ensure that we
don't leave a stream open if the underlying channels are closed. *)
Async writers have a promise that's fulfilled whenever they are closed,
so we can use it to schedule a close operation on the stream to ensure that we
don't leave a stream open if the underlying channels are closed. *)
(match res_body with
| `Empty | `String _ | `Strings _ -> ()
| `Pipe stream ->
Expand Down
3 changes: 2 additions & 1 deletion cohttp-async/test/test_async_integration.ml
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ let ts =
("Pipe with empty strings", Pipe.of_list [ ""; ""; "" ], true);
]
in
Deferred.List.iter ~how:`Sequential tests ~f:(fun (msg, pipe, expected) ->
Deferred.List.iter ~how:`Sequential tests
~f:(fun (msg, pipe, expected) ->
is_empty (`Pipe pipe) >>| fun real ->
assert_equal ~msg expected real)
>>= fun () ->
Expand Down
3 changes: 3 additions & 0 deletions cohttp-lwt-jsoo/src/cohttp_lwt_jsoo.ml
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ struct

let callv ?ctx:_ _uri _reqs = Lwt.fail Cohttp_lwt_xhr_callv_not_implemented

let call_with_closefn ?ctx:_ ?headers:_ ?body:_ ?chunked:_ _meth _uri =
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we ask for the advice of the maintainers of the library, as we advocate that this function will never be called from this library, but we want to receive a second opinion.

assert false

(* ??? *)
end

Expand Down
12 changes: 2 additions & 10 deletions cohttp-lwt-unix/bin/dune
Original file line number Diff line number Diff line change
@@ -1,14 +1,6 @@
(executables
(names cohttp_curl_lwt cohttp_proxy_lwt cohttp_server_lwt)
(libraries
cohttp-lwt-unix
cohttp_server
logs
logs.lwt
logs.fmt
logs.cli
cmdliner
conduit-lwt
fmt.tty)
(libraries cohttp-lwt-unix cohttp_server logs logs.lwt logs.fmt logs.cli
cmdliner conduit-lwt fmt.tty)
(package cohttp-lwt-unix)
(public_names cohttp-curl-lwt cohttp-proxy-lwt cohttp-server-lwt))
10 changes: 7 additions & 3 deletions cohttp-lwt/src/client.ml
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ module Make (IO : S.IO) (Net : S.Net with module IO = IO) = struct
| `DELETE -> false
| _ -> true

let call ?(ctx = Net.default_ctx) ?headers ?(body = `Empty) ?chunked meth uri
=
let call_with_closefn ?(ctx = Net.default_ctx) ?headers ?(body = `Empty)
?chunked meth uri =
let headers = match headers with None -> Header.init () | Some h -> h in
Net.connect_uri ~ctx uri >>= fun (_conn, ic, oc) ->
let closefn () = Net.close ic oc in
Expand Down Expand Up @@ -88,7 +88,11 @@ module Make (IO : S.IO) (Net : S.Net with module IO = IO) = struct
|> fun t ->
Lwt.on_cancel t closefn;
Lwt.on_failure t (fun _exn -> closefn ());
t
Lwt.return (t, closefn)

let call ?(ctx = Net.default_ctx) ?headers ?(body = `Empty) ?chunked meth uri
=
call_with_closefn ~ctx ?headers ~body ?chunked meth uri >>= fun (t, _) -> t

(* The HEAD should not have a response body *)
let head ?ctx ?headers uri = call ?ctx ?headers `HEAD uri >|= fst
Expand Down
16 changes: 15 additions & 1 deletion cohttp-lwt/src/s.ml
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,21 @@ module type Client = sig
(using [ocaml-tls]) or SSL (using [ocaml-ssl]), on [*:443] or on the
specified port by the user. If neitehr [ocaml-tls] or [ocaml-ssl] are
installed on the system, [cohttp]/[conduit] tries the usual ([*:80]) or
the specified port by the user in a non-secured way. *)
the specified port by the user in a non-secured way.

The function returns response and body. *)

val call_with_closefn :
?ctx:ctx ->
?headers:Cohttp.Header.t ->
?body:Body.t ->
?chunked:bool ->
Cohttp.Code.meth ->
Uri.t ->
((Cohttp.Response.t * Body.t) Lwt.t * (unit -> unit)) Lwt.t
(** [call_with_closefn ?ctx ?headers ?body ?chunked meth uri] is the same as
[call] but returns response, body and [close_fn] which force releases the
connection. *)

val head :
?ctx:ctx -> ?headers:Cohttp.Header.t -> Uri.t -> Cohttp.Response.t Lwt.t
Expand Down
2 changes: 1 addition & 1 deletion cohttp-mirage/src/dune
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
(preprocess
(pps ppx_sexp_conv))
(libraries conduit-mirage cohttp-lwt mirage-channel mirage-kv mirage-flow
magic-mime astring))
magic-mime astring cstruct))
3 changes: 2 additions & 1 deletion examples/async/dune
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
(executables
(names hello_world receive_post s3_cp)
(libraries mirage-crypto cohttp-async base async_kernel core_unix.command_unix))
(libraries mirage-crypto cohttp-async base async_kernel
core_unix.command_unix cstruct))

(alias
(name runtest)
Expand Down
1 change: 0 additions & 1 deletion examples/async/s3_cp.ml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ open Core
open Async
open Cohttp
open Cohttp_async

module Time = Time_float

let ksrt (k, _) (k', _) = String.compare k k'
Expand Down
Loading