You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/03/07 18:09:09 UTC
Re: Review Request 57358: WIP: Fixed potential for circular
dependencies in Future continuations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57358/
-----------------------------------------------------------
(Updated March 7, 2017, 10:09 a.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.
Changes
-------
Need to rethink approach, as this change "fixes" the leak, but ends up changing the semantics of `Future` and thereby fails a bunch of regression tests.
Summary (updated)
-----------------
WIP: Fixed potential for circular dependencies in Future continuations.
Bugs: MESOS-6919
https://issues.apache.org/jira/browse/MESOS-6919
Repository: mesos
Description
-------
This commit deals with a specific case where an object 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.
Diffs
-----
3rdparty/libprocess/include/process/future.hpp ec7ef2251947f5f42a202af0d6cb0858338e7755
Diff: https://reviews.apache.org/r/57358/diff/1/
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`"
Thanks,
Joseph Wu
Re: Review Request 57358: Fixed FD leak in SSL server socket cleanup.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57358/#review168191
-----------------------------------------------------------
Fix it, then Ship it!
Verified that this fixes the FD leak and all tests pass after the patch.
What do you think about adding a TODO and create a JIRA to fix this in a way that doesn't alter the discard semantics of the futures returned by `LibeventSSLSocketImpl::accept`? (Maybe investigate the ramifications of altering `Queue` to remove promises associated with discarded futures?) It's not ideal to change the discard behavior, but who knows if it will ever actually be an issue for us.
3rdparty/libprocess/src/libevent_ssl_socket.cpp
Lines 967 (patched)
<https://reviews.apache.org/r/57358/#comment240354>
s/an/any/
- Greg Mann
On March 7, 2017, 8:19 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57358/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 8:19 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.
>
>
> 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
>
>
Re: Review Request 57358: Fixed FD leak in SSL server socket cleanup.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57358/
-----------------------------------------------------------
(Updated March 7, 2017, 12:19 p.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.
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 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
Re: Review Request 57358: Fixed FD leak in SSL server socket cleanup.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57358/
-----------------------------------------------------------
(Updated March 7, 2017, 12:18 p.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.
Changes
-------
Different approach, which doesn't break tests, but slightly modifies the semantics of `Socket::accept`. The assumption is that, when you discard the future from `Socket::accept`, you are preparing to clean up the socket and thereby do not care if the socket gives you inconsistent results.
Summary (updated)
-----------------
Fixed FD leak in SSL server socket cleanup.
Bugs: MESOS-6919
https://issues.apache.org/jira/browse/MESOS-6919
Repository: mesos
Description (updated)
-------
Fixed FD leak in SSL server socket cleanup.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/57358/diff/1-2/
Testing (updated)
-------
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