Skip to content

Conversation

@hhugo
Copy link

@hhugo hhugo commented Apr 15, 2024

This goes with ocsigen/js_of_ocaml#1601 and should allow fixing memory leak of ocaml channels.

We first need a new release of jsoo that include the new api. We can then merge this PR (and release ppx_expect) and finally merge ocsigen/js_of_ocaml#1601

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Apr 16, 2024
@hhugo hhugo force-pushed the jsoo-leak-chan branch from e091be1 to 780614a Compare June 1, 2024 21:29
Hugo Heuzard added 2 commits June 1, 2024 23:29
Signed-off-by: Hugo Heuzard <[email protected]>
Signed-off-by: Hugo Heuzard <[email protected]>
@hhugo hhugo force-pushed the jsoo-leak-chan branch from 780614a to 16b1bb0 Compare June 1, 2024 21:29
@hhugo hhugo marked this pull request as ready for review June 1, 2024 21:29
@hhugo hhugo changed the title Use (soon added) dedicated runtime functions instead of relying on caml_ml_channels explicitly Use dedicated runtime functions instead of relying on caml_ml_channels explicitly Jun 1, 2024
@hhugo
Copy link
Author

hhugo commented Jun 15, 2024

@tkluck-janestreet, @TyOverby, this PR blocks merging ocsigen/js_of_ocaml#1601 (well, we would also need a new ppx_expect release)

@tkluck-janestreet
Copy link

hi @hhugo , just to let you know I forwarded this to the right people and they're looking at it. I'm sure they'll post an update here soon.

@dkalinichenko-js
Copy link
Contributor

dkalinichenko-js commented Jun 24, 2024

Hi, I'll submit a new release of ppx_expect once we review this internally.

This might take some time, as we also need to update our internal version of js_of_ocaml.

@hhugo
Copy link
Author

hhugo commented Jun 24, 2024

Hi, I'll submit a new release of ppx_expect once we review this internally.

This might take some time, as we also need to update our internal version of js_of_ocaml.

Thank you

@rickyvetter
Copy link

There are some timing issues on this as we are using wasm_of_ocaml with some patches and ocsigen/js_of_ocaml#1601 doesn't apply cleanly. I know @vouillon and Tarides folks are looking at getting wasm_of_ocaml up-to-date with jsoo, which would make this change quite straightforward. If that is happening in the next couple weeks then we would rather wait and then apply all of these changes after updating.

@hhugo
Copy link
Author

hhugo commented Sep 30, 2024

There are some timing issues on this as we are using wasm_of_ocaml with some patches and ocsigen/js_of_ocaml#1601 doesn't apply cleanly. I know @vouillon and Tarides folks are looking at getting wasm_of_ocaml up-to-date with jsoo, which would make this change quite straightforward. If that is happening in the next couple weeks then we would rather wait and then apply all of these changes after updating.

It's apparently not happening in the next couple of weeks

@rickyvetter
Copy link

These changes have been released internally and will be part of the next public release. Apologies for the delay here.

@dkalinichenko-js dkalinichenko-js changed the base branch from master to v0.17 October 8, 2024 19:32
@dkalinichenko-js dkalinichenko-js merged commit 9efcf64 into janestreet:v0.17 Oct 8, 2024
@dkalinichenko-js
Copy link
Contributor

Thank you! ocaml/opam-repository#26695 was submitted with this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants