mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: mesos git commit: Fixed a head-of-line blocking bug in libevent SSL socket.
Date Fri, 13 May 2016 21:47:39 GMT
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 <bmahler@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 <neil.conway@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,  <bmahler@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 <bmahler@apache.org>
>> > Authored: Tue May 10 15:33:13 2016 -0700
>> > Committer: Benjamin Mahler <bmahler@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;
>> >
>>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message