You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2016/05/11 23:50:45 UTC
mesos git commit: Fixed a head-of-line blocking bug in libevent SSL
socket.
Repository: mesos
Updated Branches:
refs/heads/master 95e670cd4 -> 28c085fca
Fixed a head-of-line blocking bug in libevent SSL socket.
Currently, the `accept_queue` is used to store connected sockets.
However, we push socket futures into this queue *before* they
complete the SSL handshake or are downgraded. This means that
a future returned from the queue may remain pending. If the user
writes a `Socket::accept` loop consuming accepted sockets they
will experience head-of-line blocking while a slow handshake or
downgrade is in progress.
Review: https://reviews.apache.org/r/47192
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/28c085fc
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/28c085fc
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/28c085fc
Branch: refs/heads/master
Commit: 28c085fcad70de128d147b486264395318c4d1ec
Parents: 95e670c
Author: Benjamin Mahler <bm...@apache.org>
Authored: Tue May 10 15:33:13 2016 -0700
Committer: Benjamin Mahler <bm...@apache.org>
Committed: Wed May 11 16:50:33 2016 -0700
----------------------------------------------------------------------
3rdparty/libprocess/src/libevent_ssl_socket.cpp | 17 ++++++++++++-----
3rdparty/libprocess/src/libevent_ssl_socket.hpp | 10 +++++-----
2 files changed, 17 insertions(+), 10 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
index b829e7d..2f844c2 100644
--- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
+++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
@@ -843,8 +843,9 @@ Future<Socket> LibeventSSLSocketImpl::accept()
// We explicitly specify the return type to avoid a type deduction
// issue in some versions of clang. See MESOS-2943.
return accept_queue.get()
- .then([](const Future<Socket>& future) -> Future<Socket> {
- return future;
+ .then([](const Future<Socket>& socket) -> Future<Socket> {
+ CHECK(!socket.isPending());
+ return socket;
});
}
@@ -920,9 +921,15 @@ void LibeventSSLSocketImpl::accept_callback(AcceptRequest* request)
{
CHECK(__in_event_loop__);
- // Enqueue a potential socket that we will set up SSL state for and
- // verify.
- accept_queue.put(request->promise.future());
+ Queue<Future<Socket>> accept_queue_ = accept_queue;
+
+ // After the socket is accepted, it must complete the SSL
+ // handshake (or be downgraded to a regular socket) before
+ // we put it in the queue of connected sockets.
+ request->promise.future()
+ .onAny([accept_queue_](Future<Socket> socket) mutable {
+ accept_queue_.put(socket);
+ });
// If we support downgrading the connection, first wait for this
// socket to become readable. We will then MSG_PEEK it to test
http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
index e773fad..9b6ba64 100644
--- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
+++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
@@ -170,11 +170,11 @@ private:
// event loop until it is destroyed.
std::weak_ptr<LibeventSSLSocketImpl>* event_loop_handle;
- // This queue stores buffered accepted sockets. 'Queue' is a thread
- // safe queue implementation, and the event loop pushes connected
- // sockets onto it, the 'accept()' call pops them off. We wrap these
- // sockets with futures so that we can pass errors through and chain
- // futures as well.
+ // This queue stores accepted sockets that are considered connected
+ // (either the SSL handshake has completed or the socket has been
+ // downgraded). The 'accept()' call returns sockets from this queue.
+ // We wrap the socket in a 'Future' so that we can pass failures or
+ // discards through.
Queue<Future<Socket>> accept_queue;
Option<std::string> peer_hostname;
Re: mesos git commit: Fixed a head-of-line blocking bug in libevent
SSL socket.
Posted by Benjamin Mahler <bm...@apache.org>.
Here is a test that catches the issue:
https://reviews.apache.org/r/47362/
On Thu, May 12, 2016 at 5:34 PM, Benjamin Mahler <bm...@apache.org> wrote:
> Yes definitely! We would want to test that a connection without any data
> sent does not block subsequent requests when SSL is enabled.
>
> On Thu, May 12, 2016 at 12:35 AM, Neil Conway <ne...@gmail.com>
> wrote:
>
>> Would it be possible to write a unit test that reproduces the original
>> problem? It should be pretty easy to repro, right?
>>
>> Neil
>>
>> On Thu, May 12, 2016 at 1:50 AM, <bm...@apache.org> wrote:
>> > Repository: mesos
>> > Updated Branches:
>> > refs/heads/master 95e670cd4 -> 28c085fca
>> >
>> >
>> > Fixed a head-of-line blocking bug in libevent SSL socket.
>> >
>> > Currently, the `accept_queue` is used to store connected sockets.
>> > However, we push socket futures into this queue *before* they
>> > complete the SSL handshake or are downgraded. This means that
>> > a future returned from the queue may remain pending. If the user
>> > writes a `Socket::accept` loop consuming accepted sockets they
>> > will experience head-of-line blocking while a slow handshake or
>> > downgrade is in progress.
>> >
>> > Review: https://reviews.apache.org/r/47192
>> >
>> >
>> > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/28c085fc
>> > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/28c085fc
>> > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/28c085fc
>> >
>> > Branch: refs/heads/master
>> > Commit: 28c085fcad70de128d147b486264395318c4d1ec
>> > Parents: 95e670c
>> > Author: Benjamin Mahler <bm...@apache.org>
>> > Authored: Tue May 10 15:33:13 2016 -0700
>> > Committer: Benjamin Mahler <bm...@apache.org>
>> > Committed: Wed May 11 16:50:33 2016 -0700
>> >
>> > ----------------------------------------------------------------------
>> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 17 ++++++++++++-----
>> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp | 10 +++++-----
>> > 2 files changed, 17 insertions(+), 10 deletions(-)
>> > ----------------------------------------------------------------------
>> >
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > ----------------------------------------------------------------------
>> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > index b829e7d..2f844c2 100644
>> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > @@ -843,8 +843,9 @@ Future<Socket> LibeventSSLSocketImpl::accept()
>> > // We explicitly specify the return type to avoid a type deduction
>> > // issue in some versions of clang. See MESOS-2943.
>> > return accept_queue.get()
>> > - .then([](const Future<Socket>& future) -> Future<Socket> {
>> > - return future;
>> > + .then([](const Future<Socket>& socket) -> Future<Socket> {
>> > + CHECK(!socket.isPending());
>> > + return socket;
>> > });
>> > }
>> >
>> > @@ -920,9 +921,15 @@ void
>> LibeventSSLSocketImpl::accept_callback(AcceptRequest* request)
>> > {
>> > CHECK(__in_event_loop__);
>> >
>> > - // Enqueue a potential socket that we will set up SSL state for and
>> > - // verify.
>> > - accept_queue.put(request->promise.future());
>> > + Queue<Future<Socket>> accept_queue_ = accept_queue;
>> > +
>> > + // After the socket is accepted, it must complete the SSL
>> > + // handshake (or be downgraded to a regular socket) before
>> > + // we put it in the queue of connected sockets.
>> > + request->promise.future()
>> > + .onAny([accept_queue_](Future<Socket> socket) mutable {
>> > + accept_queue_.put(socket);
>> > + });
>> >
>> > // If we support downgrading the connection, first wait for this
>> > // socket to become readable. We will then MSG_PEEK it to test
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > ----------------------------------------------------------------------
>> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > index e773fad..9b6ba64 100644
>> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > @@ -170,11 +170,11 @@ private:
>> > // event loop until it is destroyed.
>> > std::weak_ptr<LibeventSSLSocketImpl>* event_loop_handle;
>> >
>> > - // This queue stores buffered accepted sockets. 'Queue' is a thread
>> > - // safe queue implementation, and the event loop pushes connected
>> > - // sockets onto it, the 'accept()' call pops them off. We wrap these
>> > - // sockets with futures so that we can pass errors through and chain
>> > - // futures as well.
>> > + // This queue stores accepted sockets that are considered connected
>> > + // (either the SSL handshake has completed or the socket has been
>> > + // downgraded). The 'accept()' call returns sockets from this queue.
>> > + // We wrap the socket in a 'Future' so that we can pass failures or
>> > + // discards through.
>> > Queue<Future<Socket>> accept_queue;
>> >
>> > Option<std::string> peer_hostname;
>> >
>>
>
>
Re: mesos git commit: Fixed a head-of-line blocking bug in libevent
SSL socket.
Posted by Benjamin Mahler <bm...@apache.org>.
Here is a test that catches the issue:
https://reviews.apache.org/r/47362/
On Thu, May 12, 2016 at 5:34 PM, Benjamin Mahler <bm...@apache.org> wrote:
> Yes definitely! We would want to test that a connection without any data
> sent does not block subsequent requests when SSL is enabled.
>
> On Thu, May 12, 2016 at 12:35 AM, Neil Conway <ne...@gmail.com>
> wrote:
>
>> Would it be possible to write a unit test that reproduces the original
>> problem? It should be pretty easy to repro, right?
>>
>> Neil
>>
>> On Thu, May 12, 2016 at 1:50 AM, <bm...@apache.org> wrote:
>> > Repository: mesos
>> > Updated Branches:
>> > refs/heads/master 95e670cd4 -> 28c085fca
>> >
>> >
>> > Fixed a head-of-line blocking bug in libevent SSL socket.
>> >
>> > Currently, the `accept_queue` is used to store connected sockets.
>> > However, we push socket futures into this queue *before* they
>> > complete the SSL handshake or are downgraded. This means that
>> > a future returned from the queue may remain pending. If the user
>> > writes a `Socket::accept` loop consuming accepted sockets they
>> > will experience head-of-line blocking while a slow handshake or
>> > downgrade is in progress.
>> >
>> > Review: https://reviews.apache.org/r/47192
>> >
>> >
>> > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/28c085fc
>> > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/28c085fc
>> > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/28c085fc
>> >
>> > Branch: refs/heads/master
>> > Commit: 28c085fcad70de128d147b486264395318c4d1ec
>> > Parents: 95e670c
>> > Author: Benjamin Mahler <bm...@apache.org>
>> > Authored: Tue May 10 15:33:13 2016 -0700
>> > Committer: Benjamin Mahler <bm...@apache.org>
>> > Committed: Wed May 11 16:50:33 2016 -0700
>> >
>> > ----------------------------------------------------------------------
>> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 17 ++++++++++++-----
>> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp | 10 +++++-----
>> > 2 files changed, 17 insertions(+), 10 deletions(-)
>> > ----------------------------------------------------------------------
>> >
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > ----------------------------------------------------------------------
>> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > index b829e7d..2f844c2 100644
>> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
>> > @@ -843,8 +843,9 @@ Future<Socket> LibeventSSLSocketImpl::accept()
>> > // We explicitly specify the return type to avoid a type deduction
>> > // issue in some versions of clang. See MESOS-2943.
>> > return accept_queue.get()
>> > - .then([](const Future<Socket>& future) -> Future<Socket> {
>> > - return future;
>> > + .then([](const Future<Socket>& socket) -> Future<Socket> {
>> > + CHECK(!socket.isPending());
>> > + return socket;
>> > });
>> > }
>> >
>> > @@ -920,9 +921,15 @@ void
>> LibeventSSLSocketImpl::accept_callback(AcceptRequest* request)
>> > {
>> > CHECK(__in_event_loop__);
>> >
>> > - // Enqueue a potential socket that we will set up SSL state for and
>> > - // verify.
>> > - accept_queue.put(request->promise.future());
>> > + Queue<Future<Socket>> accept_queue_ = accept_queue;
>> > +
>> > + // After the socket is accepted, it must complete the SSL
>> > + // handshake (or be downgraded to a regular socket) before
>> > + // we put it in the queue of connected sockets.
>> > + request->promise.future()
>> > + .onAny([accept_queue_](Future<Socket> socket) mutable {
>> > + accept_queue_.put(socket);
>> > + });
>> >
>> > // If we support downgrading the connection, first wait for this
>> > // socket to become readable. We will then MSG_PEEK it to test
>> >
>> >
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > ----------------------------------------------------------------------
>> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > index e773fad..9b6ba64 100644
>> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
>> > @@ -170,11 +170,11 @@ private:
>> > // event loop until it is destroyed.
>> > std::weak_ptr<LibeventSSLSocketImpl>* event_loop_handle;
>> >
>> > - // This queue stores buffered accepted sockets. 'Queue' is a thread
>> > - // safe queue implementation, and the event loop pushes connected
>> > - // sockets onto it, the 'accept()' call pops them off. We wrap these
>> > - // sockets with futures so that we can pass errors through and chain
>> > - // futures as well.
>> > + // This queue stores accepted sockets that are considered connected
>> > + // (either the SSL handshake has completed or the socket has been
>> > + // downgraded). The 'accept()' call returns sockets from this queue.
>> > + // We wrap the socket in a 'Future' so that we can pass failures or
>> > + // discards through.
>> > Queue<Future<Socket>> accept_queue;
>> >
>> > Option<std::string> peer_hostname;
>> >
>>
>
>
Re: mesos git commit: Fixed a head-of-line blocking bug in libevent
SSL socket.
Posted by Benjamin Mahler <bm...@apache.org>.
Yes definitely! We would want to test that a connection without any data
sent does not block subsequent requests when SSL is enabled.
On Thu, May 12, 2016 at 12:35 AM, Neil Conway <ne...@gmail.com> wrote:
> Would it be possible to write a unit test that reproduces the original
> problem? It should be pretty easy to repro, right?
>
> Neil
>
> On Thu, May 12, 2016 at 1:50 AM, <bm...@apache.org> wrote:
> > Repository: mesos
> > Updated Branches:
> > refs/heads/master 95e670cd4 -> 28c085fca
> >
> >
> > Fixed a head-of-line blocking bug in libevent SSL socket.
> >
> > Currently, the `accept_queue` is used to store connected sockets.
> > However, we push socket futures into this queue *before* they
> > complete the SSL handshake or are downgraded. This means that
> > a future returned from the queue may remain pending. If the user
> > writes a `Socket::accept` loop consuming accepted sockets they
> > will experience head-of-line blocking while a slow handshake or
> > downgrade is in progress.
> >
> > Review: https://reviews.apache.org/r/47192
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/28c085fc
> > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/28c085fc
> > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/28c085fc
> >
> > Branch: refs/heads/master
> > Commit: 28c085fcad70de128d147b486264395318c4d1ec
> > Parents: 95e670c
> > Author: Benjamin Mahler <bm...@apache.org>
> > Authored: Tue May 10 15:33:13 2016 -0700
> > Committer: Benjamin Mahler <bm...@apache.org>
> > Committed: Wed May 11 16:50:33 2016 -0700
> >
> > ----------------------------------------------------------------------
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 17 ++++++++++++-----
> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp | 10 +++++-----
> > 2 files changed, 17 insertions(+), 10 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > ----------------------------------------------------------------------
> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > index b829e7d..2f844c2 100644
> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > @@ -843,8 +843,9 @@ Future<Socket> LibeventSSLSocketImpl::accept()
> > // We explicitly specify the return type to avoid a type deduction
> > // issue in some versions of clang. See MESOS-2943.
> > return accept_queue.get()
> > - .then([](const Future<Socket>& future) -> Future<Socket> {
> > - return future;
> > + .then([](const Future<Socket>& socket) -> Future<Socket> {
> > + CHECK(!socket.isPending());
> > + return socket;
> > });
> > }
> >
> > @@ -920,9 +921,15 @@ void
> LibeventSSLSocketImpl::accept_callback(AcceptRequest* request)
> > {
> > CHECK(__in_event_loop__);
> >
> > - // Enqueue a potential socket that we will set up SSL state for and
> > - // verify.
> > - accept_queue.put(request->promise.future());
> > + Queue<Future<Socket>> accept_queue_ = accept_queue;
> > +
> > + // After the socket is accepted, it must complete the SSL
> > + // handshake (or be downgraded to a regular socket) before
> > + // we put it in the queue of connected sockets.
> > + request->promise.future()
> > + .onAny([accept_queue_](Future<Socket> socket) mutable {
> > + accept_queue_.put(socket);
> > + });
> >
> > // If we support downgrading the connection, first wait for this
> > // socket to become readable. We will then MSG_PEEK it to test
> >
> >
> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > ----------------------------------------------------------------------
> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > index e773fad..9b6ba64 100644
> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > @@ -170,11 +170,11 @@ private:
> > // event loop until it is destroyed.
> > std::weak_ptr<LibeventSSLSocketImpl>* event_loop_handle;
> >
> > - // This queue stores buffered accepted sockets. 'Queue' is a thread
> > - // safe queue implementation, and the event loop pushes connected
> > - // sockets onto it, the 'accept()' call pops them off. We wrap these
> > - // sockets with futures so that we can pass errors through and chain
> > - // futures as well.
> > + // This queue stores accepted sockets that are considered connected
> > + // (either the SSL handshake has completed or the socket has been
> > + // downgraded). The 'accept()' call returns sockets from this queue.
> > + // We wrap the socket in a 'Future' so that we can pass failures or
> > + // discards through.
> > Queue<Future<Socket>> accept_queue;
> >
> > Option<std::string> peer_hostname;
> >
>
Re: mesos git commit: Fixed a head-of-line blocking bug in libevent
SSL socket.
Posted by Benjamin Mahler <bm...@apache.org>.
Yes definitely! We would want to test that a connection without any data
sent does not block subsequent requests when SSL is enabled.
On Thu, May 12, 2016 at 12:35 AM, Neil Conway <ne...@gmail.com> wrote:
> Would it be possible to write a unit test that reproduces the original
> problem? It should be pretty easy to repro, right?
>
> Neil
>
> On Thu, May 12, 2016 at 1:50 AM, <bm...@apache.org> wrote:
> > Repository: mesos
> > Updated Branches:
> > refs/heads/master 95e670cd4 -> 28c085fca
> >
> >
> > Fixed a head-of-line blocking bug in libevent SSL socket.
> >
> > Currently, the `accept_queue` is used to store connected sockets.
> > However, we push socket futures into this queue *before* they
> > complete the SSL handshake or are downgraded. This means that
> > a future returned from the queue may remain pending. If the user
> > writes a `Socket::accept` loop consuming accepted sockets they
> > will experience head-of-line blocking while a slow handshake or
> > downgrade is in progress.
> >
> > Review: https://reviews.apache.org/r/47192
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/28c085fc
> > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/28c085fc
> > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/28c085fc
> >
> > Branch: refs/heads/master
> > Commit: 28c085fcad70de128d147b486264395318c4d1ec
> > Parents: 95e670c
> > Author: Benjamin Mahler <bm...@apache.org>
> > Authored: Tue May 10 15:33:13 2016 -0700
> > Committer: Benjamin Mahler <bm...@apache.org>
> > Committed: Wed May 11 16:50:33 2016 -0700
> >
> > ----------------------------------------------------------------------
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 17 ++++++++++++-----
> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp | 10 +++++-----
> > 2 files changed, 17 insertions(+), 10 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > ----------------------------------------------------------------------
> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > index b829e7d..2f844c2 100644
> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > @@ -843,8 +843,9 @@ Future<Socket> LibeventSSLSocketImpl::accept()
> > // We explicitly specify the return type to avoid a type deduction
> > // issue in some versions of clang. See MESOS-2943.
> > return accept_queue.get()
> > - .then([](const Future<Socket>& future) -> Future<Socket> {
> > - return future;
> > + .then([](const Future<Socket>& socket) -> Future<Socket> {
> > + CHECK(!socket.isPending());
> > + return socket;
> > });
> > }
> >
> > @@ -920,9 +921,15 @@ void
> LibeventSSLSocketImpl::accept_callback(AcceptRequest* request)
> > {
> > CHECK(__in_event_loop__);
> >
> > - // Enqueue a potential socket that we will set up SSL state for and
> > - // verify.
> > - accept_queue.put(request->promise.future());
> > + Queue<Future<Socket>> accept_queue_ = accept_queue;
> > +
> > + // After the socket is accepted, it must complete the SSL
> > + // handshake (or be downgraded to a regular socket) before
> > + // we put it in the queue of connected sockets.
> > + request->promise.future()
> > + .onAny([accept_queue_](Future<Socket> socket) mutable {
> > + accept_queue_.put(socket);
> > + });
> >
> > // If we support downgrading the connection, first wait for this
> > // socket to become readable. We will then MSG_PEEK it to test
> >
> >
> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > ----------------------------------------------------------------------
> > diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > index e773fad..9b6ba64 100644
> > --- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > +++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> > @@ -170,11 +170,11 @@ private:
> > // event loop until it is destroyed.
> > std::weak_ptr<LibeventSSLSocketImpl>* event_loop_handle;
> >
> > - // This queue stores buffered accepted sockets. 'Queue' is a thread
> > - // safe queue implementation, and the event loop pushes connected
> > - // sockets onto it, the 'accept()' call pops them off. We wrap these
> > - // sockets with futures so that we can pass errors through and chain
> > - // futures as well.
> > + // This queue stores accepted sockets that are considered connected
> > + // (either the SSL handshake has completed or the socket has been
> > + // downgraded). The 'accept()' call returns sockets from this queue.
> > + // We wrap the socket in a 'Future' so that we can pass failures or
> > + // discards through.
> > Queue<Future<Socket>> accept_queue;
> >
> > Option<std::string> peer_hostname;
> >
>
Re: mesos git commit: Fixed a head-of-line blocking bug in libevent
SSL socket.
Posted by Neil Conway <ne...@gmail.com>.
Would it be possible to write a unit test that reproduces the original
problem? It should be pretty easy to repro, right?
Neil
On Thu, May 12, 2016 at 1:50 AM, <bm...@apache.org> wrote:
> Repository: mesos
> Updated Branches:
> refs/heads/master 95e670cd4 -> 28c085fca
>
>
> Fixed a head-of-line blocking bug in libevent SSL socket.
>
> Currently, the `accept_queue` is used to store connected sockets.
> However, we push socket futures into this queue *before* they
> complete the SSL handshake or are downgraded. This means that
> a future returned from the queue may remain pending. If the user
> writes a `Socket::accept` loop consuming accepted sockets they
> will experience head-of-line blocking while a slow handshake or
> downgrade is in progress.
>
> Review: https://reviews.apache.org/r/47192
>
>
> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/28c085fc
> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/28c085fc
> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/28c085fc
>
> Branch: refs/heads/master
> Commit: 28c085fcad70de128d147b486264395318c4d1ec
> Parents: 95e670c
> Author: Benjamin Mahler <bm...@apache.org>
> Authored: Tue May 10 15:33:13 2016 -0700
> Committer: Benjamin Mahler <bm...@apache.org>
> Committed: Wed May 11 16:50:33 2016 -0700
>
> ----------------------------------------------------------------------
> 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 17 ++++++++++++-----
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp | 10 +++++-----
> 2 files changed, 17 insertions(+), 10 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> ----------------------------------------------------------------------
> diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> index b829e7d..2f844c2 100644
> --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
> @@ -843,8 +843,9 @@ Future<Socket> LibeventSSLSocketImpl::accept()
> // We explicitly specify the return type to avoid a type deduction
> // issue in some versions of clang. See MESOS-2943.
> return accept_queue.get()
> - .then([](const Future<Socket>& future) -> Future<Socket> {
> - return future;
> + .then([](const Future<Socket>& socket) -> Future<Socket> {
> + CHECK(!socket.isPending());
> + return socket;
> });
> }
>
> @@ -920,9 +921,15 @@ void LibeventSSLSocketImpl::accept_callback(AcceptRequest* request)
> {
> CHECK(__in_event_loop__);
>
> - // Enqueue a potential socket that we will set up SSL state for and
> - // verify.
> - accept_queue.put(request->promise.future());
> + Queue<Future<Socket>> accept_queue_ = accept_queue;
> +
> + // After the socket is accepted, it must complete the SSL
> + // handshake (or be downgraded to a regular socket) before
> + // we put it in the queue of connected sockets.
> + request->promise.future()
> + .onAny([accept_queue_](Future<Socket> socket) mutable {
> + accept_queue_.put(socket);
> + });
>
> // If we support downgrading the connection, first wait for this
> // socket to become readable. We will then MSG_PEEK it to test
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/28c085fc/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> ----------------------------------------------------------------------
> diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> index e773fad..9b6ba64 100644
> --- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> +++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
> @@ -170,11 +170,11 @@ private:
> // event loop until it is destroyed.
> std::weak_ptr<LibeventSSLSocketImpl>* event_loop_handle;
>
> - // This queue stores buffered accepted sockets. 'Queue' is a thread
> - // safe queue implementation, and the event loop pushes connected
> - // sockets onto it, the 'accept()' call pops them off. We wrap these
> - // sockets with futures so that we can pass errors through and chain
> - // futures as well.
> + // This queue stores accepted sockets that are considered connected
> + // (either the SSL handshake has completed or the socket has been
> + // downgraded). The 'accept()' call returns sockets from this queue.
> + // We wrap the socket in a 'Future' so that we can pass failures or
> + // discards through.
> Queue<Future<Socket>> accept_queue;
>
> Option<std::string> peer_hostname;
>