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