eio_luv: fix some resource leaks

[talex5]

Previously, we asked to close the async notifier after the loop had stopped. Presumably because the loop wasn't running, this close operation never ran, which leaked some FDs. Also, the Loop.close it was supposed to call after closing itself never got called either.

Once that was fixed, Loop.close reported further resource leaks in the Poll and Process modules, which this commit also fixes.

/cc @patricoferris

Fixes #419.

[patricoferris]

The changes look good to me, I ran it on macOS which uncovered a few more leaks (it's probably worth adding some macOS GitHub action CI).

First in the timers, I think we just need to close the timer on success. I was under the impression fiber cancel functions were always called, but maybe I'm just misremembering.

diff --git a/lib_eio_luv/eio_luv.ml b/lib_eio_luv/eio_luv.ml
index fdd192a..41a77cc 100644
--- a/lib_eio_luv/eio_luv.ml
+++ b/lib_eio_luv/eio_luv.ml
@@ -637,6 +637,7 @@ module Low_level = struct
       );
     Luv.Timer.start timer delay (fun () ->
         Suspended.clear_cancel_fn k;
+        Luv.Handle.close timer (fun () -> ());
         enqueue_thread st k ()
       ) |> or_raise

The second was in the cancellation of something spawned in domain manager. The cancelled promise is resolved which I think means the Luv.Async.t is never closed leading to the EBUSY when trying to shut down the loop.

[talex5]

The changes look good to me, I ran it on macOS which uncovered a few more leaks (it's probably worth adding some macOS GitHub action CI).

Thanks - I applied your fix and fixed domain_mgr too. Agree it would be good to add a macOS CI runner.

[patricoferris]

All working for me :))

[polytypic]

BTW, I just tested with godbolt that Fun.id instead of fun () -> () or ignore generates one instruction less. Fun.id is a function that just returns. fun () -> () and ignore are functions that return the unit value (and that takes an additional instruction). I could imagine a type directed optimization that would eliminate this difference in case ignore is called with the unit value and it should also be possible to optimize fun () -> (), but the compiler doesn't seem to do that yet.

I don't suggest to change the code in this PR, but it is something I might want to consider in performance critical cases.

[talex5]

Ah, interesting! I didn't want to use ignore here in case close changed in future to pass a result type or something, but Fun.id would have worked.