You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2016/12/12 20:31:54 UTC
Re: Review Request 53299: Fixed memory leak in implementation of
Future::after().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53299/#review158898
-----------------------------------------------------------
3rdparty/libprocess/include/process/future.hpp (line 1307)
<https://reviews.apache.org/r/53299/#comment229718>
Same rename suggestion as below.
3rdparty/libprocess/include/process/future.hpp (line 1321)
<https://reviews.apache.org/r/53299/#comment229719>
Can you explain why we're adding this behavior with a comment? It's not immediately clear to me from the existing code / comment what the implication is of making this association if the weak future is `none`.
3rdparty/libprocess/include/process/future.hpp (line 1417)
<https://reviews.apache.org/r/53299/#comment229717>
Consider renaming `reference` to `weak_future`.
The reference could no longer be valid, which is why I think the name might be deceptive (even though the type is clear).
3rdparty/libprocess/src/tests/future_tests.cpp (line 266)
<https://reviews.apache.org/r/53299/#comment229714>
What does the word 'policy' here denote?
Could you pick something more consistent with the other tests?
3rdparty/libprocess/src/tests/future_tests.cpp (line 268)
<https://reviews.apache.org/r/53299/#comment229715>
Can you provide context for the scoped block?
e.g.
```
/* Future lifetime. */ {
}
```
3rdparty/libprocess/src/tests/future_tests.cpp (lines 270 - 273)
<https://reviews.apache.org/r/53299/#comment229716>
I think your indentation here is off. 4 spaces for function arguments.
- Joris Van Remoortere
On Oct. 31, 2016, 2:48 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53299/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 2:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-6484
> https://issues.apache.org/jira/browse/MESOS-6484
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Removes a reference counted pointer that futures kept to themselves
> when using the method `Future<T>::after()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 175214a9090f8cc8241a81e535c87370102f3011
> 3rdparty/libprocess/src/tests/future_tests.cpp 7c411c7be1849119fe0b070622dbe4488fa11b7a
>
> Diff: https://reviews.apache.org/r/53299/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53299: Fixed memory leak in implementation of
Future::after().
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Dec. 12, 2016, 9:31 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/tests/future_tests.cpp, line 266
> > <https://reviews.apache.org/r/53299/diff/2/?file=1549125#file1549125line266>
> >
> > What does the word 'policy' here denote?
> > Could you pick something more consistent with the other tests?
Sorry, the name came from the original piece of code where I discovered the leak. Through the many iterations I reduce the code to the most simple way to reproduce the leak but I overlooked naming.
> On Dec. 12, 2016, 9:31 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 1321
> > <https://reviews.apache.org/r/53299/diff/2/?file=1549124#file1549124line1321>
> >
> > Can you explain why we're adding this behavior with a comment? It's not immediately clear to me from the existing code / comment what the implication is of making this association if the weak future is `none`.
I added a summary comment, but the cause is a little bit more complex than a one line code. Consider the following code:
```c++
Future<Nothing> future;
{
future = someFunction().after(Milliseconds(10), [](const Future<Nothing>&) { return Nothing; });
}
```
In that example, the future returned by `someFunction()` goes out of scope almost as soon as it appears, but the future returned by the `promised` is still kept.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53299/#review158898
-----------------------------------------------------------
On Oct. 31, 2016, 3:48 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53299/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 3:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Joris Van Remoortere.
>
>
> Bugs: MESOS-6484
> https://issues.apache.org/jira/browse/MESOS-6484
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Removes a reference counted pointer that futures kept to themselves
> when using the method `Future<T>::after()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 175214a9090f8cc8241a81e535c87370102f3011
> 3rdparty/libprocess/src/tests/future_tests.cpp 7c411c7be1849119fe0b070622dbe4488fa11b7a
>
> Diff: https://reviews.apache.org/r/53299/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Alexander Rojas
>
>