Merge pull request #10550 from Icinga/fix-long-running-tests

Fix annoying long-running tests

lib/remote/httpserverconnection.cpp

@@ -18,7 +18,6 @@
 #include "base/timer.hpp"
 #include "base/tlsstream.hpp"
 #include "base/utility.hpp"
-#include <chrono>
 #include <limits>
 #include <memory>
 #include <stdexcept>
@@ -41,7 +40,7 @@ HttpServerConnection::HttpServerConnection(const WaitGroup::Ptr& waitGroup, cons
 }
 
 HttpServerConnection::HttpServerConnection(const WaitGroup::Ptr& waitGroup, const String& identity, bool authenticated, const Shared<AsioTlsStream>::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,6 +151,11 @@ bool HttpServerConnection::Disconnected()
 	return m_ShuttingDown;
 }
 
+void HttpServerConnection::SetLivenessTimeout(std::chrono::milliseconds timeout)
+{
+	m_LivenessTimeout = timeout;
+}
+
 static inline
 bool EnsureValidHeaders(
 	boost::beast::flat_buffer& buf,
@@ -453,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);
@@ -467,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());
 
 			{
@@ -519,7 +523,7 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc)
 				break;
 			}
 
-			m_Seen = std::numeric_limits<decltype(m_Seen)>::max();
+			m_Seen = ch::steady_clock::time_point::max();
 
 			ProcessRequest(request, response, m_WaitGroup, cpuBoundWorkTime, yc);
 
@@ -542,16 +546,21 @@ 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(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() - 10) {
+		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<std::chrono::seconds>(m_LivenessTimeout).count()
+				<< " seconds, disconnecting (from " << m_PeerAddress << ").";
 
 			Disconnect(yc);
 			break;

lib/remote/httpserverconnection.hpp

@@ -12,10 +12,13 @@
 #include <boost/asio/io_context.hpp>
 #include <boost/asio/io_context_strand.hpp>
 #include <boost/asio/spawn.hpp>
+#include <chrono>
 
 namespace icinga
 {
 
+using namespace std::chrono_literals;
+
 /**
  * An API client connection.
  *
@@ -33,11 +36,23 @@ public:
 	void StartDetectClientSideShutdown();
 	bool Disconnected();
 
+	/**
+	 * 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 timeout The timeout duration.
+	 */
+	void SetLivenessTimeout(std::chrono::milliseconds timeout);
+
 private:
 	WaitGroup::Ptr m_WaitGroup;
 	ApiUser::Ptr m_ApiUser;
 	Shared<AsioTlsStream>::Ptr m_Stream;
-	double m_Seen;
+	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;

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()

test/remote-httpserverconnection.cpp

@@ -45,10 +45,11 @@ struct HttpServerConnectionFixture : TlsStreamFixture, ConfigurationCacheDirFixt
 		user->Register();
 	}
 
-	void SetupHttpServerConnection(bool authenticated)
+	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);
+		m_Connection->SetLivenessTimeout(livenessTimeout);
 		m_Connection->Start();
 	}
 
@@ -552,11 +553,11 @@ BOOST_AUTO_TEST_CASE(handler_throw_streaming)
 
 BOOST_AUTO_TEST_CASE(liveness_disconnect)
 {
-	SetupHttpServerConnection(false);
+	SetupHttpServerConnection(false, std::chrono::milliseconds(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(ExpectLogPattern("No messages for HTTP connection have been received in the last \\d+ seconds, disconnecting .*"));
 	BOOST_REQUIRE(Shutdown(client));
 }