You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2013/12/12 01:45:15 UTC

Review Request 16197: Fixed the memory leak in Future::then and Promise::associate.

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

Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.


Bugs: MESOS-877
    https://issues.apache.org/jira/browse/MESOS-877


Repository: mesos-git


Description
-------

See summary. The idea is to introduce a WeakReference for Future (basically a wrapper for std::weak_ptr).

Also, added an 'upgrade' function in WeakReference which returns Option<Future<T> >. (pretty similar to std::weak_ptr::lock).

I tested the new impl. with the replicated log which requires strict reference counting and cannot tolerate a leak in Future. It passed. Of course, make check passed as well.


Diffs
-----

  3rdparty/libprocess/include/process/future.hpp e473b3d 

Diff: https://reviews.apache.org/r/16197/diff/


Testing
-------

make check


Thanks,

Jie Yu

Re: Review Request 16197: Fixed the memory leak in Future::then and Promise::associate.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16197/#review30288
-----------------------------------------------------------



3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/16197/#comment57975>

    When I had added these TODO's before I was thinking of implementing it as a WeakFuture<T> it's own top-level class. I think that the concept of a weak future can be used outside of the implementation too, and I'd rather see code like WeakFuture<T> than Future<T>::WeakReference. In fact, I can see WeakShared<T> too and I think I'd prefer that to Shared<T>::WeakReference.



3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/16197/#comment57978>

    How about s/upgrade/get/ (that's the Java API)?



3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/16197/#comment57979>

    s/reference/data/



3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/16197/#comment57981>

    A suggestion:
    
    -------
    Future<T> future;
    future.data = data.lock();
    
    if (future.data) {
      return future;
    }
    
    return None();
    -------


- Benjamin Hindman


On Dec. 12, 2013, 6:39 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16197/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 6:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-877
>     https://issues.apache.org/jira/browse/MESOS-877
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. The idea is to introduce a WeakReference for Future (basically a wrapper for std::weak_ptr).
> 
> Also, added an 'upgrade' function in WeakReference which returns Option<Future<T> >. (pretty similar to std::weak_ptr::lock).
> 
> I tested the new impl. with the replicated log which requires strict reference counting and cannot tolerate a leak in Future. It passed. Of course, make check passed as well.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/future.hpp e473b3d 
> 
> Diff: https://reviews.apache.org/r/16197/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 16197: Fixed the memory leak in Future::then and Promise::associate.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16197/#review30275
-----------------------------------------------------------


Looks good. Please ping benh to take a look as well.


3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/16197/#comment57964>

    two space indent


- Ben Mahler


On Dec. 12, 2013, 6:39 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16197/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 6:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-877
>     https://issues.apache.org/jira/browse/MESOS-877
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. The idea is to introduce a WeakReference for Future (basically a wrapper for std::weak_ptr).
> 
> Also, added an 'upgrade' function in WeakReference which returns Option<Future<T> >. (pretty similar to std::weak_ptr::lock).
> 
> I tested the new impl. with the replicated log which requires strict reference counting and cannot tolerate a leak in Future. It passed. Of course, make check passed as well.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/future.hpp e473b3d 
> 
> Diff: https://reviews.apache.org/r/16197/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 16197: Fixed the memory leak in Future::then and Promise::associate.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16197/#review30293
-----------------------------------------------------------

Ship it!


Ship It!

- Benjamin Hindman


On Dec. 12, 2013, 10:56 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16197/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 10:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-877
>     https://issues.apache.org/jira/browse/MESOS-877
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. The idea is to introduce a WeakReference for Future (basically a wrapper for std::weak_ptr).
> 
> Also, added an 'upgrade' function in WeakReference which returns Option<Future<T> >. (pretty similar to std::weak_ptr::lock).
> 
> I tested the new impl. with the replicated log which requires strict reference counting and cannot tolerate a leak in Future. It passed. Of course, make check passed as well.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/future.hpp e473b3d 
> 
> Diff: https://reviews.apache.org/r/16197/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 16197: Fixed the memory leak in Future::then and Promise::associate.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16197/
-----------------------------------------------------------

(Updated Dec. 12, 2013, 10:56 p.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.


Changes
-------

Two Ben's reviews.


Bugs: MESOS-877
    https://issues.apache.org/jira/browse/MESOS-877


Repository: mesos-git


Description
-------

See summary. The idea is to introduce a WeakReference for Future (basically a wrapper for std::weak_ptr).

Also, added an 'upgrade' function in WeakReference which returns Option<Future<T> >. (pretty similar to std::weak_ptr::lock).

I tested the new impl. with the replicated log which requires strict reference counting and cannot tolerate a leak in Future. It passed. Of course, make check passed as well.


Diffs (updated)
-----

  3rdparty/libprocess/include/process/future.hpp e473b3d 

Diff: https://reviews.apache.org/r/16197/diff/


Testing
-------

make check


Thanks,

Jie Yu

Re: Review Request 16197: Fixed the memory leak in Future::then and Promise::associate.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16197/
-----------------------------------------------------------

(Updated Dec. 12, 2013, 6:39 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.


Changes
-------

A different style (I liked this better).

In fact, exposing WeakReference is not a bad idea because it cannot be used until it is upgraded.


Bugs: MESOS-877
    https://issues.apache.org/jira/browse/MESOS-877


Repository: mesos-git


Description
-------

See summary. The idea is to introduce a WeakReference for Future (basically a wrapper for std::weak_ptr).

Also, added an 'upgrade' function in WeakReference which returns Option<Future<T> >. (pretty similar to std::weak_ptr::lock).

I tested the new impl. with the replicated log which requires strict reference counting and cannot tolerate a leak in Future. It passed. Of course, make check passed as well.


Diffs (updated)
-----

  3rdparty/libprocess/include/process/future.hpp e473b3d 

Diff: https://reviews.apache.org/r/16197/diff/


Testing
-------

make check


Thanks,

Jie Yu