Remove retry on close failure (#1271)

* remove retrying on close failing with EINTR

* give more specific debug message when close fails

* add close$NOCANCEL declaration

* fix compile error

* ignore strerror_r's return type which differs from mac to linux

* see if it will compile without __DARWIN_NON_CANCELABLE

* strerror's return value cannot be ignore by casting to (void). so just use it.

* remove redundant semicolon

* deal with unused variable warning

* refactory: extract close method into library utility

* fix include format

* fix compile error

* erase default parameter & add comments

* remove retry on closing listening socket

* remove redundant logging when closing server

Author: Nov11

CMakeLists.txt

@@ -77,6 +77,10 @@ if(UNIX OR APPLE)
     if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
         set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-tautological-undefined-compare -Wno-unused-private-field -Wno-braced-scalar-init -Wno-constant-conversion -Wno-potentially-evaluated-expression -Wno-infinite-recursion -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -I/usr/local/opt/openssl/include")
     endif()
+# ---[ close$NOCANCEL for Mac OS
+    if(APPLE)
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-dollar-in-identifier-extension")
+    endif()
 endif()
 
 # There is a problem with building on g++5.4 on Ubuntu 17.10 where

src/common/utility.cpp

@@ -0,0 +1,66 @@
+//===----------------------------------------------------------------------===//
+//
+//                         Peloton
+//
+// utility.cpp
+//
+// Identification: src/include/common/utility.h
+//
+// Copyright (c) 2015-17, Carnegie Mellon University Database Group
+//
+//===----------------------------------------------------------------------===//
+
+#include <unistd.h>
+#include <vector>
+#include <string.h>
+#include <errno.h>
+
+#include "common/logger.h"
+#include "common/utility.h"
+#if __APPLE__
+extern "C"{
+#include <sys/cdefs.h>
+int close$NOCANCEL(int);
+};
+#endif
+
+namespace peloton {
+
+int peloton_close(int fd) {
+  //On Mac OS, close$NOCANCEL guarantees that no descriptor leak & no need to retry on failure.
+  //On linux, close will do the same.
+  //In short, call close/close$NOCANCEL once and consider it done. AND NEVER RETRY ON FAILURE.
+  //The errno code is just a hint. It's logged but no further processing on it.
+  //Retry on failure may close another file descriptor that just has been assigned by OS with the same number
+  //and break assumptions of other threads.
+
+  int close_ret = -1;
+#if __APPLE__
+  close_ret = ::close$NOCANCEL(fd);
+#else
+  close_ret = close(fd);
+#endif
+
+  if (close_ret != 0) {
+    auto error_message = peloton_error_message();
+    LOG_DEBUG("Close failed on fd: %d, errno: %d [%s]", fd, errno, error_message.c_str());
+  }
+
+  return close_ret;
+}
+
+std::string peloton_error_message() {
+  std::vector<char> buffer(100, '\0');
+  int saved_errno = errno;
+  char *error_message = nullptr;
+#if __APPLE__
+  (void)strerror_r(errno, buffer.data(), buffer.size() - 1);
+  error_message = buffer.data();
+#else
+  error_message = strerror_r(saved_errno, buffer.data(), buffer.size() - 1);
+#endif
+
+  errno = saved_errno;
+  return std::string(error_message);
+}
+}

src/include/common/utility.h

@@ -0,0 +1,21 @@
+//===----------------------------------------------------------------------===//
+//
+//                         Peloton
+//
+// utility.h
+//
+// Identification: src/include/common/utility.h
+//
+// Copyright (c) 2015-16, Carnegie Mellon University Database Group
+//
+//===----------------------------------------------------------------------===//
+
+#pragma once
+#include <string>
+
+namespace peloton{
+
+  int peloton_close(int fd);
+
+  std::string peloton_error_message();
+}

src/network/connection_handle.cpp

@@ -20,6 +20,7 @@
 #include "network/protocol_handler_factory.h"
 
 #include "settings/settings_manager.h"
+#include "common/utility.h"
 
 namespace peloton {
 namespace network {
@@ -89,7 +90,7 @@ DEF_TRANSITION_GRAPH
     ON(WAKEUP) SET_STATE_TO(READ) AND_INVOKE(FillReadBuffer)
     ON(PROCEED) SET_STATE_TO(PROCESS) AND_INVOKE(Process)
     ON(NEED_DATA) SET_STATE_TO(READ) AND_WAIT
-    ON(FINISH) SET_STATE_TO(CLOSING) AND_INVOKE(CloseSocket) 
+    ON(FINISH) SET_STATE_TO(CLOSING) AND_INVOKE(CloseSocket)
   END_DEF
 
   DEFINE_STATE(PROCESS_WRITE_SSL_HANDSHAKE)
@@ -99,23 +100,23 @@ DEF_TRANSITION_GRAPH
     ON(FINISH) SET_STATE_TO(CLOSING) AND_INVOKE(CloseSocket)
     ON(PROCEED) SET_STATE_TO(PROCESS) AND_INVOKE(Process)
   END_DEF
-  
-  DEFINE_STATE(PROCESS) 
+
+  DEFINE_STATE(PROCESS)
     ON(PROCEED) SET_STATE_TO(WRITE) AND_INVOKE(ProcessWrite)
     ON(NEED_DATA) SET_STATE_TO(READ) AND_INVOKE(FillReadBuffer)
     ON(GET_RESULT) SET_STATE_TO(GET_RESULT) AND_WAIT
     ON(FINISH) SET_STATE_TO(CLOSING) AND_INVOKE(CloseSocket)
-    ON(NEED_SSL_HANDSHAKE) SET_STATE_TO(PROCESS_WRITE_SSL_HANDSHAKE) 
+    ON(NEED_SSL_HANDSHAKE) SET_STATE_TO(PROCESS_WRITE_SSL_HANDSHAKE)
       AND_INVOKE(ProcessWrite_SSLHandshake)
   END_DEF
 
-  DEFINE_STATE(WRITE) 
+  DEFINE_STATE(WRITE)
     ON(WAKEUP) SET_STATE_TO(WRITE) AND_INVOKE(ProcessWrite)
     ON(NEED_DATA) SET_STATE_TO(PROCESS) AND_INVOKE(Process)
     ON(PROCEED) SET_STATE_TO(PROCESS) AND_INVOKE(Process)
   END_DEF
 
-  DEFINE_STATE(GET_RESULT) 
+  DEFINE_STATE(GET_RESULT)
     ON(WAKEUP) SET_STATE_TO(GET_RESULT) AND_INVOKE(GetResult)
     ON(PROCEED) SET_STATE_TO(WRITE) AND_INVOKE(ProcessWrite)
   END_DEF
@@ -557,18 +558,9 @@ Transition ConnectionHandle::CloseSocket() {
     conn_SSL_context = nullptr;
   }
 
-  while (true) {
-    int status = close(sock_fd_);
-    if (status < 0) {
-      // failed close
-      if (errno == EINTR) {
-        // interrupted, try closing again
-        continue;
-      }
-    }
-    LOG_DEBUG("Already Closed the connection %d", sock_fd_);
-    return Transition::NONE;
-  }
+  peloton_close(sock_fd_);
+  return Transition::NONE;
+
 }
 
 Transition ConnectionHandle::ProcessWrite_SSLHandshake() {

src/network/peloton_server.cpp

@@ -13,6 +13,7 @@
 
 #include <fstream>
 #include <memory>
+#include "common/utility.h"
 #include "event2/thread.h"
 
 #include "common/dedicated_thread_registry.h"
@@ -276,12 +277,8 @@ void PelotonServer::ServerLoop() {
         .RegisterDedicatedThread<PelotonRpcHandlerTask>(this, rpc_task);
   }
   dispatcher_task_->EventLoop();
-  LOG_INFO("Closing server");
-  int status;
-  do {
-    status = close(listen_fd_);
-  } while (status < 0 && errno == EINTR);
-  LOG_DEBUG("Already Closed the connection %d", listen_fd_);
+
+  peloton_close(listen_fd_);
 
   LOG_INFO("Server Closed");
 }