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

Remove lwt from low level finalise #2064

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/irmin-pack/unix/async.ml
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ module Unix = struct
{ pid; status = `Running }

let status_of_process_outcome = function
| Lwt_unix.WSIGNALED x when x = Sys.sigkill ->
| Unix.WSIGNALED x when x = Sys.sigkill ->
(* x is actually -7; -7 is the Sys.sigkill definition (not the OS' 9 as
might be expected) *)
`Success
(* the child is killing itself when it's done *)
| Lwt_unix.WSIGNALED n -> `Failure (Fmt.str "Signaled %d" n)
| Lwt_unix.WEXITED n -> `Failure (Fmt.str "Exited %d" n)
| Lwt_unix.WSTOPPED n -> `Failure (Fmt.str "Stopped %d" n)
| Unix.WSIGNALED n -> `Failure (Fmt.str "Signaled %d" n)
| Unix.WEXITED n -> `Failure (Fmt.str "Exited %d" n)
| Unix.WSTOPPED n -> `Failure (Fmt.str "Stopped %d" n)

let cancel t =
let () =
Expand Down Expand Up @@ -108,10 +108,10 @@ module Unix = struct
let await t =
match t.status with
| `Running ->
let+ pid, status = Lwt_unix.waitpid [] t.pid in
let pid, status = Unix.waitpid [] t.pid in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit concerned about this change - Lwt_unix.waitpid blocked the current promise, but allowed for other Lwt promises to run. I think with this change we are blocking all promises in this thread?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is a good question. We do not use Lwt for other I/O operations (like in io.ml) so I'm wondering if this change makes anything that much worse. In the high-level gc api (that is used by Tezos), await will never be called -- we only check status. Does that change anything for you?

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should avoid doing this as it is blocking the whole process that defeat the purpose of that API

Copy link
Member

Choose a reason for hiding this comment

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

@samoht this function is not called as a part of the high-level api. It only calls status, which is already using Unix.waitpid [ Unix.WNOHANG ] t.pid.

let s = status_of_process_outcome status in
Exit.remove pid;
t.status <- s;
s
| #outcome as s -> Lwt.return s
| #outcome as s -> s
end
2 changes: 1 addition & 1 deletion src/irmin-pack/unix/async_intf.ml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ module type S = sig
val async : (unit -> unit) -> t
(** Start a task. *)

val await : t -> [> outcome ] Lwt.t
val await : t -> [> outcome ]
(** If running, wait for a task to finish and return its outcome.

If not running, return the oucome of the task. *)
Expand Down
2 changes: 1 addition & 1 deletion src/irmin-pack/unix/ext.ml
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ module Maker (Config : Conf.S) = struct
| Some { gc; _ } ->
if t.during_batch then
Lwt.return_error `Gc_forbidden_during_batch
else Gc.finalise ~wait gc
else Gc.finalise ~wait gc |> Lwt.return
in
match result with
| Ok (`Finalised _ as x) ->
Expand Down
8 changes: 4 additions & 4 deletions src/irmin-pack/unix/gc.ml
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ module Make (Args : Args) = struct

let finalise ~wait t =
match t.stats with
| Some stats -> Lwt.return_ok (`Finalised stats)
| Some stats -> Ok (`Finalised stats)
| None -> (
let go status =
let start = t.elapsed () in
Expand Down Expand Up @@ -614,14 +614,14 @@ module Make (Args : Args) = struct
let () = Lwt.wakeup_later t.resolver err in
err
in
Lwt.return result
result
in
if wait then
let* status = Async.await t.task in
let status = Async.await t.task in
go status
else
match Async.status t.task with
| `Running -> Lwt.return_ok `Running
| `Running -> Ok `Running
| status -> go status)
Comment on lines 619 to 625
Copy link
Contributor

Choose a reason for hiding this comment

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

A solution to the waitpid/lwt problem would be to turn the existing finalise into finalise_nowait:

val finalise : wait:bool -> t -> ([> 'Running | 'Finalised of stats ], Args.Errs.t) result Lwt.t
val finalise_nowait :       t -> ([> 'Running | 'Finalised of stats ], Args.Errs.t) result

and then add a new finalise_wait function that uses lwt:

val finalise : wait:bool -> t -> ([> 'Running | 'Finalised of stats ], Args.Errs.t) result Lwt.t
val finalise_wait :         t -> ([> 'Running | 'Finalised of stats ], Args.Errs.t) result Lwt.t

This way, lwt would be gone from all but one function of that part of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would lead to some code duplication, I'm not sure its worth: its not removing lwt from async and finalise is already the only place in the gc where we return lwt.

I'm closing this for now. If you disagree, please open this again.


let on_finalise t f =
Expand Down
4 changes: 1 addition & 3 deletions src/irmin-pack/unix/gc_intf.ml
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,7 @@ module type S = sig
(** Creates and starts a new GC process. *)

val finalise :
wait:bool ->
t ->
([> `Running | `Finalised of stats ], Args.Errs.t) result Lwt.t
wait:bool -> t -> ([> `Running | `Finalised of stats ], Args.Errs.t) result
(** [finalise ~wait t] returns the state of the GC process.

If [wait = true], the call will block until GC finishes. *)
Expand Down
8 changes: 4 additions & 4 deletions src/irmin-pack/unix/s.ml
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ module type S = sig
Finalising consists of mutating [repo] so that it points to the new file
and to flush the internal caches that could be referencing GCed objects.

If [wait = true] (the default), the call blocks until the GC process
finishes. If [wait = false], finalisation will occur if the process has
ended.
If [wait = true] (the default), the calling process blocks until the GC
process finishes. If [wait = false], finalisation will occur if the
process has ended.

If there are no running GCs, the call is a no-op and it returns [`Idle].

Expand Down Expand Up @@ -119,7 +119,7 @@ module type S = sig
messages should be used only for informational purposes, like logging. *)

val wait : repo -> (stats option, msg) result Lwt.t
(** [wait repo] blocks until GC is finished or is idle.
(** [wait repo] blocks the process until GC is finished or is idle.

If a GC finalises, its stats are returned.

Expand Down