Merge pull request #10929 from Icinga/backport-10833-to-support/2.16

[Backport support/2.16] Fix that NewClientHandler() could hang indefinitely, preventing new connection attempts

Author: julianbrost

lib/remote/apilistener.cpp

@@ -767,16 +767,6 @@ void ApiListener::NewClientHandlerInternal(
 		return;
 	}
 
-	Defer shutdownSslConn ([&sslConn, &yc]() {
-		// Ignore the error, but do not throw an exception being swallowed at all cost.
-		// https://github.com/Icinga/icinga2/issues/7351
-		boost::system::error_code ec;
-
-		// Using async_shutdown() instead of AsioTlsStream::GracefulDisconnect() as this whole function
-		// is already guarded by a timeout based on the connect timeout.
-		sslConn.async_shutdown(yc[ec]);
-	});
-
 	std::shared_ptr<X509> cert (sslConn.GetPeerCertificate());
 	bool verify_ok = false;
 	String identity;
@@ -828,8 +818,46 @@ void ApiListener::NewClientHandlerInternal(
 
 	ClientType ctype;
 
-	try {
-		if (role == RoleClient) {
+	if (role == RoleClient) {
+		JsonRpc::SendMessage(client, new Dictionary({
+			{ "jsonrpc", "2.0" },
+			{ "method", "icinga::Hello" },
+			{ "params", new Dictionary({
+				{ "version", (double)l_AppVersionInt },
+				{ "capabilities", (double)ApiCapabilities::MyCapabilities }
+			}) }
+		}), yc);
+
+		client->async_flush(yc);
+
+		ctype = ClientJsonRpc;
+	} else {
+		{
+			boost::system::error_code ec;
+
+			if (client->async_fill(yc[ec]) == 0u) {
+				if (identity.IsEmpty()) {
+					Log(LogInformation, "ApiListener")
+						<< "No data received on new API connection " << conninfo << ": " << ec.message()
+						<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
+				} else {
+					Log(LogWarning, "ApiListener")
+						<< "No data received on new API connection " << conninfo << " for identity '" << identity << "': " << ec.message()
+						<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
+				}
+
+				return;
+			}
+		}
+
+		char firstByte = 0;
+
+		{
+			asio::mutable_buffer firstByteBuf (&firstByte, 1);
+			client->peek(firstByteBuf);
+		}
+
+		if (firstByte >= '0' && firstByte <= '9') {
 			JsonRpc::SendMessage(client, new Dictionary({
 				{ "jsonrpc", "2.0" },
 				{ "method", "icinga::Hello" },
@@ -843,58 +871,15 @@ void ApiListener::NewClientHandlerInternal(
 
 			ctype = ClientJsonRpc;
 		} else {
-			{
-				boost::system::error_code ec;
-
-				if (client->async_fill(yc[ec]) == 0u) {
-					if (identity.IsEmpty()) {
-						Log(LogInformation, "ApiListener")
-							<< "No data received on new API connection " << conninfo << ": " << ec.message()
-							<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
-					} else {
-						Log(LogWarning, "ApiListener")
-							<< "No data received on new API connection " << conninfo << " for identity '" << identity << "': " << ec.message()
-							<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
-					}
-
-					return;
-				}
-			}
-
-			char firstByte = 0;
-
-			{
-				asio::mutable_buffer firstByteBuf (&firstByte, 1);
-				client->peek(firstByteBuf);
-			}
-
-			if (firstByte >= '0' && firstByte <= '9') {
-				JsonRpc::SendMessage(client, new Dictionary({
-					{ "jsonrpc", "2.0" },
-					{ "method", "icinga::Hello" },
-					{ "params", new Dictionary({
-						{ "version", (double)l_AppVersionInt },
-						{ "capabilities", (double)ApiCapabilities::MyCapabilities }
-					}) }
-				}), yc);
-
-				client->async_flush(yc);
-
-				ctype = ClientJsonRpc;
-			} else {
-				ctype = ClientHttp;
-			}
-		}
-	} catch (const boost::system::system_error& systemError) {
-		if (systemError.code() == boost::asio::error::operation_aborted) {
-			shutdownSslConn.Cancel();
+			ctype = ClientHttp;
 		}
-
-		throw;
 	}
 
 	std::shared_lock wgLock(*m_ListenerWaitGroup, std::try_to_lock);
 	if (!wgLock) {
+		// Close the connection cleanly, signals to the other endpoint that it wasn't closed due to an error.
+		client->GracefulDisconnect(*strand, yc);
+
 		return;
 	}
 
@@ -929,20 +914,19 @@ void ApiListener::NewClientHandlerInternal(
 				<< "Ignoring anonymous JSON-RPC connection " << conninfo
 				<< ". Max connections (" << GetMaxAnonymousClients() << ") exceeded.";
 
-			aclient = nullptr;
-		}
+			// Close the connection cleanly, signals to the other endpoint that it wasn't closed due to an error.
+			client->GracefulDisconnect(*strand, yc);
 
-		if (aclient) {
-			aclient->Start();
-			shutdownSslConn.Cancel();
+			return;
 		}
+
+		aclient->Start();
 	} else {
 		Log(LogNotice, "ApiListener", "New HTTP client");
 
 		HttpServerConnection::Ptr aclient = new HttpServerConnection(m_WaitGroup, identity, verify_ok, client);
 		AddHttpClient(aclient);
 		aclient->Start();
-		shutdownSslConn.Cancel();
 	}
 }