Address feedback

  - use ::close and ::unlink instead of _close and _unlink on Windows
  - fix name of raw_socket_stream functions to specify Unix
  - use \ instead of @ in doxygen comments
  - write unittest to make sure timeout works correctly

Author: cpsughrue

llvm/include/llvm/Support/raw_socket_stream.h

@@ -25,9 +25,8 @@ namespace llvm {
 class raw_socket_stream;
 
 #ifdef _WIN32
-/// @brief Ensures proper initialization and cleanup of winsock resources
+/// \brief Ensures proper initialization and cleanup of winsock resources
 ///
-/// @details
 /// Make sure that calls to WSAStartup and WSACleanup are balanced.
 class WSABalancer {
 public:
@@ -36,27 +35,27 @@ class WSABalancer {
 };
 #endif // _WIN32
 
-/// @class ListeningSocket
-/// @brief Manages a passive (i.e., listening) UNIX domain socket
+/// \class ListeningSocket
+/// \brief Manages a passive (i.e., listening) UNIX domain socket
 ///
 /// The ListeningSocket class encapsulates a UNIX domain socket that can listen
 /// and accept incoming connections. ListeningSocket is portable and supports
 /// Windows builds begining with Insider Build 17063. ListeningSocket is
-/// designed for server-side operations, working alongside raw_socket_streams
+/// designed for server-side operations, working alongside \p raw_socket_streams
 /// that function as client connections.
 ///
 /// Usage example:
-/// @code{.cpp}
+/// \code{.cpp}
 /// std::string Path = "/path/to/socket"
 /// Expected<ListeningSocket> S = ListeningSocket::createListeningSocket(Path);
 ///
-/// if (listeningSocket) {
-///     auto connection = S->accept();
-///     if (connection) {
-///         // Use the accepted raw_socket_stream for communication.
-///     }
+/// if (S) {
+///   Expected<std::unique_ptr<raw_socket_stream>> connection = S->accept();
+///   if (connection) {
+///     // Use the accepted raw_socket_stream for communication.
+///   }
 /// }
-/// @endcode
+/// \endcode
 ///
 class ListeningSocket {
   std::atomic<int> FD;
@@ -74,15 +73,15 @@ class ListeningSocket {
   ListeningSocket &operator=(const ListeningSocket &) = delete;
 
   /// Closes the socket's FD and unlinks the socket file from the file system.
-  /// The method is idempotent
+  /// The method is thread and signal safe
   void shutdown();
 
   /// Accepts an incoming connection on the listening socket. This method can
   /// optionally either block until a connection is available or timeout after a
   /// specified amount of time has passed. By default the method will block
   /// until the socket has recieved a connection
   ///
-  /// @param Timeout An optional timeout duration in microseconds
+  /// \param Timeout An optional timeout duration in microseconds
   ///
   Expected<std::unique_ptr<raw_socket_stream>>
   accept(std::optional<std::chrono::microseconds> Timeout = std::nullopt);
@@ -91,10 +90,10 @@ class ListeningSocket {
   /// Handles the socket creation, binding, and immediately starts listening for
   /// incoming connections.
   ///
-  /// @param SocketPath The file system path where the socket will be created
-  /// @param MaxBacklog The max number of connections in a socket's backlog
+  /// \param SocketPath The file system path where the socket will be created
+  /// \param MaxBacklog The max number of connections in a socket's backlog
   ///
-  static Expected<ListeningSocket> createListeningSocket(
+  static Expected<ListeningSocket> createListeningUnixSocket(
       StringRef SocketPath,
       int MaxBacklog = llvm::hardware_concurrency().compute_thread_count());
 };
@@ -110,12 +109,11 @@ class raw_socket_stream : public raw_fd_stream {
 #endif // _WIN32
 
 public:
-  // TODO: Should probably be private
   raw_socket_stream(int SocketFD);
-  /// Create a \p raw_socket_stream connected to the Unix domain socket at \p
+  /// Create a \p raw_socket_stream connected to the UNIX domain socket at \p
   /// SocketPath.
   static Expected<std::unique_ptr<raw_socket_stream>>
-  createConnectedSocket(StringRef SocketPath);
+  createConnectedUnixSocket(StringRef SocketPath);
   ~raw_socket_stream();
 };
 

llvm/lib/Support/raw_socket_stream.cpp

@@ -58,24 +58,6 @@ static std::error_code getLastSocketErrorCode() {
 #endif
 }
 
-static void closeFD(int FD) {
-#ifdef _WIN32
-  // on windows ::close is a deprecated alias for _close
-  _close(FD);
-#else
-  ::close(FD);
-#endif
-}
-
-static void unlinkFile(StringRef Path) {
-#ifdef _WIN32
-  // on windows ::unlink is a deprecated alias for _unlink
-  _unlink(Path.str().c_str());
-#else
-  ::unlink(Path.str().c_str());
-#endif
-}
-
 static sockaddr_un setSocketAddr(StringRef SocketPath) {
   struct sockaddr_un Addr;
   memset(&Addr, 0, sizeof(Addr));
@@ -119,7 +101,8 @@ ListeningSocket::ListeningSocket(ListeningSocket &&LS)
 }
 
 Expected<ListeningSocket>
-ListeningSocket::createListeningSocket(StringRef SocketPath, int MaxBacklog) {
+ListeningSocket::createListeningUnixSocket(StringRef SocketPath,
+                                           int MaxBacklog) {
 
   // Handle instances where the target socket address already exists and
   // differentiate between a preexisting file with and without a bound socket
@@ -132,14 +115,14 @@ ListeningSocket::createListeningSocket(StringRef SocketPath, int MaxBacklog) {
     if (!MaybeFD) {
 
       // Regardless of the error, notify the caller that a file already exists
-      // at the desired socket address. The file must be removed before ::bind
-      // can use the socket address
+      // at the desired socket address and that there is no bound socket at that
+      // address. The file must be removed before ::bind can use the address
       consumeError(MaybeFD.takeError());
       return llvm::make_error<StringError>(
           std::make_error_code(std::errc::file_exists),
           "Socket address unavailable");
     }
-    closeFD(std::move(*MaybeFD));
+    ::close(std::move(*MaybeFD));
 
     // Notify caller that the provided socket address already has a bound socket
     return llvm::make_error<StringError>(
@@ -184,31 +167,28 @@ ListeningSocket::createListeningSocket(StringRef SocketPath, int MaxBacklog) {
 Expected<std::unique_ptr<raw_socket_stream>>
 ListeningSocket::accept(std::optional<std::chrono::microseconds> Timeout) {
 
-  int SelectStatus;
-  int AcceptFD;
-
+  fd_set Readfds;
+  FD_ZERO(&Readfds);
 #ifdef _WIN32
   SOCKET WinServerSock = _get_osfhandle(FD);
+  FD_SET(WinServerSock, &Readfds);
+#else
+  FD_SET(FD, &Readfds);
 #endif
 
-  fd_set Readfds;
+  int SelectStatus;
   if (Timeout.has_value()) {
     timeval TV = {0, Timeout.value().count()};
-    FD_ZERO(&Readfds);
-#ifdef _WIN32
-    FD_SET(WinServerSock, &Readfds);
-#else
-    FD_SET(FD, &Readfds);
-#endif
     SelectStatus = ::select(FD + 1, &Readfds, NULL, NULL, &TV);
   } else {
-    SelectStatus = 1;
+    SelectStatus = ::select(FD + 1, &Readfds, NULL, NULL, NULL);
   }
 
   if (SelectStatus == -1)
     return llvm::make_error<StringError>(getLastSocketErrorCode(),
-                                         "Select failed");
+                                         "select failed");
 
+  int AcceptFD;
   if (SelectStatus) {
 #ifdef _WIN32
     SOCKET WinAcceptSock = ::accept(WinServerSock, NULL, NULL);
@@ -218,20 +198,20 @@ ListeningSocket::accept(std::optional<std::chrono::microseconds> Timeout) {
 #endif
   } else
     return llvm::make_error<StringError>(
-        std::make_error_code(std::errc::timed_out), "Accept timed out");
+        std::make_error_code(std::errc::timed_out), "accept timed out");
 
   if (AcceptFD == -1)
     return llvm::make_error<StringError>(getLastSocketErrorCode(),
-                                         "Accept failed");
+                                         "accept failed");
 
   return std::make_unique<raw_socket_stream>(AcceptFD);
 }
 
 void ListeningSocket::shutdown() {
   if (FD == -1)
     return;
-  closeFD(FD);
-  unlinkFile(SocketPath);
+  ::close(FD);
+  ::unlink(SocketPath.c_str());
   FD = -1;
 }
 
@@ -245,7 +225,7 @@ raw_socket_stream::raw_socket_stream(int SocketFD)
     : raw_fd_stream(SocketFD, true) {}
 
 Expected<std::unique_ptr<raw_socket_stream>>
-raw_socket_stream::createConnectedSocket(StringRef SocketPath) {
+raw_socket_stream::createConnectedUnixSocket(StringRef SocketPath) {
 #ifdef _WIN32
   WSABalancer _;
 #endif // _WIN32

llvm/unittests/Support/raw_socket_stream_test.cpp

@@ -32,22 +32,19 @@ TEST(raw_socket_streamTest, CLIENT_TO_SERVER_AND_SERVER_TO_CLIENT) {
     GTEST_SKIP();
 
   SmallString<100> SocketPath;
-  llvm::sys::fs::createUniquePath("test_raw_socket_stream.sock", SocketPath,
-                                  true);
+  llvm::sys::fs::createUniquePath("client_server_comms.sock", SocketPath, true);
 
   // Make sure socket file does not exist. May still be there from the last test
   std::remove(SocketPath.c_str());
 
-  char Bytes[8];
-
   Expected<ListeningSocket> MaybeServerListener =
-      ListeningSocket::createListeningSocket(SocketPath);
+      ListeningSocket::createListeningUnixSocket(SocketPath);
   ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded());
 
   ListeningSocket ServerListener = std::move(*MaybeServerListener);
 
   Expected<std::unique_ptr<raw_socket_stream>> MaybeClient =
-      raw_socket_stream::createConnectedSocket(SocketPath);
+      raw_socket_stream::createConnectedUnixSocket(SocketPath);
   ASSERT_THAT_EXPECTED(MaybeClient, llvm::Succeeded());
 
   raw_socket_stream &Client = **MaybeClient;
@@ -61,11 +58,43 @@ TEST(raw_socket_streamTest, CLIENT_TO_SERVER_AND_SERVER_TO_CLIENT) {
   Client << "01234567";
   Client.flush();
 
+  char Bytes[8];
   ssize_t BytesRead = Server.read(Bytes, 8);
 
   std::string string(Bytes, 8);
 
   ASSERT_EQ(8, BytesRead);
   ASSERT_EQ("01234567", string);
 }
-} // namespace
+
+TEST(raw_socket_streamTest, TIMEOUT_PROVIDED) {
+  if (!hasUnixSocketSupport())
+    GTEST_SKIP();
+
+  SmallString<100> SocketPath;
+  llvm::sys::fs::createUniquePath("timout_provided.sock", SocketPath, true);
+
+  // Make sure socket file does not exist. May still be there from the last test
+  std::remove(SocketPath.c_str());
+
+  Expected<ListeningSocket> MaybeServerListener =
+      ListeningSocket::createListeningUnixSocket(SocketPath);
+  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded());
+  ListeningSocket ServerListener = std::move(*MaybeServerListener);
+
+  std::chrono::seconds Timeout = std::chrono::seconds(5);
+  auto Start = std::chrono::steady_clock::now();
+  Expected<std::unique_ptr<raw_socket_stream>> MaybeServer =
+      ServerListener.accept(Timeout);
+  auto End = std::chrono::steady_clock::now();
+  auto Duration = std::chrono::duration_cast<std::chrono::seconds>(End - Start);
+  ASSERT_NEAR(Duration.count(), Timeout.count(), 1);
+
+  ASSERT_THAT_EXPECTED(MaybeServer, Failed());
+  llvm::Error Err = MaybeServer.takeError();
+  llvm::handleAllErrors(std::move(Err), [&](const llvm::StringError &SE) {
+    std::error_code EC = SE.convertToErrorCode();
+    ASSERT_EQ(EC, std::errc::timed_out);
+  });
+}
+} // namespace