Re: Review Request 64907: Added abandonment and process exit support to `loop()`.

[Benjamin Mahler]

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


Ship it!




Ship It!

- 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 64907: Added abandonment and process exit support to `loop()`.

[Benjamin Mahler]

---
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)


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



3rdparty/libprocess/include/process/loop.hpp
Lines 360-375 (original), 370-390 (patched)


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)


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)


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)


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
> 
>

Review Request 64907: Added abandonment and process exit support to `loop()`.

[Benjamin Hindman]

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

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/1/


Testing
---

make check


Thanks,

Benjamin Hindman