[llvm][Support] ListeningSocket::accept returns operation_canceled if FD is set to -1

[cpsughrue]

If ::poll returns and FD equals -1, then ListeningSocket::shutdown has been called. So, regardless of any other information that could be gleaned from FDs.revents or PollStatus, it is appropriate to return std::errc::operation_canceled. ListeningSocket::shutdown copies FD's value to ObservedFD then sets FD to -1 before canceling ::poll by calling ::close(ObservedFD) and writing to the pipe.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Connor Sughrue (cpsughrue)

Changes

If ::poll returns and FD equals -1, then ListeningSocket::shutdown has been called. So, regardless of any other information that could be gleaned from FDs.revents or PollStatus, it is appropriate to return std::errc::operation_canceled. ListeningSocket::shutdown copies FD's value to ObservedFD then sets FD to -1 before canceling ::poll by calling ::close on ObservedFD and writing to the pipe.

---

Full diff: https://github.com/llvm/llvm-project/pull/89479.diff

1 Files Affected:

• (modified) llvm/lib/Support/raw_socket_stream.cpp (+16-11)

diff --git a/llvm/lib/Support/raw_socket_stream.cpp b/llvm/lib/Support/raw_socket_stream.cpp
index 14e2308df4d7ed..ff14b3b6e09c19 100644
--- a/llvm/lib/Support/raw_socket_stream.cpp
+++ b/llvm/lib/Support/raw_socket_stream.cpp
@@ -200,21 +200,32 @@ ListeningSocket::accept(std::chrono::milliseconds Timeout) {
   while (PollStatus == -1 && (Timeout.count() == -1 || ElapsedTime < Timeout)) {
     if (Timeout.count() != -1)
       RemainingTime -= ElapsedTime.count();
-
     auto Start = std::chrono::steady_clock::now();
+
 #ifdef _WIN32
     PollStatus = WSAPoll(FDs, 2, RemainingTime);
-    if (PollStatus == SOCKET_ERROR) {
 #else
     PollStatus = ::poll(FDs, 2, RemainingTime);
+#endif
+    // If FD equals -1 then ListeningSocket::shutdown has been called and it is
+    // appropriate to return operation_canceled. ListeningSocket::shutdown
+    // copies FD's value to ObservedFD then sets FD to -1 before canceling
+    // ::poll by calling close on ObservedFD and writing to the pipe.
+    if (FD.load() == -1)
+      return llvm::make_error<StringError>(
+          std::make_error_code(std::errc::operation_canceled),
+          "Accept canceled");
+
+#if _WIN32
+    if (PollStatus == SOCKET_ERROR) {
+#else
     if (PollStatus == -1) {
 #endif
-      // Ignore error if caused by interupting signal
       std::error_code PollErrCode = getLastSocketErrorCode();
+      // Ignore EINTR (signal occured before any request event) and retry
       if (PollErrCode != std::errc::interrupted)
         return llvm::make_error<StringError>(PollErrCode, "FD poll failed");
     }
-
     if (PollStatus == 0)
       return llvm::make_error<StringError>(
           std::make_error_code(std::errc::timed_out),
@@ -222,13 +233,7 @@ ListeningSocket::accept(std::chrono::milliseconds Timeout) {
 
     if (FDs[0].revents & POLLNVAL)
       return llvm::make_error<StringError>(
-          std::make_error_code(std::errc::bad_file_descriptor),
-          "File descriptor closed by another thread");
-
-    if (FDs[1].revents & POLLIN)
-      return llvm::make_error<StringError>(
-          std::make_error_code(std::errc::operation_canceled),
-          "Accept canceled");
+          std::make_error_code(std::errc::bad_file_descriptor));
 
     auto Stop = std::chrono::steady_clock::now();
     ElapsedTime +=

[Bigcheese]

This looks correct with some minor comments, but it would be good to figure out why the existing shutdown test already returned std::errc::operation_canceled, but the module build daemon was seeing std::errc::bad_file_descriptor on shutdown.

[cpsughrue]

This looks correct with some minor comments, but it would be good to figure out why the existing shutdown test already returned std::errc::operation_canceled, but the module build daemon was seeing std::errc::bad_file_descriptor on shutdown.

Still trying to figure out the root cause but the jist of the issue is that the below code does not fail

  llvm::handleAllErrors(std::move(Err), [&](const llvm::StringError &SE) {
    ASSERT_EQ(0,1);
  });

The raw_socket_stream FILE_DESCRIPTOR_CLOSED test should fail but wrapping a gtest macro in llvm::handleAllErrors seems to "silence" it.

EDIT: The gtest macros don't use exception but rather either return AssertionSuccess or AssertionFailure which was being ignored by the handler passed to handleAllErrors

[cpsughrue]

Another important note. The code

  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);
  });

results in two instances of MaybeServer.takeError() which is problematic and was also causing issues. Comments in Error.h explain that after calling takeError, Expected<T> is in an indeterminate state that can only be safely destructed. The hidden MaybeServer.takeError() takes place during the evaluation of ASSERT_THAT_EXPECTED.

[Bigcheese]

lgtm