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 18:57:36 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?
> 
> Aaron Wood wrote:
>     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

Sorry, meant to link directly to the post https://github.com/apache/mesos/pull/157#issuecomment-240482442


- Aaron


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


On Aug. 17, 2016, 6:21 p.m., Aaron Wood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51068/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2016, 6:21 p.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