mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 64907: Added abandonment and process exit support to `loop()`.
Date Sat, 13 Jan 2018 00:24:28 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64907/#review195362
-----------------------------------------------------------



Thanks for fixing this:

(1) There seems to be an issue with the Waiter not terminating in the case that the caller
passes their own Process for the loop's execution context?

(2) Longer term, the way abandonement needed to be implemented looks unfortunate. Ideally
we could have something like "onAny" that includes abandonement so that the new code here
would have been alongside the other isReady/isFailed/isDiscarded handling rather than having
to be far away from it.


3rdparty/libprocess/include/process/loop.hpp
Lines 363 (patched)
<https://reviews.apache.org/r/64907/#comment274535>

    Need to terminate the waiter here? Ditto other reset cases.



3rdparty/libprocess/include/process/loop.hpp
Lines 360-375 (original), 370-390 (patched)
<https://reviews.apache.org/r/64907/#comment274531>

    The existing and newly added `if (self) {` blocks seem to contain a lot of code and add
nesting. Maybe this would all be a bit flatter and easier to read if we flipped it?
    
    ```
    if (!self) { return; }
    
    ...
    ```



3rdparty/libprocess/include/process/loop.hpp
Lines 378-381 (original), 393-415 (patched)
<https://reviews.apache.org/r/64907/#comment274534>

    Well, does a ternary work within the onAny?
    Or can you pull the onAbandoned cases into a single one after the if condition? Ditto
below.



3rdparty/libprocess/include/process/loop.hpp
Lines 466-471 (patched)
<https://reviews.apache.org/r/64907/#comment274533>

    This is where an updated "onAny" that has abandonment would simplify things, no need for
all these extra .onAbandoned cases here and instead they would go alongside the isReady/isFailed/isDiscarded
cases.



3rdparty/libprocess/include/process/loop.hpp
Lines 496 (patched)
<https://reviews.apache.org/r/64907/#comment274530>

    Maybe a little comment here saying that the waiter will ensure the loop is destroyed if
the execution process is terminated?
    
    Also, looking at the code, won't we leak a Waiter if the caller provided their own process
for the execution context of the loop? Seems like we need to terminate the waiter whenever
we Break from the loop? (i.e. all the `self->reset()` cases above).


- Benjamin Mahler


On Jan. 3, 2018, 7:20 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64907/
> -----------------------------------------------------------
> 
> (Updated Jan. 3, 2018, 7:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current semantics for `loop()` mean that we'll leak resources
> under two different circumstances:
> 
>   (1) If either the "iteration" or "body" functions return future's
>       that are abandoned.
> 
>   (2) If the loop is using a process that exits.
> 
> This patch adds support to make sure that abandoned futures are
> properly propagated in the event that either of these situations
> occurs.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/loop.hpp 69d90f3e1b16189e0b1a6f1981e8509c18b38465

>   3rdparty/libprocess/src/tests/loop_tests.cpp 8d1837a0baedc12591f92c8f0f8ea83d0aa44ab0

> 
> 
> Diff: https://reviews.apache.org/r/64907/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message