Skip to content

Commit ed0c8a2

Browse files
committed
Fix that NewClientHandler() could hang indefinitely, preventing new connection attempts
There is some race condition when the `async_write()`/`async_flush()` operation for the `icinga::Hello` message fails (connection reset by peer for example) around the same time the connect timeout fires and calls `cancel()` on the stream, the following call to `async_shutdown()` may block indefinitely. If that happens, the endpoint remains in the connecting state and no new connection attemps are initiated. This commit fixes the issue by removing the `Defer` containing the `async_shutdown()`. The purpose of `async_shutdown()` is to signal a clean termination of the connection to the peer, which really isn't something that makes sense to to in a `Defer` block that is also executed in case of errors. For the one situation where doing a clean TLS shutdown makes some sense (closing anonymous client connections), a call to GracefulShutdown() is added to that specific code path. A large part of the change is just changing the indentation of the code, given that a now unnecessary `try`/`catch` block is removed. The following Go code creates a TLS server that can be used to demonstrate the issue. Note that given that a race condition is involved, this is not reliable and the sleep duration may need some fine-tuning. For this to work, `ApiListener.tls_handshake_timeout` needs to be set to a large-enough value like 60s to disable the timeout for `async_handshake()` itself so that the overall connect timeout is the one that fires. However, changing the timeout is not a prerequisite for the problem, it just makes it easier to reproduce. The error can also happen with the default timeouts if the TCP connect takes long enough so that the handshake is started late enough that its timeout expires after the connect timeout. package main import ( "crypto/tls" "log" "net" "time" ) func main() { cert, err := tls.LoadX509KeyPair("bad-agent.crt", "bad-agent.key") if err != nil { panic(err) } listener, err := tls.Listen("tcp", ":1337", &tls.Config{ Certificates: []tls.Certificate{cert}, }) if err != nil { panic(err) } log.Println("Listening on", listener.Addr()) for { conn, err := listener.Accept() if err != nil { panic(err) } go handle(conn.(*tls.Conn)) } } func handle(conn *tls.Conn) { addr := conn.RemoteAddr().String() log.Println(addr, "new connection") time.Sleep(15*time.Second - 10*time.Millisecond) log.Println(addr, "SetLinger(0)", conn.NetConn().(*net.TCPConn).SetLinger(0)) log.Println(addr, "Handshake()", conn.Handshake()) log.Println(addr, "conn.NetConn().Close()", conn.NetConn().Close()) } With additional logging in the `catch` block for `boost::system::system_error` and `Defer shutdownSslConn` (both removed by this commit), this showed the following. Note that in particular, `async_shutdown()` never returned, indicating that it hangs in there. [2026-04-24 17:32:56 +0200] information/ApiListener: Reconnecting to endpoint 'bad-agent' via host 'host.docker.internal' and port '1337' [2026-04-24 17:33:11 +0200] critical/ApiListener: Timeout while reconnecting to endpoint 'bad-agent' via host 'host.docker.internal' and port '1337', cancelling attempt [2026-04-24 17:33:11 +0200] information/ApiListener: New client connection for identity 'bad-agent' to [172.17.0.1]:1337 [2026-04-24 17:33:12 +0200] information/ApiListener: rethrowing for bad-agent: Error: Connection reset by peer [system:104 at /usr/include/boost/asio/detail/reactive_socket_send_op.hpp:137 in function 'do_complete'] [2026-04-24 17:33:12 +0200] information/ApiListener: doing async_shutdown for bad-agent (cherry picked from commit 78a281d)
1 parent b891360 commit ed0c8a2

1 file changed

Lines changed: 49 additions & 65 deletions

File tree

lib/remote/apilistener.cpp

Lines changed: 49 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -760,16 +760,6 @@ void ApiListener::NewClientHandlerInternal(
760760
return;
761761
}
762762

763-
Defer shutdownSslConn ([&sslConn, &yc]() {
764-
// Ignore the error, but do not throw an exception being swallowed at all cost.
765-
// https://github.com/Icinga/icinga2/issues/7351
766-
boost::system::error_code ec;
767-
768-
// Using async_shutdown() instead of AsioTlsStream::GracefulDisconnect() as this whole function
769-
// is already guarded by a timeout based on the connect timeout.
770-
sslConn.async_shutdown(yc[ec]);
771-
});
772-
773763
std::shared_ptr<X509> cert (sslConn.GetPeerCertificate());
774764
bool verify_ok = false;
775765
String identity;
@@ -821,8 +811,46 @@ void ApiListener::NewClientHandlerInternal(
821811

822812
ClientType ctype;
823813

824-
try {
825-
if (role == RoleClient) {
814+
if (role == RoleClient) {
815+
JsonRpc::SendMessage(client, new Dictionary({
816+
{ "jsonrpc", "2.0" },
817+
{ "method", "icinga::Hello" },
818+
{ "params", new Dictionary({
819+
{ "version", (double)l_AppVersionInt },
820+
{ "capabilities", (double)ApiCapabilities::MyCapabilities }
821+
}) }
822+
}), yc);
823+
824+
client->async_flush(yc);
825+
826+
ctype = ClientJsonRpc;
827+
} else {
828+
{
829+
boost::system::error_code ec;
830+
831+
if (client->async_fill(yc[ec]) == 0u) {
832+
if (identity.IsEmpty()) {
833+
Log(LogInformation, "ApiListener")
834+
<< "No data received on new API connection " << conninfo << ": " << ec.message()
835+
<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
836+
} else {
837+
Log(LogWarning, "ApiListener")
838+
<< "No data received on new API connection " << conninfo << " for identity '" << identity << "': " << ec.message()
839+
<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
840+
}
841+
842+
return;
843+
}
844+
}
845+
846+
char firstByte = 0;
847+
848+
{
849+
asio::mutable_buffer firstByteBuf (&firstByte, 1);
850+
client->peek(firstByteBuf);
851+
}
852+
853+
if (firstByte >= '0' && firstByte <= '9') {
826854
JsonRpc::SendMessage(client, new Dictionary({
827855
{ "jsonrpc", "2.0" },
828856
{ "method", "icinga::Hello" },
@@ -836,58 +864,15 @@ void ApiListener::NewClientHandlerInternal(
836864

837865
ctype = ClientJsonRpc;
838866
} else {
839-
{
840-
boost::system::error_code ec;
841-
842-
if (client->async_fill(yc[ec]) == 0u) {
843-
if (identity.IsEmpty()) {
844-
Log(LogInformation, "ApiListener")
845-
<< "No data received on new API connection " << conninfo << ": " << ec.message()
846-
<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
847-
} else {
848-
Log(LogWarning, "ApiListener")
849-
<< "No data received on new API connection " << conninfo << " for identity '" << identity << "': " << ec.message()
850-
<< ". Ensure that the remote endpoints are properly configured in a cluster setup.";
851-
}
852-
853-
return;
854-
}
855-
}
856-
857-
char firstByte = 0;
858-
859-
{
860-
asio::mutable_buffer firstByteBuf (&firstByte, 1);
861-
client->peek(firstByteBuf);
862-
}
863-
864-
if (firstByte >= '0' && firstByte <= '9') {
865-
JsonRpc::SendMessage(client, new Dictionary({
866-
{ "jsonrpc", "2.0" },
867-
{ "method", "icinga::Hello" },
868-
{ "params", new Dictionary({
869-
{ "version", (double)l_AppVersionInt },
870-
{ "capabilities", (double)ApiCapabilities::MyCapabilities }
871-
}) }
872-
}), yc);
873-
874-
client->async_flush(yc);
875-
876-
ctype = ClientJsonRpc;
877-
} else {
878-
ctype = ClientHttp;
879-
}
880-
}
881-
} catch (const boost::system::system_error& systemError) {
882-
if (systemError.code() == boost::asio::error::operation_aborted) {
883-
shutdownSslConn.Cancel();
867+
ctype = ClientHttp;
884868
}
885-
886-
throw;
887869
}
888870

889871
std::shared_lock wgLock(*m_ListenerWaitGroup, std::try_to_lock);
890872
if (!wgLock) {
873+
// Close the connection cleanly, signals to the other endpoint that it wasn't closed due to an error.
874+
client->GracefulDisconnect(*strand, yc);
875+
891876
return;
892877
}
893878

@@ -922,20 +907,19 @@ void ApiListener::NewClientHandlerInternal(
922907
<< "Ignoring anonymous JSON-RPC connection " << conninfo
923908
<< ". Max connections (" << GetMaxAnonymousClients() << ") exceeded.";
924909

925-
aclient = nullptr;
926-
}
910+
// Close the connection cleanly, signals to the other endpoint that it wasn't closed due to an error.
911+
client->GracefulDisconnect(*strand, yc);
927912

928-
if (aclient) {
929-
aclient->Start();
930-
shutdownSslConn.Cancel();
913+
return;
931914
}
915+
916+
aclient->Start();
932917
} else {
933918
Log(LogNotice, "ApiListener", "New HTTP client");
934919

935920
HttpServerConnection::Ptr aclient = new HttpServerConnection(m_WaitGroup, identity, verify_ok, client);
936921
AddHttpClient(aclient);
937922
aclient->Start();
938-
shutdownSslConn.Cancel();
939923
}
940924
}
941925

0 commit comments

Comments
 (0)