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: WIP: Fixed FD leak in SSL server socket cleanup.
Date Mon, 13 Mar 2017 22:35:09 GMT


> On March 13, 2017, 3:08 p.m., Benjamin Mahler wrote:
> > Do you still need this?

The description of the problem still stands, but, as we discussed offline, the fix will be
applied to the `Queue` class rather than the socket implementation.


- Joseph


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


On March 13, 2017, 3:34 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57358/
> -----------------------------------------------------------
> 
> (Updated March 13, 2017, 3:34 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 extends the future returned by the Libevent SSL Socket
> to deallocate some extra memory upon being discarded.  This will
> remove the extra reference to the SocketImpl class and thereby
> resolve the issue of a dangling pointer.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 9493a50243340a1c48ab1c67f6e61cc081ef24ce

>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 7d493301bd5c0f24bf89e0b213f07ffe7801508b

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