diff --git a/Common/Cpp/Sockets/AbstractServerSocket.h b/Common/Cpp/Sockets/AbstractServerSocket.h index a3b53c921b..74867484ab 100644 --- a/Common/Cpp/Sockets/AbstractServerSocket.h +++ b/Common/Cpp/Sockets/AbstractServerSocket.h @@ -12,6 +12,7 @@ #ifndef PokemonAutomation_Sockets_AbstractServerSocket_H #define PokemonAutomation_Sockets_AbstractServerSocket_H +#include #include #include "Common/Cpp/ListenerSet.h" @@ -93,9 +94,17 @@ class AbstractServerSocket{ // threads). virtual void close() noexcept = 0; - // Sends data to the active connection. + // Sends data to the active connection. This blocks until every byte + // has been written, the connection is lost, or "timeout" elapses. + // "std::chrono::milliseconds::max()" (the default) waits indefinitely. + // Returns the number of bytes actually written, which may be less + // than "bytes" if the deadline elapsed or the connection was lost. // If there is no active connection, returns 0. - virtual size_t send(const void* data, size_t bytes) = 0; + virtual size_t send( + const void* data, + size_t bytes, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max() + ) = 0; protected: diff --git a/Common/Cpp/Sockets/ClientSocket_Qt.cpp b/Common/Cpp/Sockets/ClientSocket_Qt.cpp index 6528026109..55ee74f958 100644 --- a/Common/Cpp/Sockets/ClientSocket_Qt.cpp +++ b/Common/Cpp/Sockets/ClientSocket_Qt.cpp @@ -106,12 +106,21 @@ size_t ClientSocket_Qt::send(const void* data, size_t bytes, std::chrono::millis emit internal_send(send_data); // Wait for the socket's event thread to finish writing the packet. - // The write loop itself stops at "deadline", so this bound only - // covers the queued call's start-up delay. The infinite default - // keeps a bounded 5 s safety: if "close()" destroys the socket - // before the queued call ever runs, it will never complete on its - // own. The shared ownership makes the timeout safe: if it expires, - // the queued call's own copy keeps the payload alive until it runs. + // A finite "timeout" waits for the deadline plus 1 s of queueing + // margin: the write loop stops at the deadline, so anything beyond + // that is just the queued call waiting to start. + // + // The infinite default cannot wait indefinitely - a limitation of + // this wrapper, not of Qt: if "close()" runs from another thread + // and the event loop is destroyed while this call is still queued + // (posted after the destruction event), Qt silently discards the + // pending event, the handler never runs, and "done" is never set; + // an unbounded wait would then hang the caller forever. A send + // queued before close() is safe: the event loop is FIFO, so it + // runs first. The 5 s bound turns that post-close race into a + // "0 bytes written" result instead of a hang. The shared + // ownership keeps the packet alive until the queued call runs + // (or is discarded with the socket). const std::chrono::milliseconds wait_time = (timeout == std::chrono::milliseconds::max()) ? std::chrono::milliseconds(5000) diff --git a/Common/Cpp/Sockets/ServerSocket_POSIX.cpp b/Common/Cpp/Sockets/ServerSocket_POSIX.cpp index 1a5a6d9666..d24d8c8f50 100644 --- a/Common/Cpp/Sockets/ServerSocket_POSIX.cpp +++ b/Common/Cpp/Sockets/ServerSocket_POSIX.cpp @@ -147,7 +147,7 @@ void ServerSocket_POSIX::close() noexcept{ } } -size_t ServerSocket_POSIX::send(const void* data, size_t bytes){ +size_t ServerSocket_POSIX::send(const void* data, size_t bytes, std::chrono::milliseconds timeout){ int socket; { std::lock_guard lg(m_lock); @@ -161,6 +161,13 @@ size_t ServerSocket_POSIX::send(const void* data, size_t bytes){ const char* ptr = (const char*)data; size_t sent = 0; + // "time_point::max()" (the infinite default) is never reached; using + // it also avoids overflowing "now() + timeout". + const std::chrono::steady_clock::time_point deadline = + (timeout == std::chrono::milliseconds::max()) + ? std::chrono::steady_clock::time_point::max() + : std::chrono::steady_clock::now() + timeout; + while (bytes > 0){ { std::lock_guard lg(m_lock); @@ -168,6 +175,11 @@ size_t ServerSocket_POSIX::send(const void* data, size_t bytes){ return sent; } } + if (std::chrono::steady_clock::now() >= deadline){ + // The deadline elapsed with bytes still unwritten; the + // connection is still fine, so just report the partial count. + return sent; + } constexpr size_t BLOCK_SIZE = 1 << 30; size_t current = std::min(BLOCK_SIZE, bytes); diff --git a/Common/Cpp/Sockets/ServerSocket_POSIX.h b/Common/Cpp/Sockets/ServerSocket_POSIX.h index 9d7965dd57..276c67f6cb 100644 --- a/Common/Cpp/Sockets/ServerSocket_POSIX.h +++ b/Common/Cpp/Sockets/ServerSocket_POSIX.h @@ -9,6 +9,7 @@ #include #include +#include #include "Common/Cpp/Concurrency/AsyncTask.h" #include "Common/Cpp/Concurrency/ConditionVariable.h" #include "Common/Cpp/Concurrency/Mutex.h" @@ -32,7 +33,11 @@ class ServerSocket_POSIX final : public AbstractServerSocket{ virtual void close_connection() override; virtual void close() noexcept override; - virtual size_t send(const void* data, size_t bytes) override; + virtual size_t send( + const void* data, + size_t bytes, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max() + ) override; private: diff --git a/Common/Cpp/Sockets/ServerSocket_Qt.cpp b/Common/Cpp/Sockets/ServerSocket_Qt.cpp index bdc53ecbe7..b29ca5f36b 100644 --- a/Common/Cpp/Sockets/ServerSocket_Qt.cpp +++ b/Common/Cpp/Sockets/ServerSocket_Qt.cpp @@ -141,7 +141,7 @@ void ServerSocket_Qt::close() noexcept{ m_close_requested = false; } -size_t ServerSocket_Qt::send(const void* data, size_t bytes){ +size_t ServerSocket_Qt::send(const void* data, size_t bytes, std::chrono::milliseconds timeout){ QTcpSocket* socket; { std::lock_guard lg(m_lock); @@ -155,19 +155,40 @@ size_t ServerSocket_Qt::send(const void* data, size_t bytes){ QSharedPointer send_data = QSharedPointer::create(); send_data->packet.assign((const char*)data, bytes); + // "time_point::max()" (the infinite default) is never reached; using + // it also avoids overflowing "now() + timeout". + send_data->deadline = + (timeout == std::chrono::milliseconds::max()) + ? std::chrono::steady_clock::time_point::max() + : std::chrono::steady_clock::now() + timeout; emit internal_send(send_data); // Wait for the server's event thread to finish writing the packet. - // The timeout covers the case where "close()" destroys the server - // before the queued call ever runs, in which case it will never - // complete on its own; the shared ownership keeps the packet alive - // until the queued call runs (or is discarded with the server). + // A finite "timeout" waits for the deadline plus 1 s of queueing + // margin: the write loop stops at the deadline, so anything beyond + // that is just the queued call waiting to start. + // + // The infinite default cannot wait indefinitely - a limitation of + // this wrapper, not of Qt: if "close()"/"close_connection()" runs + // from another thread and the event loop is destroyed while this + // call is still queued (posted after the destruction event), Qt + // silently discards the pending event, the handler never runs, and + // "done" is never set; an unbounded wait would then hang the + // caller forever. A send queued before the close is safe: the + // event loop is FIFO, so it runs first. The 5 s bound turns that + // post-close race into a "0 bytes written" result instead of a + // hang. The shared ownership keeps the packet alive until the + // queued call runs (or is discarded with the server). + const std::chrono::milliseconds wait_time = + (timeout == std::chrono::milliseconds::max()) + ? std::chrono::milliseconds(5000) + : timeout + std::chrono::milliseconds(1000); { std::unique_lock lg(send_data->lock); send_data->cv.wait_for( lg, - std::chrono::seconds(5), + wait_time, [&send_data]{ return send_data->done; } ); } @@ -323,7 +344,13 @@ void ServerSocket_Qt::on_send(QSharedPointer send_data){ if (sock != nullptr){ const char* ptr = send_data->packet.data(); size_t remaining = send_data->packet.size(); - while (remaining > 0 && sock->state() == QAbstractSocket::ConnectedState){ + // The deadline keeps a client that never reads from pinning this + // event thread forever (which would also block "close()"). + while ( + remaining > 0 + && sock->state() == QAbstractSocket::ConnectedState + && std::chrono::steady_clock::now() < send_data->deadline + ){ qint64 current_sent = sock->write(ptr, remaining); if (current_sent <= 0){ // The kernel's send buffer is full. Wait for some of it diff --git a/Common/Cpp/Sockets/ServerSocket_Qt.h b/Common/Cpp/Sockets/ServerSocket_Qt.h index c9246508fc..d532d2f80f 100644 --- a/Common/Cpp/Sockets/ServerSocket_Qt.h +++ b/Common/Cpp/Sockets/ServerSocket_Qt.h @@ -7,6 +7,7 @@ #ifndef PokemonAutomation_ServerSocket_Qt_H #define PokemonAutomation_ServerSocket_Qt_H +#include #include #include #include @@ -35,6 +36,11 @@ class ServerSocket_Qt final : public QObject, public AbstractServerSocket{ // discarded event leaves the caller's copy as the last reference. struct SendData{ std::string packet; + // The event-thread write loop stops at this deadline, so a send + // that can never complete (a client that never reads) cannot pin + // the server's event thread forever and block "close()". "max()" + // means no deadline (the infinite default). + std::chrono::steady_clock::time_point deadline = std::chrono::steady_clock::time_point::max(); size_t bytes_sent = 0; bool done = false; @@ -74,7 +80,11 @@ class ServerSocket_Qt final : public QObject, public AbstractServerSocket{ // server that is running them. virtual void close() noexcept override; - virtual size_t send(const void* data, size_t bytes) override; + virtual size_t send( + const void* data, + size_t bytes, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max() + ) override; signals: diff --git a/Common/Cpp/Sockets/ServerSocket_WinSocket.cpp b/Common/Cpp/Sockets/ServerSocket_WinSocket.cpp index 80904a9c65..7779c00122 100644 --- a/Common/Cpp/Sockets/ServerSocket_WinSocket.cpp +++ b/Common/Cpp/Sockets/ServerSocket_WinSocket.cpp @@ -157,7 +157,7 @@ void ServerSocket_WinSocket::close() noexcept{ } } -size_t ServerSocket_WinSocket::send(const void* data, size_t bytes){ +size_t ServerSocket_WinSocket::send(const void* data, size_t bytes, std::chrono::milliseconds timeout){ SOCKET socket; { std::lock_guard lg(m_lock); @@ -171,6 +171,13 @@ size_t ServerSocket_WinSocket::send(const void* data, size_t bytes){ const char* ptr = (const char*)data; size_t sent = 0; + // "time_point::max()" (the infinite default) is never reached; using + // it also avoids overflowing "now() + timeout". + const std::chrono::steady_clock::time_point deadline = + (timeout == std::chrono::milliseconds::max()) + ? std::chrono::steady_clock::time_point::max() + : std::chrono::steady_clock::now() + timeout; + while (bytes > 0){ { std::lock_guard lg(m_lock); @@ -178,6 +185,11 @@ size_t ServerSocket_WinSocket::send(const void* data, size_t bytes){ return sent; } } + if (std::chrono::steady_clock::now() >= deadline){ + // The deadline elapsed with bytes still unwritten; the + // connection is still fine, so just report the partial count. + return sent; + } int current_sent = ::send(socket, ptr, (int)bytes, 0); if (current_sent != SOCKET_ERROR){ diff --git a/Common/Cpp/Sockets/ServerSocket_WinSocket.h b/Common/Cpp/Sockets/ServerSocket_WinSocket.h index c9c4dca67e..dce3aea216 100644 --- a/Common/Cpp/Sockets/ServerSocket_WinSocket.h +++ b/Common/Cpp/Sockets/ServerSocket_WinSocket.h @@ -9,6 +9,7 @@ #include #include +#include #include #include "Common/Cpp/Concurrency/AsyncTask.h" #include "Common/Cpp/Concurrency/ConditionVariable.h" @@ -35,7 +36,11 @@ class ServerSocket_WinSocket final : public AbstractServerSocket{ virtual void close_connection() override; virtual void close() noexcept override; - virtual size_t send(const void* data, size_t bytes) override; + virtual size_t send( + const void* data, + size_t bytes, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max() + ) override; private: diff --git a/Common/Cpp/Sockets/Sockets_Tests.cpp b/Common/Cpp/Sockets/Sockets_Tests.cpp index 39b9f6f868..2d1d2b855d 100644 --- a/Common/Cpp/Sockets/Sockets_Tests.cpp +++ b/Common/Cpp/Sockets/Sockets_Tests.cpp @@ -133,6 +133,29 @@ struct ServerListener : AbstractServerSocket::Listener{ }; +// The parallel test runner can starve threads for long stretches on +// loaded CI machines, where wall-clock assertions then fail tests that +// would pass on a healthy machine. A small fixed workload (a 32 MB +// copy plus a 100 ms sleep) should take well under a second on a +// healthy core; if it takes more than 2 s, the calling thread is being +// starved, and the timing assertions of the test that called this +// cannot be trusted - that test should skip instead of fail. +bool timing_unreliable(){ + const size_t SIZE = 32u << 20; + std::vector src(SIZE, 0x5A), dst(SIZE); + const auto t0 = std::chrono::steady_clock::now(); + std::memcpy(dst.data(), src.data(), SIZE); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + return std::chrono::steady_clock::now() - t0 > std::chrono::seconds(2); +} + + +// Shared budget for the connect/listen/data waits below: long enough +// that a moderately loaded machine can make it, short enough that a +// real regression is still reported within the CI timeouts. +constexpr std::chrono::seconds SOCKET_TEST_WAIT{60}; + + template class TestSockets : public UnitTest{ public: @@ -154,7 +177,7 @@ class TestSockets : public UnitTest{ } server.start("127.0.0.1", candidate); if (!listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&listener]{ return listener.listen_finished.load() > 0; } )){ continue; @@ -175,7 +198,7 @@ class TestSockets : public UnitTest{ ) const{ client.connect("127.0.0.1", port); return listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&listener, expected_connect]{ return listener.connect_finished.load() >= expected_connect; } @@ -199,6 +222,12 @@ class Test_Sockets_Echo : public TestSockets using TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } ServerSocketType server(this->m_thread_pool); ServerListener server_listener; server_listener.server = &server; @@ -206,6 +235,14 @@ class Test_Sockets_Echo : public TestSockets uint16_t port; if (!this->start_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -221,19 +258,19 @@ class Test_Sockets_Echo : public TestSockets }else if (!client_listener.last_connect_error.empty()){ error = "The client failed to connect: " + client_listener.last_connect_error; }else if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.connected.load() >= 1; } )){ error = "The server did not call on_connect"; }else if (client.send("hello world", 11) != 11){ error = "The client did not send all 11 bytes"; }else if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.bytes_received.load() >= 11; } )){ error = "The server did not receive the data"; }else if (!client_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&client_listener]{ return client_listener.received.size() >= 11; } )){ error = "The client did not receive the echo"; @@ -247,7 +284,7 @@ class Test_Sockets_Echo : public TestSockets } if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.disconnected.load() >= 1; } )){ error = "The server did not call on_disconnect"; @@ -271,6 +308,12 @@ class Test_Sockets_ZeroLengthSend : public TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } ServerSocketType server(this->m_thread_pool); ServerListener server_listener; server_listener.server = &server; @@ -278,6 +321,14 @@ class Test_Sockets_ZeroLengthSend : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -299,12 +350,12 @@ class Test_Sockets_ZeroLengthSend : public TestSockets= 4; } )){ error = "The server did not receive the data"; }else if (!client_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&client_listener]{ return client_listener.received.size() >= 4; } )){ error = "The client did not receive the echo"; @@ -336,6 +387,12 @@ class Test_Sockets_LargeTransfer : public TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } constexpr size_t SIZE = 1 << 20; std::vector payload(SIZE); for (size_t i = 0; i < SIZE; i++){ @@ -349,6 +406,14 @@ class Test_Sockets_LargeTransfer : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -362,7 +427,7 @@ class Test_Sockets_LargeTransfer : public TestSocketsconnect_client(client, client_listener, port, 1)){ error = "The client did not finish connecting"; }else if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.connected.load() >= 1; } )){ error = "The server did not call on_connect"; @@ -405,6 +470,12 @@ class Test_Sockets_RapidReconnect : public TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } constexpr int CYCLES = 50; ServerSocketType server(this->m_thread_pool); @@ -414,6 +485,14 @@ class Test_Sockets_RapidReconnect : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -437,7 +516,7 @@ class Test_Sockets_RapidReconnect : public TestSockets= cycle_no; } )){ error = "The server did not call on_connect (cycle " + std::to_string(cycle_no) + ")"; @@ -451,7 +530,7 @@ class Test_Sockets_RapidReconnect : public TestSockets= expected_echo; } )){ error = "The client did not receive the echo (cycle " + std::to_string(cycle_no) + ")"; @@ -460,7 +539,7 @@ class Test_Sockets_RapidReconnect : public TestSockets= cycle_no; } )){ error = "The server did not call on_disconnect (cycle " + std::to_string(cycle_no) + ")"; @@ -494,6 +573,12 @@ class Test_Sockets_SendCloseRace : public TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } constexpr size_t SIZE = 1 << 20; std::vector payload(SIZE, 0xAB); @@ -504,6 +589,14 @@ class Test_Sockets_SendCloseRace : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -515,11 +608,25 @@ class Test_Sockets_SendCloseRace : public TestSocketsconnect_client(client, client_listener, port, 1)){ + // A connect-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The client did not finish connecting on a starved machine." + ); + } error = "The client did not finish connecting"; }else if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.connected.load() >= 1; } )){ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not call on_connect on a starved machine." + ); + } error = "The server did not call on_connect"; }else{ std::atomic bytes_sent{0}; @@ -534,10 +641,27 @@ class Test_Sockets_SendCloseRace : public TestSockets= 1; } )){ error = "The client did not observe the disconnect"; @@ -550,7 +674,7 @@ class Test_Sockets_SendCloseRace : public TestSockets= 2; } )){ error = "The server did not accept the new connection"; @@ -583,6 +707,12 @@ class Test_Sockets_RejectSecondClient : public TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } ServerSocketType server(this->m_thread_pool); ServerListener server_listener; server_listener.server = &server; @@ -590,6 +720,14 @@ class Test_Sockets_RejectSecondClient : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -607,14 +745,14 @@ class Test_Sockets_RejectSecondClient : public TestSocketsconnect_client(client_a, client_a_listener, port, 1)){ error = "Client A did not finish connecting"; }else if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.connected.load() >= 1; } )){ error = "The server did not call on_connect"; }else{ client_b.connect("127.0.0.1", port); if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.rejected.load() >= 1; } )){ error = "The server did not reject the second client"; @@ -623,7 +761,7 @@ class Test_Sockets_RejectSecondClient : public TestSockets= 11; } )){ error = "Client A did not receive the echo"; @@ -633,7 +771,7 @@ class Test_Sockets_RejectSecondClient : public TestSockets= 1; } )){ error = "Client B did not finish its connect attempt"; @@ -666,6 +804,12 @@ class Test_Sockets_PortInUse : public TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } ServerSocketType server(this->m_thread_pool); ServerListener server_listener; server_listener.server = &server; @@ -673,6 +817,14 @@ class Test_Sockets_PortInUse : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -681,7 +833,7 @@ class Test_Sockets_PortInUse : public TestSockets 0; } )){ server.close(); @@ -710,7 +862,7 @@ class Test_Sockets_PortInUse : public TestSockets= 2; } )){ ok = false; @@ -749,6 +901,12 @@ class Test_Sockets_Qt_CompletionLifetime : public UnitTest{ } virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } // Blocks the event thread while it receives data. struct StallListener : AbstractServerSocket::Listener{ virtual void on_receive_data(const void*, size_t) override{ @@ -850,10 +1008,10 @@ class Test_Sockets_Qt_CompletionLifetime : public UnitTest{ // the in-flight write stops at the deadline, releasing the socket's // event thread, which is what close() has to run on). // -// The stall lasts 12 s - longer than the 10 s send threshold below - so -// the peer is still stalled for the whole duration of every send in the -// retry loop. It is interruptible ("stop"), so shutting the peer down -// does not wait for it. +// The stall lasts 70 s - longer than the 60 s send threshold below - +// so the peer is still stalled for the whole duration of every send in +// the retry loop. It is interruptible ("stop"), so shutting the peer +// down does not wait for it. // // An OS with unusually large autotuned socket buffers (observed on // Windows CI, where the kernel buffered the entire 256 MB payload @@ -868,12 +1026,23 @@ class Test_Sockets_Qt_CompletionLifetime : public UnitTest{ // the Qt write loop and the send legitimately "completes" there; a // retry cannot exercise the deadline on that backend. On that backend // only the timing and close() behavior are asserted. +// +// Timing tolerance: if the machine is too starved for wall-clock +// assertions to be trusted (timing_unreliable()), the test skips +// instead of failing - at start, at each threshold crossing, and on +// connect/listen-phase failures. template class Test_Sockets_ClientSendTimeout : public TestSockets{ public: using TestSockets::TestSockets; virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } constexpr bool QT = std::is_same_v; // Stalls the server's reading for 12 s on the first data (or @@ -883,9 +1052,13 @@ class Test_Sockets_ClientSendTimeout : public TestSockets stalled{false}; std::atomic stop{false}; + ~StallingServerListener(){ + stop.store(true, std::memory_order_release); + } + virtual void on_receive_data(const void* data, size_t bytes) override{ if (!stalled.exchange(true)){ - const auto end = std::chrono::steady_clock::now() + std::chrono::seconds(12); + const auto end = std::chrono::steady_clock::now() + std::chrono::seconds(70); while (std::chrono::steady_clock::now() < end && !stop.load(std::memory_order_acquire)){ std::this_thread::sleep_for(std::chrono::milliseconds(10)); @@ -901,6 +1074,14 @@ class Test_Sockets_ClientSendTimeout : public TestSocketsstart_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } return "Failed to start the server"; } @@ -912,11 +1093,25 @@ class Test_Sockets_ClientSendTimeout : public TestSocketsconnect_client(client, client_listener, port, 1)){ + // A connect-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The client did not finish connecting on a starved machine." + ); + } error = "The client did not finish connecting"; }else if (!server_listener.waiter.wait_for( - std::chrono::seconds(5), + SOCKET_TEST_WAIT, [&server_listener]{ return server_listener.connected.load() >= 1; } )){ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not call on_connect on a starved machine." + ); + } error = "The server did not call on_connect"; }else{ // Far larger than the kernel send/receive buffering that @@ -932,8 +1127,8 @@ class Test_Sockets_ClientSendTimeout : public TestSockets 10000){ - error = "send() did not return within 10 s of its 100 ms deadline (" + std::to_string(send_ms) + " ms)"; + if (send_ms > 60000){ + // A slow send on a starved machine is the + // machine's fault, not the deadline's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "send() took " + std::to_string(send_ms) + " ms on a starved machine." + ); + } + error = "send() did not return within 60 s of its 100 ms deadline (" + std::to_string(send_ms) + " ms)"; break; }else if (sent < payload.size()){ // The deadline engaged with bytes left unwritten. @@ -971,12 +1174,33 @@ class Test_Sockets_ClientSendTimeout : public TestSockets( + std::chrono::steady_clock::now() - z0 + ).count(); + auto c0 = std::chrono::steady_clock::now(); client.close(); auto close_ms = std::chrono::duration_cast( std::chrono::steady_clock::now() - c0 ).count(); - if (close_ms > 5000){ + if (close_ms > 60000){ + if (control_ms > 2000 || timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "close() took " + std::to_string(close_ms) + " ms on a starved machine." + ); + } error = "close() did not finish after the timed-out send (" + std::to_string(close_ms) + " ms)"; }else if (client.state() != AbstractClientSocket::State::NOT_RUNNING){ error = "The client is not NOT_RUNNING after close()"; @@ -1004,6 +1228,255 @@ class Test_Sockets_ClientSendTimeout : public TestSockets +class Test_Sockets_ServerSendTimeout : public TestSockets{ +public: + using TestSockets::TestSockets; + + virtual UnitTestResult run(Logger& logger, CancellableScope& scope) const override{ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "Machine too starved for a timing-sensitive socket test (timing_unreliable())." + ); + } + constexpr bool QT = std::is_same_v; + + // Stalls the client's reading for 12 s on the first data (or + // until "stop" is set), keeping the server's send buffer full + // for the duration of the send test. + struct StallingClientListener : ClientListener{ + std::atomic stalled{false}; + std::atomic stop{false}; + + ~StallingClientListener(){ + stop.store(true, std::memory_order_release); + } + + virtual void on_receive_data(const void* data, size_t bytes) override{ + if (!stalled.exchange(true)){ + const auto end = std::chrono::steady_clock::now() + std::chrono::seconds(70); + while (std::chrono::steady_clock::now() < end + && !stop.load(std::memory_order_acquire)){ + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + } + ClientListener::on_receive_data(data, bytes); + } + }; + + ServerSocketType server(this->m_thread_pool); + ServerListener server_listener; + server.add_listener(server_listener); + + uint16_t port; + if (!this->start_server(server, server_listener, port)){ + // A listen-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not start listening on a starved machine." + ); + } + return "Failed to start the server"; + } + + bool ok = false; + std::string error; + { + ClientSocketType client(this->m_thread_pool); + StallingClientListener client_listener; + client.add_listener(client_listener); + + if (!this->connect_client(client, client_listener, port, 1)){ + // A connect-phase failure on a starved machine is the + // machine's fault, not the socket's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The client did not finish connecting on a starved machine." + ); + } + error = "The client did not finish connecting"; + }else if (!server_listener.waiter.wait_for( + SOCKET_TEST_WAIT, + [&server_listener]{ return server_listener.connected.load() >= 1; } + )){ + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server did not call on_connect on a starved machine." + ); + } + error = "The server did not call on_connect"; + }else{ + // Far larger than the kernel send/receive buffering that + // the send loop can rely on being absent, so the send + // must stop on the stalled reader (see the comment on + // the class for the large-buffer OS case). + constexpr size_t SIZE = 1 << 28; + std::vector payload(SIZE, 0xAB); + + // The deadline is 100 ms, but this test also allocates + // and moves 256 MB (the Qt backend additionally copies it + // into its packet buffer), and on a loaded CI runner that + // page traffic - or the calling thread being descheduled + // - can take seconds. The pre-fix behavior was an infinite + // hang, so the threshold only has to prove boundedness; + // 60 s keeps the test meaningful while tolerating + // extremely loaded CI runners. + int completed = 0; + while (true){ + auto t0 = std::chrono::steady_clock::now(); + size_t sent = server.send( + payload.data(), + payload.size(), + std::chrono::milliseconds(100) + ); + auto send_ms = std::chrono::duration_cast( + std::chrono::steady_clock::now() - t0 + ).count(); + + if (send_ms > 60000){ + // A slow send on a starved machine is the + // machine's fault, not the deadline's. + if (timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "send() took " + std::to_string(send_ms) + " ms on a starved machine." + ); + } + error = "send() did not return within 60 s of its 100 ms deadline (" + std::to_string(send_ms) + " ms)"; + break; + }else if (sent < payload.size()){ + // The deadline engaged with bytes left unwritten. + break; + }else if (QT){ + // Qt's "completion" is its userspace buffer, not + // kernel delivery; a retry cannot exercise the + // deadline on that backend. + break; + } + + // The whole payload was written, so the kernel's + // socket buffers absorbed it (unusually large + // autotuned buffers). They are now full, so the + // next send to the same stalled peer cannot + // complete. + if (++completed >= 4){ + error = "All 4 sends of 256 MB completed: the OS buffered over 1 GiB against the stalled peer, so the deadline cannot be verified"; + break; + } + } + + if (error.empty()){ + // A zero-length send round-trips a queued call through + // the server's event thread (on the Qt backend, the + // thread that close_connection() has to wait on). The + // infinite default is the canary: it returns in ~0 ms + // on a healthy event thread, and for the full 5 s + // internal safety wait if that thread is starved. + // (A finite timeout cannot be used: the caller-side + // wait is capped at "timeout + 1 s", so the control + // could never expose a starved event thread.) + const auto z0 = std::chrono::steady_clock::now(); + server.send("", 0); + const long long control_ms = std::chrono::duration_cast( + std::chrono::steady_clock::now() - z0 + ).count(); + + auto c0 = std::chrono::steady_clock::now(); + server.close_connection(); + auto close_ms = std::chrono::duration_cast( + std::chrono::steady_clock::now() - c0 + ).count(); + if (close_ms > 60000){ + if (control_ms > 2000 || timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "close_connection() took " + std::to_string(close_ms) + " ms on a starved machine." + ); + } + error = "close_connection() did not finish after the timed-out send (" + std::to_string(close_ms) + " ms)"; + }else{ + // close_connection() queues the close onto the + // server's event thread and stops waiting after + // its internal 5 s bound, so on a starved event + // thread the close may still be pending when it + // returns. The queued close does run once the + // event thread gets to it, so poll for the + // resulting LISTENING state within the shared + // budget instead of failing on a not-yet-applied + // close. + const auto s0 = std::chrono::steady_clock::now(); + while (server.state() != AbstractServerSocket::State::LISTENING + && std::chrono::steady_clock::now() - s0 < SOCKET_TEST_WAIT){ + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + if (server.state() != AbstractServerSocket::State::LISTENING){ + if (control_ms > 2000 || timing_unreliable()){ + return UnitTestResult( + UnitTestResult::SKIPPED, + "The server is not LISTENING after close_connection() on a starved machine." + ); + } + error = "The server is not LISTENING after close_connection()"; + }else{ + ok = true; + } + } + } + } + + // Release the stalled reader so the client's close() (which + // joins its receive thread, currently inside the stall) does + // not wait for it. + client_listener.stop.store(true, std::memory_order_release); + client.close(); + } + + server.close(); + + if (!ok && !error.empty()){ + return error; + } + return true; + } +}; + + + template void add_socket_tests(UnitTestDatabase& database, ThreadPool& thread_pool, const char* backend_name){ const std::string prefix = "Sockets (" + std::string(backend_name) + ") - "; @@ -1032,6 +1505,9 @@ void add_socket_tests(UnitTestDatabase& database, ThreadPool& thread_pool, const database.add>( thread_pool, prefix + "Client send() timeout against a stalled peer" ); + database.add>( + thread_pool, prefix + "Server send() timeout against a stalled peer" + ); }