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 4 commits into
base: v5-backports
Choose a base branch
from

Conversation

gabrielmoise17
Copy link

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.

@@ -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.

@saroupille
Copy link

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

@art-w
Copy link
Contributor

art-w commented Oct 4, 2024

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!

@gabrielmoise17 gabrielmoise17 force-pushed the cohttp_expose_closefn branch 3 times, most recently from 4cc3ac3 to 996424f Compare October 4, 2024 11:00
@gabrielmoise17
Copy link
Author

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?

@art-w
Copy link
Contributor

art-w commented Oct 10, 2024

I think that you were very close to fixing the CI and that you are the most motivated to see it through! Run dune fmt, follow the opam lint and lowerbound instructions, do a local opam update/upgrade to reproduce the compilation errors on your computer and take inspiration from #1084 (and other "chore" PRs) to fix the upstream breaking changes (note that I didn't break the CI either, but contributing to open-source may require taking part in the maintenance effort).

I would also open a new PR specifically to fix the v5 CI (an easy merge once green), and rebase your other PRs on top (and then politely ping the maintainers to see if they are interested).

@gabrielmoise17 gabrielmoise17 force-pushed the cohttp_expose_closefn branch 2 times, most recently from 8460a03 to 23da4ff Compare October 11, 2024 10:57
@gabrielmoise17 gabrielmoise17 force-pushed the cohttp_expose_closefn branch 2 times, most recently from 72ec356 to 1ceb38f Compare October 11, 2024 11:43
@gabrielmoise17
Copy link
Author

I think that you were very close to fixing the CI and that you are the most motivated to see it through! Run dune fmt, follow the opam lint and lowerbound instructions, do a local opam update/upgrade to reproduce the compilation errors on your computer and take inspiration from #1084 (and other "chore" PRs) to fix the upstream breaking changes (note that I didn't break the CI either, but contributing to open-source may require taking part in the maintenance effort).

I would also open a new PR specifically to fix the v5 CI (an easy merge once green), and rebase your other PRs on top (and then politely ping the maintainers to see if they are interested).

basically, I cannot reproduce the behaviour from the CI locally. Locally, running make does not get me any error. However, in the current CI, there is an issue (among others), which suggests me using Ivar.fill_exn instead of Ivar.fill, but this does not work for me locally https://ocaml.ci.dev/github/mirage/ocaml-cohttp/commit/1ceb38f71decc4fa71d8e988d70f6752f21a571b/variant/macos-homebrew-5.2_arm64_opam-2.2. I restarted the build from scratch and now my opam switch is broken.

@art-w
Copy link
Contributor

art-w commented Oct 11, 2024

If you can't reproduce locally then you don't have the latest dependencies installed. I don't know how a fresh build resulted in a broken opam switch or how to repair it, but I hope you'll find a way to get back to a working environment! In any case, every ocaml-ci log starts with instructions to reproduce locally with only docker (and those are the exact instructions used by the CI, so it will install the right set of packages versions to break just like the CI).

I vaguely remember the Ivar.fill depreciation, so I believe every CI issue you are running into has already been addressed on master which means you can search there and git-blame to find the recommended fix: e.g. https://github.com/mirage/ocaml-cohttp/blame/f6ed2e2ca758c4e42367b2f1c23abd9fa2b062b7/cohttp-curl-async/src/cohttp_curl_async.ml#L44

@gabrielmoise17
Copy link
Author

If you can't reproduce locally then you don't have the latest dependencies installed. I don't know how a fresh build resulted in a broken opam switch or how to repair it, but I hope you'll find a way to get back to a working environment! In any case, every ocaml-ci log starts with instructions to reproduce locally with only docker (and those are the exact instructions used by the CI, so it will install the right set of packages versions to break just like the CI).

I vaguely remember the Ivar.fill depreciation, so I believe every CI issue you are running into has already been addressed on master which means you can search there and git-blame to find the recommended fix: e.g. https://github.com/mirage/ocaml-cohttp/blame/f6ed2e2ca758c4e42367b2f1c23abd9fa2b062b7/cohttp-curl-async/src/cohttp_curl_async.ml#L44

I tried reproducing the behaviour from here, but the docker build . does not work, do you have some hint on how to progress?

$ docker build .
[+] Building 0.9s (2/2) FINISHED                                                     docker:desktop-linux
 => [internal] load build definition from Dockerfile                                                 0.0s
 => => transferring dockerfile: 4.96kB                                                               0.0s
 => ERROR [internal] load metadata for docker.io/library/macos-homebrew-ocaml-5.2:latest             0.8s
------
 > [internal] load metadata for docker.io/library/macos-homebrew-ocaml-5.2:latest:
------
Dockerfile:1
--------------------
   1 | >>> FROM macos-homebrew-ocaml-5.2
   2 |     # macos-homebrew-5.2_arm64_opam-2.2
   3 |     USER 1000:1000
--------------------
ERROR: failed to solve: macos-homebrew-ocaml-5.2: failed to resolve source metadata for docker.io/library/macos-homebrew-ocaml-5.2:latest: pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed

@art-w
Copy link
Contributor

art-w commented Oct 15, 2024

I guess you need a macos to run this one. The linux images should work though.

@gabrielmoise17
Copy link
Author

I guess you need a macos to run this one. The linux images should work though.

I used that because I have macOS specifically, that is why I am confused.

@art-w
Copy link
Contributor

art-w commented Oct 16, 2024

Right, my bad, the CI has to use a docker alternative to run natively on macos (but I don't think it's worth the effort to set it up on your system for this PR, as the CI issues are not macos related). Your previous terminal paste mentions docker:desktop-linux, can you at least run the linux images which have the same compilation errors as the macos ones?

On a side note, you have 5 co-authors on your PR. Can they help you out?

@gabrielmoise17
Copy link
Author

Right, my bad, the CI has to use a docker alternative to run natively on macos (but I don't think it's worth the effort to set it up on your system for this PR, as the CI issues are not macos related). Your previous terminal paste mentions docker:desktop-linux, can you at least run the linux images which have the same compilation errors as the macos ones?

On a side note, you have 5 co-authors on your PR. Can they help you out?

Hey! Sorry for the late reply, I was caught with other projects..
Regarding your "other contributors" question, they unfortunately are in other teams with other projects now, so it is hard to contribute further.

I opened a new PR to fix the CI (#1094), but I have something to ask. Is it required that all the tests work from the "Main workflow"? I tried with ocaml version 5 and with 4.14, and they have different errors, depending on what version I choose. It seems like I cannot make both of them happy, as for instance in 5 I use Ivar.fill_exn and in 4.14 I use Ivar.fill. What is the point here?

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

Successfully merging this pull request may close these issues.

3 participants