From d768c909373b108da764dacf4ff8e90d972edc43 Mon Sep 17 00:00:00 2001
From: Yonas Habteab <yonas.habteab@icinga.com>
Date: Mon, 4 Nov 2024 11:26:10 +0100
Subject: [PATCH] HttpServerConnection: Don't spawn useless coroutines

Currently, for each `Disconnect()` call, we spawn a coroutine, but every
one of them is just usesless, except the first one. However, since all
`Disconnect()` usages share the same asio strand and cannot interfere
with each other, spawning another coroutine within `Disconnect()` isn't
even necessary. When a coroutine calls `Disconnect()` now, it will
immediately initiate an async shutdown of the socket, potentially causing
the coroutine to yield and allowing the others to resume. Therefore, the
`m_ShuttingDown` flag is still required by the coroutines to be checked
regularly.
---
 lib/remote/httpserverconnection.cpp | 65 ++++++++++++++++-------------
 lib/remote/httpserverconnection.hpp |  3 +-
 2 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/lib/remote/httpserverconnection.cpp b/lib/remote/httpserverconnection.cpp
index 9b800ea8668..0a25514ebe1 100644
--- a/lib/remote/httpserverconnection.cpp
+++ b/lib/remote/httpserverconnection.cpp
@@ -68,44 +68,49 @@ void HttpServerConnection::Start()
 	IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) { CheckLiveness(yc); });
 }
 
-void HttpServerConnection::Disconnect()
+/**
+ * Tries to asynchronously shut down the SSL stream and underlying socket.
+ *
+ * It is important to note that this method should only be called from within a coroutine that uses `m_IoStrand`.
+ *
+ * @param yc boost::asio::yield_context The coroutine yield context which you are calling this method from.
+ */
+void HttpServerConnection::Disconnect(boost::asio::yield_context yc)
 {
 	namespace asio = boost::asio;
 
-	HttpServerConnection::Ptr keepAlive (this);
+	if (m_ShuttingDown) {
+		return;
+	}
 
-	IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) {
-		if (!m_ShuttingDown) {
-			m_ShuttingDown = true;
+	m_ShuttingDown = true;
 
-			Log(LogInformation, "HttpServerConnection")
-				<< "HTTP client disconnected (from " << m_PeerAddress << ")";
-
-			/*
-			 * Do not swallow exceptions in a coroutine.
-			 * https://github.com/Icinga/icinga2/issues/7351
-			 * We must not catch `detail::forced_unwind exception` as
-			 * this is used for unwinding the stack.
-			 *
-			 * Just use the error_code dummy here.
-			 */
-			boost::system::error_code ec;
+	Log(LogInformation, "HttpServerConnection")
+		<< "HTTP client disconnected (from " << m_PeerAddress << ")";
+
+	/*
+	 * Do not swallow exceptions in a coroutine.
+	 * https://github.com/Icinga/icinga2/issues/7351
+	 * We must not catch `detail::forced_unwind exception` as
+	 * this is used for unwinding the stack.
+	 *
+	 * Just use the error_code dummy here.
+	 */
+	boost::system::error_code ec;
 
-			m_CheckLivenessTimer.cancel();
+	m_CheckLivenessTimer.cancel();
 
-			m_Stream->lowest_layer().cancel(ec);
+	m_Stream->lowest_layer().cancel(ec);
 
-			m_Stream->next_layer().async_shutdown(yc[ec]);
+	m_Stream->next_layer().async_shutdown(yc[ec]);
 
-			m_Stream->lowest_layer().shutdown(m_Stream->lowest_layer().shutdown_both, ec);
+	m_Stream->lowest_layer().shutdown(m_Stream->lowest_layer().shutdown_both, ec);
 
-			auto listener (ApiListener::GetInstance());
+	auto listener (ApiListener::GetInstance());
 
-			if (listener) {
-				listener->RemoveHttpClient(this);
-			}
-		}
-	});
+	if (listener) {
+		listener->RemoveHttpClient(this);
+	}
 }
 
 void HttpServerConnection::StartStreaming()
@@ -126,7 +131,7 @@ void HttpServerConnection::StartStreaming()
 				m_Stream->async_read_some(readBuf, yc[ec]);
 			} while (!ec);
 
-			Disconnect();
+			Disconnect(yc);
 		}
 	});
 }
@@ -563,7 +568,7 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc)
 		}
 	}
 
-	Disconnect();
+	Disconnect(yc);
 }
 
 void HttpServerConnection::CheckLiveness(boost::asio::yield_context yc)
@@ -582,7 +587,7 @@ void HttpServerConnection::CheckLiveness(boost::asio::yield_context yc)
 			Log(LogInformation, "HttpServerConnection")
 				<<  "No messages for HTTP connection have been received in the last 10 seconds.";
 
-			Disconnect();
+			Disconnect(yc);
 			break;
 		}
 	}
diff --git a/lib/remote/httpserverconnection.hpp b/lib/remote/httpserverconnection.hpp
index 9c812e526bd..63f99e19cf5 100644
--- a/lib/remote/httpserverconnection.hpp
+++ b/lib/remote/httpserverconnection.hpp
@@ -28,7 +28,6 @@ class HttpServerConnection final : public Object
 	HttpServerConnection(const String& identity, bool authenticated, const Shared<AsioTlsStream>::Ptr& stream);
 
 	void Start();
-	void Disconnect();
 	void StartStreaming();
 
 	bool Disconnected();
@@ -45,6 +44,8 @@ class HttpServerConnection final : public Object
 
 	HttpServerConnection(const String& identity, bool authenticated, const Shared<AsioTlsStream>::Ptr& stream, boost::asio::io_context& io);
 
+	void Disconnect(boost::asio::yield_context yc);
+
 	void ProcessMessages(boost::asio::yield_context yc);
 	void CheckLiveness(boost::asio::yield_context yc);
 };