fix(socket): a signal during the handshake is not the deadline

wait_before_recv took any wait that ended without a readable socket for the
deadline. Without a stop token that wait is one poll, and a signal ends poll
with EINTR even when its handler was installed with SA_RESTART, which restarts
the blocking recv the wait stands in front of. A host with such a handler
(a profiler's SIGPROF, a timer) saw handshakes fail as "TLS handshake timed
out" long before the time was up; on master the recv restarted and the
handshake went on.

A wait that ends early is now waited again for the time that is left, and the
deadline is hit only when the clock has passed it. The new case signals the
handshaking thread every 100 ms under a 1000 ms timeout; before this change
it ended after 100 ms. The move operations carry deadline_hit_ with deadline_.

Author: Sunrisepeak

src/socket.cppm

@@ -52,7 +52,8 @@ public:
 
     // Move constructor
     Socket(Socket&& other) noexcept
-        : fd_(other.fd_), stop_(std::move(other.stop_)), deadline_(other.deadline_) {
+        : fd_(other.fd_), stop_(std::move(other.stop_)), deadline_(other.deadline_)
+        , deadline_hit_(other.deadline_hit_) {
         other.fd_ = INVALID_SOCKET_FD;
     }
 
@@ -63,6 +64,7 @@ public:
             fd_ = other.fd_;
             stop_ = std::move(other.stop_);
             deadline_ = other.deadline_;
+            deadline_hit_ = other.deadline_hit_;
             other.fd_ = INVALID_SOCKET_FD;
         }
         return *this;
@@ -91,17 +93,27 @@ public:
     // What `bio_recv` calls ahead of a `recv` that would otherwise block. True
     // when a `recv` now will not block. With neither a token nor a deadline it
     // answers true without waiting, so the plain `recv` is as it was.
+    //
+    // A wait that ends before the deadline is waited again for what is left: a
+    // signal ends `poll` with EINTR even under SA_RESTART, which the blocking
+    // `recv` this wait stands in front of would have restarted. The deadline is
+    // hit only when the clock says so.
     bool wait_before_recv() {
         if (!stop_.stop_possible() && !deadline_) return true;
-        int wait = -1;
-        if (deadline_) {
-            const auto left = std::chrono::ceil<std::chrono::milliseconds>(
-                *deadline_ - std::chrono::steady_clock::now()).count();
-            wait = left > 0 ? static_cast<int>(left) : 0;
+        for (;;) {
+            int wait = -1;
+            if (deadline_) {
+                const auto left = std::chrono::ceil<std::chrono::milliseconds>(
+                    *deadline_ - std::chrono::steady_clock::now()).count();
+                wait = left > 0 ? static_cast<int>(left) : 0;
+            }
+            if (wait_readable(wait)) return true;
+            if (stop_.stop_requested() || !deadline_) return false;
+            if (std::chrono::steady_clock::now() >= *deadline_) {
+                deadline_hit_ = true;
+                return false;
+            }
         }
-        if (wait_readable(wait)) return true;
-        if (deadline_ && !stop_.stop_requested()) deadline_hit_ = true;
-        return false;
     }
 
     bool connect(const char* host, int port, int timeoutMs) {

tests/test_handshake_timeout.cpp

@@ -15,6 +15,10 @@ import std;
 #include <gtest/gtest.h>
 #include "proxy_test_server.hpp"
 #include "tls_test_server.hpp"
+#ifndef _WIN32
+#include <pthread.h>
+#include <signal.h>
+#endif
 
 import mcpplibs.tinyhttps;
 #ifndef _LIBCPP_VERSION
@@ -215,6 +219,50 @@ TEST_F(HandshakeTimeoutTest, ABytePerReadDoesNotHoldTheHandshakeOpen) {
     EXPECT_LT(since_ms(start), kPromptMs);
 }
 
+// A signal whose handler was installed with SA_RESTART restarts a blocking
+// `recv`, so it never ended a handshake before the deadline existed. The wait
+// that the deadline puts ahead of each `recv` is a `poll`, which a signal ends
+// with EINTR whatever the flag; that has to be waited out, not taken for the
+// deadline. The signals go to this thread, the one in the handshake.
+#ifndef _WIN32
+namespace {
+void ignore_signal(int) {}
+}
+
+TEST_F(HandshakeTimeoutTest, ASignalDuringTheHandshakeIsNotATimeout) {
+    Silent peer;
+    ASSERT_FALSE(peer.failed());
+
+    struct sigaction action {}, previous {};
+    action.sa_handler = ignore_signal;
+    action.sa_flags = SA_RESTART;
+    sigemptyset(&action.sa_mask);
+    ASSERT_EQ(sigaction(SIGUSR1, &action, &previous), 0);
+
+    auto cfg = test_config();
+    cfg.connectTimeoutMs = 1000;
+    https::HttpClient client(cfg);
+    const pthread_t self = pthread_self();
+    std::atomic<bool> done { false };
+    std::thread signaller([&] {
+        for (int i = 0; i < 5 && !done.load(); ++i) {
+            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+            if (!done.load()) pthread_kill(self, SIGUSR1);
+        }
+    });
+    const auto start = Clock::now();
+    auto res = client.send(get(peer.url()));
+    const auto took = since_ms(start);
+    done = true;
+    signaller.join();
+    sigaction(SIGUSR1, &previous, nullptr);
+
+    EXPECT_EQ(res.statusText, "TLS handshake timed out");
+    EXPECT_GE(took, 900) << "the handshake ended at a signal, not at the deadline";
+    EXPECT_LT(took, kPromptMs);
+}
+#endif
+
 // connectTimeoutMs is for establishing the connection. Once the handshake is
 // done a read waits as long as it always did: here 1600 ms of silence after a
 // 1000 ms timeout, and then the bytes arrive. The timeout leaves room for a