From 97ad0fc552e5b9b02c9f9e3d7bd46cddc08f5288 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 29 Aug 2025 15:06:52 +0200 Subject: [PATCH 1/3] Make HTTP livness timout configurable for unittests It's annoying to have to wait 10 seconds for the `liveness_disconnect` test to complete, so make the timeout configurable and set it to a way lower value to test the functionality. --- lib/remote/httpserverconnection.cpp | 12 ++++++++++-- lib/remote/httpserverconnection.hpp | 12 ++++++++++++ test/remote-httpserverconnection.cpp | 7 ++++--- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/remote/httpserverconnection.cpp b/lib/remote/httpserverconnection.cpp index d8befd211..fde33fdcc 100644 --- a/lib/remote/httpserverconnection.cpp +++ b/lib/remote/httpserverconnection.cpp @@ -152,6 +152,11 @@ bool HttpServerConnection::Disconnected() return m_ShuttingDown; } +void HttpServerConnection::SetLivenessTimeout(double seconds) +{ + m_LivenessTimeout = seconds; +} + static inline bool EnsureValidHeaders( boost::beast::flat_buffer& buf, @@ -542,14 +547,17 @@ void HttpServerConnection::CheckLiveness(boost::asio::yield_context yc) boost::system::error_code ec; for (;;) { - m_CheckLivenessTimer.expires_from_now(boost::posix_time::seconds(5)); + // Wait for the half of the liveness timeout to give the connection some leeway to do other work. + // But never wait longer than 5 seconds to ensure timely shutdowns. + auto sleepTime = std::min(5ll*1000, static_cast(m_LivenessTimeout * 500)); + m_CheckLivenessTimer.expires_from_now(boost::posix_time::milliseconds(sleepTime)); m_CheckLivenessTimer.async_wait(yc[ec]); if (m_ShuttingDown) { break; } - if (m_Seen < Utility::GetTime() - 10) { + if (m_Seen < Utility::GetTime() - m_LivenessTimeout) { Log(LogInformation, "HttpServerConnection") << "No messages for HTTP connection have been received in the last 10 seconds."; diff --git a/lib/remote/httpserverconnection.hpp b/lib/remote/httpserverconnection.hpp index 1f3d5d7f9..63f1ad6f7 100644 --- a/lib/remote/httpserverconnection.hpp +++ b/lib/remote/httpserverconnection.hpp @@ -33,11 +33,23 @@ public: void StartDetectClientSideShutdown(); bool Disconnected(); + /** + * Sets the liveness timeout in seconds. + * + * If we don't receive any data from the client in this time frame, we consider the connection + * dead and close it. The default is 10 seconds. This function should only be used for unit tests + * to speed them up. + * + * @param seconds The timeout in seconds. + */ + void SetLivenessTimeout(double seconds); + private: WaitGroup::Ptr m_WaitGroup; ApiUser::Ptr m_ApiUser; Shared::Ptr m_Stream; double m_Seen; + double m_LivenessTimeout{10.0}; // The liveness timeout in seconds. @see SetLivenessTimeout() for details. String m_PeerAddress; boost::asio::io_context::strand m_IoStrand; bool m_ShuttingDown; diff --git a/test/remote-httpserverconnection.cpp b/test/remote-httpserverconnection.cpp index f0b45125d..7596ae325 100644 --- a/test/remote-httpserverconnection.cpp +++ b/test/remote-httpserverconnection.cpp @@ -45,10 +45,11 @@ struct HttpServerConnectionFixture : TlsStreamFixture, ConfigurationCacheDirFixt user->Register(); } - void SetupHttpServerConnection(bool authenticated) + void SetupHttpServerConnection(bool authenticated, double livenessTimeout = 10.0) { String identity = authenticated ? "client" : "invalid"; m_Connection = new HttpServerConnection(m_WaitGroup, identity, authenticated, server); + m_Connection->SetLivenessTimeout(livenessTimeout); m_Connection->Start(); } @@ -552,9 +553,9 @@ BOOST_AUTO_TEST_CASE(handler_throw_streaming) BOOST_AUTO_TEST_CASE(liveness_disconnect) { - SetupHttpServerConnection(false); + SetupHttpServerConnection(false, .300); // 300ms liveness timeout is more than enough! - BOOST_REQUIRE(AssertServerDisconnected(std::chrono::seconds(11))); + BOOST_REQUIRE(AssertServerDisconnected(std::chrono::milliseconds(450))); // Give some leeway to Asio's timers BOOST_REQUIRE(ExpectLogPattern("HTTP client disconnected .*")); BOOST_REQUIRE(ExpectLogPattern("No messages for HTTP connection have been received in the last 10 seconds.")); BOOST_REQUIRE(Shutdown(client)); From a2b44c0fbb3fe9a3e87d18adc43884bbf9e62155 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 29 Aug 2025 16:06:07 +0200 Subject: [PATCH 2/3] tests: speed up timer tests using smaller sleep times --- test/base-timer.cpp | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/test/base-timer.cpp b/test/base-timer.cpp index 696a474b4..f0fffb553 100644 --- a/test/base-timer.cpp +++ b/test/base-timer.cpp @@ -22,40 +22,40 @@ BOOST_AUTO_TEST_CASE(interval) BOOST_CHECK(timer->GetInterval() == 1.5); } -int counter = 0; - -static void Callback(const Timer * const&) -{ - counter++; -} - BOOST_AUTO_TEST_CASE(invoke) { - Timer::Ptr timer = Timer::Create(); - timer->OnTimerExpired.connect(&Callback); - timer->SetInterval(1); + int counter = 0; + + Timer::Ptr timer = Timer::Create(); + timer->OnTimerExpired.connect([&counter](const Timer* const&) { counter++; }); + timer->SetInterval(.1); - counter = 0; timer->Start(); - Utility::Sleep(5.5); + Utility::Sleep(.55); timer->Stop(); - BOOST_CHECK(counter >= 4 && counter <= 6); + // At this point, the timer should have fired exactly 5 times (0.5 / 0.1) and the sixth time + // should not have fired yet as we stopped the timer after 0.55 seconds (0.6 would be needed). + BOOST_CHECK_EQUAL(5, counter); } BOOST_AUTO_TEST_CASE(scope) { + int counter = 0; + Timer::Ptr timer = Timer::Create(); - timer->OnTimerExpired.connect(&Callback); - timer->SetInterval(1); + timer->OnTimerExpired.connect([&counter](const Timer* const&) { counter++; }); + timer->SetInterval(.1); - counter = 0; timer->Start(); - Utility::Sleep(5.5); + Utility::Sleep(.55); timer.reset(); - Utility::Sleep(5.5); + Utility::Sleep(.1); - BOOST_CHECK(counter >= 4 && counter <= 6); + // At this point, the timer should have fired exactly 5 times (0.5 / 0.1) and the sixth time + // should not have fired yet as we destroyed the timer after 0.55 seconds (0.6 would be needed), + // and even if we wait another 0.1 seconds after its destruction, it should not fire again. + BOOST_CHECK_EQUAL(5, counter); } BOOST_AUTO_TEST_SUITE_END() From 5f862ce3bbb0963af568208b0ac9e7a3045a0ec8 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Wed, 3 Sep 2025 16:50:12 +0200 Subject: [PATCH 3/3] HttpServerConnection: use `std::chrono` for `m_Seen` --- lib/remote/httpserverconnection.cpp | 23 ++++++++++++----------- lib/remote/httpserverconnection.hpp | 13 ++++++++----- test/remote-httpserverconnection.cpp | 6 +++--- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/lib/remote/httpserverconnection.cpp b/lib/remote/httpserverconnection.cpp index fde33fdcc..39fa2d79d 100644 --- a/lib/remote/httpserverconnection.cpp +++ b/lib/remote/httpserverconnection.cpp @@ -18,7 +18,6 @@ #include "base/timer.hpp" #include "base/tlsstream.hpp" #include "base/utility.hpp" -#include #include #include #include @@ -41,7 +40,7 @@ HttpServerConnection::HttpServerConnection(const WaitGroup::Ptr& waitGroup, cons } HttpServerConnection::HttpServerConnection(const WaitGroup::Ptr& waitGroup, const String& identity, bool authenticated, const Shared::Ptr& stream, boost::asio::io_context& io) - : m_WaitGroup(waitGroup), m_Stream(stream), m_Seen(Utility::GetTime()), m_IoStrand(io), m_ShuttingDown(false), m_ConnectionReusable(true), m_CheckLivenessTimer(io) + : m_WaitGroup(waitGroup), m_Stream(stream), m_IoStrand(io), m_ShuttingDown(false), m_ConnectionReusable(true), m_CheckLivenessTimer(io) { if (authenticated) { m_ApiUser = ApiUser::GetByClientCN(identity); @@ -152,9 +151,9 @@ bool HttpServerConnection::Disconnected() return m_ShuttingDown; } -void HttpServerConnection::SetLivenessTimeout(double seconds) +void HttpServerConnection::SetLivenessTimeout(std::chrono::milliseconds timeout) { - m_LivenessTimeout = seconds; + m_LivenessTimeout = timeout; } static inline @@ -458,7 +457,7 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc) beast::flat_buffer buf; while (m_WaitGroup->IsLockable()) { - m_Seen = Utility::GetTime(); + m_Seen = ch::steady_clock::now(); HttpRequest request(m_Stream); HttpResponse response(m_Stream, this); @@ -472,7 +471,7 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc) break; } - m_Seen = Utility::GetTime(); + m_Seen = ch::steady_clock::now(); auto start (ch::steady_clock::now()); { @@ -524,7 +523,7 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc) break; } - m_Seen = std::numeric_limits::max(); + m_Seen = ch::steady_clock::time_point::max(); ProcessRequest(request, response, m_WaitGroup, cpuBoundWorkTime, yc); @@ -549,17 +548,19 @@ void HttpServerConnection::CheckLiveness(boost::asio::yield_context yc) for (;;) { // Wait for the half of the liveness timeout to give the connection some leeway to do other work. // But never wait longer than 5 seconds to ensure timely shutdowns. - auto sleepTime = std::min(5ll*1000, static_cast(m_LivenessTimeout * 500)); - m_CheckLivenessTimer.expires_from_now(boost::posix_time::milliseconds(sleepTime)); + auto sleepTime = std::min(5000ms, m_LivenessTimeout / 2); + m_CheckLivenessTimer.expires_from_now(boost::posix_time::milliseconds(sleepTime.count())); m_CheckLivenessTimer.async_wait(yc[ec]); if (m_ShuttingDown) { break; } - if (m_Seen < Utility::GetTime() - m_LivenessTimeout) { + if (m_LivenessTimeout < std::chrono::steady_clock::now() - m_Seen) { Log(LogInformation, "HttpServerConnection") - << "No messages for HTTP connection have been received in the last 10 seconds."; + << "No messages for HTTP connection have been received in the last " + << std::chrono::duration_cast(m_LivenessTimeout).count() + << " seconds, disconnecting (from " << m_PeerAddress << ")."; Disconnect(yc); break; diff --git a/lib/remote/httpserverconnection.hpp b/lib/remote/httpserverconnection.hpp index 63f1ad6f7..4436897f1 100644 --- a/lib/remote/httpserverconnection.hpp +++ b/lib/remote/httpserverconnection.hpp @@ -12,10 +12,13 @@ #include #include #include +#include namespace icinga { +using namespace std::chrono_literals; + /** * An API client connection. * @@ -34,22 +37,22 @@ public: bool Disconnected(); /** - * Sets the liveness timeout in seconds. + * Sets the liveness timeout. * * If we don't receive any data from the client in this time frame, we consider the connection * dead and close it. The default is 10 seconds. This function should only be used for unit tests * to speed them up. * - * @param seconds The timeout in seconds. + * @param timeout The timeout duration. */ - void SetLivenessTimeout(double seconds); + void SetLivenessTimeout(std::chrono::milliseconds timeout); private: WaitGroup::Ptr m_WaitGroup; ApiUser::Ptr m_ApiUser; Shared::Ptr m_Stream; - double m_Seen; - double m_LivenessTimeout{10.0}; // The liveness timeout in seconds. @see SetLivenessTimeout() for details. + std::chrono::steady_clock::time_point m_Seen{std::chrono::steady_clock::now()}; + std::chrono::milliseconds m_LivenessTimeout{10s}; String m_PeerAddress; boost::asio::io_context::strand m_IoStrand; bool m_ShuttingDown; diff --git a/test/remote-httpserverconnection.cpp b/test/remote-httpserverconnection.cpp index 7596ae325..ac0f895b8 100644 --- a/test/remote-httpserverconnection.cpp +++ b/test/remote-httpserverconnection.cpp @@ -45,7 +45,7 @@ struct HttpServerConnectionFixture : TlsStreamFixture, ConfigurationCacheDirFixt user->Register(); } - void SetupHttpServerConnection(bool authenticated, double livenessTimeout = 10.0) + void SetupHttpServerConnection(bool authenticated, std::chrono::milliseconds livenessTimeout = std::chrono::milliseconds(10000)) { String identity = authenticated ? "client" : "invalid"; m_Connection = new HttpServerConnection(m_WaitGroup, identity, authenticated, server); @@ -553,11 +553,11 @@ BOOST_AUTO_TEST_CASE(handler_throw_streaming) BOOST_AUTO_TEST_CASE(liveness_disconnect) { - SetupHttpServerConnection(false, .300); // 300ms liveness timeout is more than enough! + SetupHttpServerConnection(false, std::chrono::milliseconds(300)); // 300ms liveness timeout is more than enough! BOOST_REQUIRE(AssertServerDisconnected(std::chrono::milliseconds(450))); // Give some leeway to Asio's timers BOOST_REQUIRE(ExpectLogPattern("HTTP client disconnected .*")); - BOOST_REQUIRE(ExpectLogPattern("No messages for HTTP connection have been received in the last 10 seconds.")); + BOOST_REQUIRE(ExpectLogPattern("No messages for HTTP connection have been received in the last \\d+ seconds, disconnecting .*")); BOOST_REQUIRE(Shutdown(client)); }