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 <neil.con...@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, <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; > > >
