----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57358/#review169065 -----------------------------------------------------------
Can you split the change to add discard semantics to queue? That patch seems orthogonal and doesn't need the big description about the socket problem. I think it's a pretty natural change to describe on its own. 3rdparty/libprocess/include/process/queue.hpp Lines 80-86 (patched) <https://reviews.apache.org/r/57358/#comment241381> This looks racy: (1) `put()` hits line 48, found the promise without a discard request. (2) Caller discards future corresponding to the promise found in (1), executes promise->discard. (3) `put()` hits line 59 and drops the `t` item on the floor. Perhaps we could implement the expensive discard approach with a TODO to optimize it, if it's simpler? Alternatively, we need to lock this section, but we need to make sure we don't deadlock (will let you think more about that). 3rdparty/libprocess/src/process.cpp Lines 942-947 (patched) <https://reviews.apache.org/r/57358/#comment241379> Can you comment on who is doing this discard? Or why it surfaces as discarded? The implication in your comment seems to be that this is the case where libprocess is finalizing? Couldn't quite follow why that's the case. - Benjamin Mahler On March 15, 2017, 6:01 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57358/ > ----------------------------------------------------------- > > (Updated March 15, 2017, 6:01 p.m.) > > > Review request for mesos, Benjamin Mahler and Jie Yu. > > > Bugs: MESOS-6919 > https://issues.apache.org/jira/browse/MESOS-6919 > > > Repository: mesos > > > Description > ------- > > This commit deals with a specific case where the SocketImpl class > passes a self-reference (shared_ptr) into a Future continuation and > then discards the original Future. The behavior of `Future::discard` > is that the `onDiscard` event is only chained to the future immediately > following the discarded future. i.e. > > ``` > Future<A> top; > top > .onDiscard([]() { /* This gets executed. */ }) > > .then([](const A&) { return Future<B>(); }) > .onDiscard([]() { /* This also gets executed. */ }) > > .then([](const B&) { return Future<C>(); }) > .onDiscard([]() { /* But not this. */ }); > > top.discard(); > ``` > > When a Future is discarded, we only clear the `onDiscard` callbacks, > which means that all other continuations will continue to reside in > memory. In the case of the `Socket` class, the Libevent SSL socket > implementation had several levels of continuations: > > ``` > // Each item in this queue is backed by a Promise<>::Future. > Queue<Future<std::shared_ptr<SocketImpl>>> accept_queue; > > // LibeventSSLSocketImpl::accept. > accept_queue.get() > .then(...) > > // Socket::accept. This continuation holds a self-reference > // to the `shared_ptr<SocketImpl>`. > .then([self](...) {...}) > ``` > > Because the second continuation is never discarded nor called, we > end up with a dangling pointer. For the `Socket` class, this leads > to an FD leak. > > This commit fixes the self-reference by implementing discard > handlers in the libprocess Queue class. When a waiting `Queue::get` > is discarded, the Promise backing the `Queue::get` is lazily dropped > and cleaned up. Data on the queue will then skip these discarded > Promises and satisfy the next pending waiter. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/queue.hpp > ab08e30df742412f22a06202526f8b55350ed435 > 3rdparty/libprocess/src/process.cpp > f6ee24e2db43d63d91222549efee85421bbf9bf3 > 3rdparty/libprocess/src/tests/queue_tests.cpp > 95b738133fa50641f8f9b83014837d2808e0e4ff > > > Diff: https://reviews.apache.org/r/57358/diff/3/ > > > Testing > ------- > > cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1 > > make libprocess-tests > > 3rdparty/libprocess/src/tests/libprocess-tests > --gtest_filter="Scheme/HTTPTest.Endpoints/0" --gtest_repeat="`ulimit -n`" > > make check > > > Thanks, > > Joseph Wu > >
