mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 57358: Implemented discard behavior in process::Queue.
Date Wed, 15 Mar 2017 18:01:28 GMT

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

(Updated March 15, 2017, 11:01 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Changes
-------

Changed approach.  This commit now modifies `process::Queue` and adds logic to handle discarded
futures in the Queue itself.


Summary (updated)
-----------------

Implemented discard behavior in process::Queue.


Bugs: MESOS-6919
    https://issues.apache.org/jira/browse/MESOS-6919


Repository: mesos


Description (updated)
-------

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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/57358/diff/2-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


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