Preserve caller context in TcpAcceptor (#1087)

Before this fix, acceptOne() was losing FTP DATA TcpAcceptor context by unconditionally resetting the context to a nil listenPort_. Also, TcpAcceptor methods should not (be expected to) explicitly set their job code context. Instead, the job should be created in the right code context, allowing job-created async calls to auto-manage code context. The updated port-iterating clientHttpConnectionsOpen() and Ftp::StartListening() loops now set the right code context when creating async calls that result in TcpAcceptor job creation.
squid-cache · Jul 15, 2022 · f38db63 · f38db63
1 parent d448e1e
commit f38db63
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 19 deletions.
diff --git a/src/client_side.cc b/src/client_side.cc
@@ -3303,7 +3303,9 @@ AddOpenedHttpSocket(const Comm::ConnectionPointer &conn)
 static void
 clientHttpConnectionsOpen(void)
 {
+    const auto savedContext = CodeContext::Current();
     for (AnyP::PortCfgPointer s = HttpPortList; s != nullptr; s = s->next) {
+        CodeContext::Reset(s);
         const SBuf &scheme = AnyP::UriScheme(s->transport.protocol).image();
 
         if (MAXTCPLISTENPORTS == NHttpSockets) {
@@ -3344,6 +3346,7 @@ clientHttpConnectionsOpen(void)
                 CommAcceptCbPtrFun(isHttps ? httpsAccept : httpAccept, CommAcceptCbParams(nullptr)));
         clientStartListeningOn(s, subCall, isHttps ? Ipc::fdnHttpsSocket : Ipc::fdnHttpSocket);
     }
+    CodeContext::Reset(savedContext);
 }
 
 void
@@ -3424,13 +3427,16 @@ clientOpenListenSockets(void)
 void
 clientConnectionsClose()
 {
+    const auto savedContext = CodeContext::Current();
     for (AnyP::PortCfgPointer s = HttpPortList; s != nullptr; s = s->next) {
+        CodeContext::Reset(s);
         if (s->listenConn != nullptr) {
             debugs(1, Important(14), "Closing HTTP(S) port " << s->listenConn->local);
             s->listenConn->close();
             s->listenConn = nullptr;
         }
     }
+    CodeContext::Reset(savedContext);
 
     Ftp::StopListening();
 

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
@@ -11,6 +11,7 @@
 #include "squid.h"
 #include "acl/FilledChecklist.h"
 #include "anyp/PortCfg.h"
+#include "base/CodeContext.h"
 #include "base/TextException.h"
 #include "client_db.h"
 #include "comm/AcceptLimiter.h"
@@ -72,8 +73,6 @@ Comm::TcpAcceptor::unsubscribe(const char *reason)
 void
 Comm::TcpAcceptor::start()
 {
-    if (listenPort_)
-        CodeContext::Reset(listenPort_);
     debugs(5, 5, status() << " AsyncCall Subscription: " << theCallSub);
 
     Must(IsConnOpen(conn));
@@ -249,17 +248,16 @@ void
 Comm::TcpAcceptor::logAcceptError(const ConnectionPointer &tcpClient) const
 {
     AccessLogEntry::Pointer al = new AccessLogEntry;
-    CodeContext::Reset(al);
-    al->tcpClient = tcpClient;
-    al->url = "error:accept-client-connection";
-    al->setVirginUrlForMissingRequest(al->url);
-    ACLFilledChecklist ch(nullptr, nullptr, nullptr);
-    ch.src_addr = tcpClient->remote;
-    ch.my_addr = tcpClient->local;
-    ch.al = al;
-    accessLogLog(al, &ch);
-
-    CodeContext::Reset(listenPort_);
+    CallBack(al, [&] {
+        al->tcpClient = tcpClient;
+        al->url = "error:accept-client-connection";
+        al->setVirginUrlForMissingRequest(al->url);
+        ACLFilledChecklist ch(nullptr, nullptr, nullptr);
+        ch.src_addr = tcpClient->remote;
+        ch.my_addr = tcpClient->local;
+        ch.al = al;
+        accessLogLog(al, &ch);
+    });
 }
 
 void
@@ -291,12 +289,12 @@ Comm::TcpAcceptor::acceptOne()
         debugs(5, 5, "try later: " << conn << " handler Subscription: " << theCallSub);
     } else {
         // TODO: When ALE, MasterXaction merge, use them or ClientConn instead.
-        CodeContext::Reset(newConnDetails);
-        debugs(5, 5, "Listener: " << conn <<
-               " accepted new connection " << newConnDetails <<
-               " handler Subscription: " << theCallSub);
-        notify(flag, newConnDetails);
-        CodeContext::Reset(listenPort_);
+        CallBack(newConnDetails, [&] {
+            debugs(5, 5, "Listener: " << conn <<
+                   " accepted new connection " << newConnDetails <<
+                   " handler Subscription: " << theCallSub);
+            notify(flag, newConnDetails);
+        });
     }
 
     SetSelect(conn->fd, COMM_SELECT_READ, doAccept, this, 0);

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
@@ -264,7 +264,9 @@ Ftp::Server::AcceptCtrlConnection(const CommAcceptCbParams &params)
 void
 Ftp::StartListening()
 {
+    const auto savedContext = CodeContext::Current();
     for (AnyP::PortCfgPointer s = FtpPortList; s != nullptr; s = s->next) {
+        CodeContext::Reset(s);
         if (MAXTCPLISTENPORTS == NHttpSockets) {
             debugs(1, DBG_IMPORTANT, "Ignoring ftp_port lines exceeding the" <<
                    " limit of " << MAXTCPLISTENPORTS << " ports.");
@@ -278,18 +280,22 @@ Ftp::StartListening()
                                                CommAcceptCbParams(nullptr)));
         clientStartListeningOn(s, subCall, Ipc::fdnFtpSocket);
     }
+    CodeContext::Reset(savedContext);
 }
 
 void
 Ftp::StopListening()
 {
+    const auto savedContext = CodeContext::Current();
     for (AnyP::PortCfgPointer s = FtpPortList; s != nullptr; s = s->next) {
+        CodeContext::Reset(s);
         if (s->listenConn != nullptr) {
             debugs(1, DBG_IMPORTANT, "Closing FTP port " << s->listenConn->local);
             s->listenConn->close();
             s->listenConn = nullptr;
         }
     }
+    CodeContext::Reset(savedContext);
 }
 
 void