search

mirage / ocaml-cohttp

An OCaml library for HTTP clients and servers using Lwt or Async
Other
704 stars 174 forks source link

Cohttp expose closefn function #1036

Open gabrielmoise17 opened 3 months ago

gabrielmoise17 commented 3 months ago

This work has been done together by the following people: @savvadia @vect0r-vicall @picojulien @johnyob @raphael-proust

Motivation

cohttp is not closing client-side connections on request in relay connections, which could lead to resource leak. Exposing the close_fn function is already present in the newer v6 version, so this addition we think is necessary as it allows finer resource management and is likely to be a backport of the feature from v6 to v5.

Solution

cohttp now exposes close_fn defined by Cohttp_lwt.Client.call. This callback should be used when a newly created connection should be closed, thanks to the new Cohttp_lwt.Client.call_with_closefn function.

saroupille commented 2 days ago

@art-w Do you know how we can move forward on this?

art-w commented 1 day ago

I believe the cohttp maintainers are looking to publish 6.0 but have very limited time. Would using the v6 address your issue or do you have a use-case that must stick with the v5 and is leaking without this new function? (Note that I agree with the leak analysis and proposed fix! But it's not entirely clear from the report if the PR is a nice-to-have or a must-have, which would provide motivation for maintainers to spend time on a v5 release instead of a v6 release... but I'm not a cohttp maintainer so I can't speak for them!)

If you need a new release of v5 with this and https://github.com/mirage/ocaml-cohttp/pull/1035 , it would help to fix the CI as it's going to be a blocker for opam. I know the errors were not introduced by your PRs, but I believe it would maximize your chances if you minimize the time spent by maintainers to upstream your work. Good luck!

gabrielmoise17 commented 20 hours ago

I believe the cohttp maintainers are looking to publish 6.0 but have very limited time. Would using the v6 address your issue or do you have a use-case that must stick with the v5 and is leaking without this new function? (Note that I agree with the leak analysis and proposed fix! But it's not entirely clear from the report if the PR is a nice-to-have or a must-have, which would provide motivation for maintainers to spend time on a v5 release instead of a v6 release... but I'm not a cohttp maintainer so I can't speak for them!)

If you need a new release of v5 with this and #1035 , it would help to fix the CI as it's going to be a blocker for opam. I know the errors were not introduced by your PRs, but I believe it would maximize your chances if you minimize the time spent by maintainers to upstream your work. Good luck!

Hello! Thanks for the reply! I tried to fix the CI myself, but it seems like it is more complicated that I thought. I am not sure why this happens, since my changes do not impact the CI (as you mentioned). I am not sure I am the most suitable for fixing the CI, ultimately. What do you think?