mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Wood <aaron.w...@verizon.com>
Subject Re: Review Request 51068: Prevent memory leaks
Date Wed, 17 Aug 2016 17:57:13 GMT


> On Aug. 16, 2016, 11:35 p.m., Benjamin Mahler wrote:
> > src/zookeeper/zookeeper.cpp, lines 196-220
> > <https://reviews.apache.org/r/51068/diff/1/?file=1472488#file1472488line196>
> >
> >     Promise is now on the stack here, but the asynchronous callbacks (voidCompletion,
stringCompletion, statCompletion, dataCompletion) need to access the promise to satisfy the
future. There doesn't appear to have been a leak here in that the callbacks (voidCompletion,
stringCompletion, statCompletion, dataCompletion) delete the promise after satisfying it.
Can you add more detail as to why you're making this change?

Hi Ben, thanks for reviewing this patch! Can I ask you the same thing that I asked in my most
recent comment here? https://github.com/apache/mesos/pull/157

I see what you're saying about the callbacks taking care of deleting the promise when necessary,
I missed that the first time around. The callbacks should be taking the pointer to the promise
from the args object that's allocated on the heap here, right? If that's the case, I'm thinking
that what's happening in the original version is this:

1. The promise object is allocated on the heap
2. The pointer to this object is passed into the copy constructor of the tuple so that a copy
is taken internally in args
3. The future is returned without deleting the memory for the promise
4. Later on when one of the callbacks is called and the promise gets deleted, the promise
that actually gets deleted is the copy that was taken in the tuple which was passed to one
of the zookeeper C functions
5. The original promise that was copied into the tuple via the copy constructor still lives


- Aaron


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


On Aug. 13, 2016, 8:08 a.m., Aaron Wood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51068/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2016, 8:08 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This should prevent any of the promises that are created in the various ZookeeperProcess
class methods from leaking memory.
> 
> 
> Diffs
> -----
> 
>   docs/contributors.yaml 3f06000 
>   src/zookeeper/zookeeper.cpp e105377 
> 
> Diff: https://reviews.apache.org/r/51068/diff/
> 
> 
> Testing
> -------
> 
> make && make check
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>


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