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 <[email protected]> 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 <[email protected]> > 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, <[email protected]> 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 <[email protected]> >> > Authored: Tue May 10 15:33:13 2016 -0700 >> > Committer: Benjamin Mahler <[email protected]> >> > 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; >> > >> > >
