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-lwt: Don't leak asynchronous Retry exceptions. #992

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

mefyl
Copy link
Contributor

@mefyl mefyl commented Jul 19, 2023

Cohttp-lwt currently let Retry exceptions escape from Lwt.async, which wrongfully triggers Lwt.async_exception_hook.

@mseri
Copy link
Collaborator

mseri commented Jul 19, 2023

Ah fantastic, thanks!

@mseri
Copy link
Collaborator

mseri commented Jul 19, 2023

This should fix #963

@mefyl
Copy link
Contributor Author

mefyl commented Jul 19, 2023

Actually wait a second, it looks like there are other occurences, patch incoming.

@mefyl mefyl changed the title cohttp-lwt: Don't leak asynchronous Retry exceptions. WIP: cohttp-lwt: Don't leak asynchronous Retry exceptions. Jul 19, 2023
@mefyl mefyl changed the title WIP: cohttp-lwt: Don't leak asynchronous Retry exceptions. cohttp-lwt: Don't leak asynchronous Retry exceptions. Jul 19, 2023
@mefyl
Copy link
Contributor Author

mefyl commented Jul 19, 2023

There is another Lwt.async is shutdown, but it looks like this function is never ever called.
The other occurrences are in the caching client, which I have not yet used, so I supposed merging this is already a win.

@mseri
Copy link
Collaborator

mseri commented Jul 19, 2023

I think the leaked Retry was just the one here. If I am wrong, the issue can be reopened

@mseri mseri merged commit b0104b4 into mirage:master Jul 19, 2023
17 of 19 checks passed
mseri added a commit to mseri/opam-repository that referenced this pull request Aug 8, 2023
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
mseri added a commit to mseri/opam-repository that referenced this pull request Aug 8, 2023
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
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.

None yet

2 participants