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.

Sign up for GitHub

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

Jump to bottom

eio_luv: fix some resource leaks #421

Merged
merged 1 commit into from
Jan 30, 2023
Merged

eio_luv: fix some resource leaks #421

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions lib_eio_luv/eio_luv.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ end = struct
let m = if Lwt_dllist.is_empty events.read then m else `READABLE :: m in
if m = [] then (
Luv.Poll.stop events.handle |> or_raise;
t.fd_map <- Fd_map.remove events.fd t.fd_map
t.fd_map <- Fd_map.remove events.fd t.fd_map;
Luv.Handle.close events.handle (fun () -> ())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) else (
Luv.Poll.start events.handle m (poll_callback t events)
)
Expand Down Expand Up @@ -605,8 +606,9 @@ module Low_level = struct
let spawn ?cwd ?env ?uid ?gid ?(redirect=[]) ~sw cmd args =
let status, set_status = Promise.create () in
let hook = ref None in
let on_exit _ ~exit_status ~term_signal =
let on_exit proc ~exit_status ~term_signal =
Option.iter Switch.remove_hook !hook;
Luv.Handle.close proc (fun () -> ());
Promise.resolve set_status (term_signal, exit_status)
in
let proc = Luv.Process.spawn ?environment:env ?uid ?gid ~loop:(get_loop ()) ?working_directory:cwd ~redirect ~on_exit cmd args |> or_raise in
Expand All @@ -632,6 +634,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

Expand Down Expand Up @@ -969,7 +972,8 @@ let domain_mgr ~run_event_loop =
let run_raw (type a) ~pre_spawn fn =
let domain_k : (unit Domain.t * a Suspended.t) option ref = ref None in
let result = ref None in
let async = Luv.Async.init ~loop:(get_loop ()) (fun async ->
enter @@ fun st k ->
let async = Luv.Async.init ~loop:st.loop (fun async ->
(* This is called in the parent domain after returning to the mainloop,
so [domain_k] must be set by then. *)
let domain, k = Option.get !domain_k in
Expand All @@ -979,7 +983,6 @@ let domain_mgr ~run_event_loop =
Suspended.continue_result k (Option.get !result)
) |> or_raise
in
enter @@ fun _st k ->
pre_spawn k;
let d = Domain.spawn (fun () ->
result := Some (match fn () with
Expand Down Expand Up @@ -1292,11 +1295,12 @@ let rec run2 : type a. (_ -> a) -> a = fun main ->
| v -> main_status := `Done v
| exception ex -> main_status := `Ex (ex, Printexc.get_raw_backtrace ())
end;
Luv.Handle.close async @@ fun () ->
Luv.Loop.stop loop
);
ignore (Luv.Loop.run ~loop () : bool);
Luv.Loop.close loop |> or_raise;
Lf_queue.close st.run_q;
Luv.Handle.close async (fun () -> Luv.Loop.close loop |> or_raise);
match !main_status with
| `Done v -> v
| `Ex (ex, bt) -> Printexc.raise_with_backtrace ex bt
Expand Down