----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51299/#review151193 -----------------------------------------------------------
Fix it, then Ship it! Modify the header i.e., we are not == Unix Pipes now and we do notify the caller now via discarding the future. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 341 - 342) <https://reviews.apache.org/r/51299/#comment219468> Modify this comment, ditto for the other comment. Do a sweep if there are nore. - Anand Mazumdar On Aug. 23, 2016, 10:49 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51299/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2016, 10:49 p.m.) > > > Review request for mesos, Anand Mazumdar, Greg Mann, and Artem Harutyunyan. > > > Repository: mesos > > > Description > ------- > > The `process::http::Pipe` class leaks its underlying `shared_ptr` > due to how the master (accidentally) causes the `shared_ptr` to hold > a self-reference. > > When the master receives a `SUBSCRIBE` call from an V1 HTTP API > framework, the master creates a `process::http::Pipe` to manage the > reading and writing in separate locations in the code. For > synchronization, the read/write ends of the HTTP connection share > some state (via `shared_ptr`). > > The master introduces a self-reference via this line in > `Master::addFramework`: > ``` > http.closed() > .onAny(defer(self(), &Self::exited, framework->id(), http)); > ``` > > `http.closed()` returns a future managed by the read-end of the `Pipe`. > `http` holds a copy of the write-end of the `Pipe`, which in turn has > a copy of the `shared_ptr`. Because `http` is passed into the future's > continuation, a copy of `http` will live inside the read-end's future > until the either (a) the continuation is executed or (b) the future > is destructed. > > Due to how we manage the future, neither (a) nor (b) are performed: > (a) When the read-end of the `Pipe` is closed, we only complete the > future if the write-end of the `Pipe` is still open. During > framework teardown, the write-end is closed first. > (b) The future in question lives inside a `Promise`, which lives > inside the `shared_ptr`. Because the self-reference exists, the > `shared_ptr` is never destructed; which means the `Promise` and > future are never destructed either. > > --- > > This patch fixes the leak by making sure the continuation is always > executed (a) or discarded. It does so by discarding the related > future when the write-end of the `Pipe` is already closed. > > > Diffs > ----- > > 3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e > 3rdparty/libprocess/src/tests/http_tests.cpp > 24b266df5f17e28e0c95326f5d1ea451952500e8 > > Diff: https://reviews.apache.org/r/51299/diff/ > > > Testing > ------- > > Found this leak in collaboration with Greg :) > > Ran valgrind before applying this patch: > ``` > LD_RUN_PATH=/path/to/mesos/build/src/.libs > LD_LIBRARY_PATH=/path/to/mesos/build/src/.libs valgrind --leak-check=full > src/.libs/mesos-tests --gtest_filter="*SchedulerTest.Teardown*" > ``` > > Observed the following leak (among many false positives): > ``` > 1,881 (216 direct, 1,665 indirect) bytes in 1 blocks are definitely lost in > loss record 2,405 of 2,507 > at 0x4C2A105: operator new(unsigned long) (in > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) > by 0xF259A9: process::http::Pipe::Pipe() (http.hpp:414) > by 0x5D629A1: > mesos::internal::master::Master::Http::scheduler(process::http::Request > const&, Option<std::string> const&) const (http.cpp:796) > by 0x5DB5806: operator() (master.cpp:858) > ... > ``` > > Applied the patch and re-ran valgrind. Confirmed that leak is gone. > > > Thanks, > > Joseph Wu > >
