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, <bmah...@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 <bmah...@apache.org> > Authored: Tue May 10 15:33:13 2016 -0700 > Committer: Benjamin Mahler <bmah...@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; >
