mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From b...@apache.org
Subject mesos git commit: Eliminated an EOF race condition in libprocess SSL socket.
Date Wed, 11 Jan 2017 02:24:03 GMT
Repository: mesos
Updated Branches:
  refs/heads/master bea98b9b9 -> d4185d4a1


Eliminated an EOF race condition in libprocess SSL socket.

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.

Review: https://reviews.apache.org/r/53802/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d4185d4a
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d4185d4a
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d4185d4a

Branch: refs/heads/master
Commit: d4185d4a117d9cba32d80993d14fcefb566cd9f9
Parents: bea98b9
Author: Greg Mann <greg@mesosphere.io>
Authored: Tue Jan 10 18:23:45 2017 -0800
Committer: Benjamin Hindman <benjamin.hindman@gmail.com>
Committed: Tue Jan 10 18:23:45 2017 -0800

----------------------------------------------------------------------
 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 56 ++++++++++++++------
 3rdparty/libprocess/src/libevent_ssl_socket.hpp |  5 ++
 2 files changed, 44 insertions(+), 17 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/d4185d4a/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 dddd0e2..b0319b2 100644
--- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
+++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
@@ -249,23 +249,22 @@ void LibeventSSLSocketImpl::recv_callback()
   // g. libevent callback is called for the event queued at step b.
   // h. libevent callback finds the length of the buffer as 0 but the request is
   //    a non-nullptr due to step f.
-  if (buffer_length > 0) {
+  if (buffer_length > 0 || received_eof) {
     synchronized (lock) {
       std::swap(request, recv_request);
     }
   }
 
   if (request.get() != nullptr) {
-    // There is an invariant that if we are executing a
-    // 'recv_callback' and we have a request there must be data here
-    // because we should not be getting a spurrious receive callback
-    // invocation. Even if we discarded a request, the manual
-    // invocation of 'recv_callback' guarantees that there is a
-    // non-zero amount of data available in the bufferevent.
-    size_t length = bufferevent_read(bev, request->data, request->size);
-    CHECK(length > 0);
-
-    request->promise.set(length);
+    if (buffer_length > 0) {
+      size_t length = bufferevent_read(bev, request->data, request->size);
+      CHECK(length > 0);
+
+      request->promise.set(length);
+    } else {
+      CHECK(received_eof);
+      request->promise.set(0);
+    }
   }
 }
 
@@ -360,11 +359,33 @@ void LibeventSSLSocketImpl::event_callback(short events)
   // either because it was never created, it has already been
   // completed, or it has been discarded.
 
+  // The case below where `EVUTIL_SOCKET_ERROR() == 0` will catch
+  // unclean shutdowns of the socket.
+  //
+  // TODO(greggomann): We should make use of the `BEV_EVENT_READING`
+  // and `BEV_EVENT_WRITING` flags to handle read and write errors
+  // differently. Related JIRA: MESOS-6770
   if (events & BEV_EVENT_EOF ||
      (events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0)) {
     // At end of file, close the connection.
     if (current_recv_request.get() != nullptr) {
-      current_recv_request->promise.set(0);
+      received_eof = true;
+      // Drain any remaining data from the bufferevent or complete the
+      // promise with 0 to signify EOF. Because we set `received_eof`,
+      // subsequent calls to `recv` will return 0 if there is no data
+      // remaining on the buffer.
+      if (evbuffer_get_length(bufferevent_get_input(bev)) > 0) {
+        size_t length =
+          bufferevent_read(
+              bev,
+              current_recv_request->data,
+              current_recv_request->size);
+        CHECK(length > 0);
+
+        current_recv_request->promise.set(length);
+      } else {
+        current_recv_request->promise.set(0);
+      }
     }
 
     if (current_send_request.get() != nullptr) {
@@ -659,11 +680,12 @@ Future<size_t> LibeventSSLSocketImpl::recv(char* data, size_t
size)
             evbuffer* input = bufferevent_get_input(self->bev);
             size_t length = evbuffer_get_length(input);
 
-            // If there is already data in the buffer, fulfill the
-            // 'recv_request' by calling 'recv_callback()'. Otherwise
-            // do nothing and wait for the 'recv_callback' to run when
-            // we receive data over the network.
-            if (length > 0) {
+            // If there is already data in the buffer or an EOF has
+            // been received, fulfill the 'recv_request' by calling
+            // 'recv_callback()'. Otherwise do nothing and wait for
+            // the 'recv_callback' to run when we receive data over
+            // the network.
+            if (length > 0 || self->received_eof) {
               self->recv_callback();
             }
           }

http://git-wip-us.apache.org/repos/asf/mesos/blob/d4185d4a/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 57eaf4f..65da091 100644
--- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
+++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
@@ -150,6 +150,11 @@ private:
   Owned<SendRequest> send_request;
   Owned<ConnectRequest> connect_request;
 
+  // Indicates whether or not an EOF has been received on this socket.
+  // Our accesses to this member are not synchronized because they all
+  // occur within the event loop, which runs on a single thread.
+  bool received_eof = false;
+
   // This is a weak pointer to 'this', i.e., ourselves, this class
   // instance. We need this for our event loop callbacks because it's
   // possible that we'll actually want to cleanup this socket impl


Mime
View raw message