Remove lwt from low level finalise #2064
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I agree we should avoid doing this as it is blocking the whole process that defeat the purpose of that API
There was a problem hiding this comment.
@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.
Codecov Report
@@ Coverage Diff @@
## main #2064 +/- ##
==========================================
- Coverage 64.34% 64.33% -0.02%
==========================================
Files 131 133 +2
Lines 15584 15571 -13
==========================================
- Hits 10028 10017 -11
+ Misses 5556 5554 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I now remember how we planned on shipping the GC without Lwt. The idea was to check for GC-worker termination using a non-blocking call to Removing |
|
@Ngoguey42 we already do not use Lwt for checking status of the gc worker process, so I think this is already done. See #2064 (comment) |
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
icristescu
force-pushed
the
lwt_finalise
branch
from
2835be7
to
ae959a3
Compare
Part of #2039 is to remove lwt from low level start (already done) and finalise (this PR).
Rebased on #2063.