Propagate FTP server connection closures to idle FTP gw clients but

carefully close the FTP server connection upon receiving an FTP 221 response.

FTP gateway code pins server connections; when there is no traffic, the
client-side code holds on to the server connection. The old code did not
notice pinned server connection closure until it was time to write the next
FTP request to that connection. Squid now monitors the idle pinned server
connection and closes the client connection if the server connection closes
(or, to be more precise, if the server connection gets marked as ready for
reading).

Client-side monitoring of an idle pinned connection ends when the server side
starts using the pinned connection again (because the server side becomes
responsible for monitoring EOF conditions then). Added
borrowPinnedConnection() and related methods to distinguish validation of
pinned connection and responsibility transfer.

Careful closure of the FTP server connection upon receiving an FTP 221
response is necessary to avoid the client-side idle server connection
monitoring code closing the client connection upon detecting server-side EOF
_before_ the client side had enough time to forward that 221 response to the
not-yet-idle client. The latter may take some time because, in part, the
response may have to go through an ICAP RESPMOD service first.

diff --git a/src/FtpGatewayServer.cc b/src/FtpGatewayServer.cc
index 1749e6076e1f24ef16ad18e68166f1e96b18ebc7..21cbb74f4baa8c818b4bcaf3d0611a54136a8a82 100644 (file)
--- a/src/FtpGatewayServer.cc
+++ b/src/FtpGatewayServer.cc
@@ -133,11 +133,22 @@ ServerStateData::start()
 void
 ServerStateData::serverComplete()
 {
-    if (Comm::IsConnOpen(ctrl.conn)) {
-        debugs(9, 5, "preserve FTP server FD " << ctrl.conn->fd);
-        fwd->unregister(ctrl.conn);
-        ctrl.forget();
-        // fwd->request->clientConnectionManager has this connection pinned
+    CbcPointer<ConnStateData> &mgr = fwd->request->clientConnectionManager;
+    if (mgr.valid()) {
+        if (Comm::IsConnOpen(ctrl.conn)) {
+            debugs(9, 7, "completing FTP server " << ctrl.conn <<
+                   " after " << ctrl.replycode);
+            fwd->unregister(ctrl.conn);
+            if (ctrl.replycode == 221) { // Server sends FTP 221 before closing
+                mgr->unpinConnection(false);
+                ctrl.close();
+            } else {
+                mgr->pinConnection(ctrl.conn, fwd->request,
+                                   ctrl.conn->getPeer(),
+                                   fwd->request->flags.connectionAuth);
+                ctrl.forget();
+            }
+        }
     }
     Ftp::ServerStateData::serverComplete();
 }

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 64f572873281fa08c40042d580871f97c27a9537..9d66a7a88738317fb0502604acacc779bee3bdbb 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -1123,7 +1123,7 @@ FwdState::connectStart()
         debugs(17,7, "pinned peer connection: " << pinned_connection);
         // pinned_connection may become nil after a pconn race
         if (pinned_connection)
-            serverConn = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer());
+            serverConn = pinned_connection->borrowPinnedConnection(request, serverDestinations[0]->getPeer());
         else
             serverConn = NULL;
         if (Comm::IsConnOpen(serverConn)) {

diff --git a/src/client_side.cc b/src/client_side.cc
index 58ab5011439abee7c6eb7ba2cdfc5ae6cf3c14c0..e900168bc395a91946be67cfdc8617aa2e338a58 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -950,7 +950,7 @@ ConnStateData::swanSong()
     assert(areAllContextsForThisConnection());
     freeAllContexts();
 
-    unpinConnection();
+    unpinConnection(true);
 
     if (Comm::IsConnOpen(clientConnection))
         clientConnection->close();
@@ -4797,7 +4797,7 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
     assert(pinning.serverConnection == io.conn);
     pinning.closeHandler = NULL; // Comm unregisters handlers before calling
     const bool sawZeroReply = pinning.zeroReply; // reset when unpinning
-    unpinConnection();
+    unpinConnection(false);
     if (sawZeroReply) {
         debugs(33, 3, "Closing client connection on pinned zero reply.");
         clientConnection->close();
@@ -4815,19 +4815,24 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
 void
 ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth)
 {
-    char desc[FD_DESC_SZ];
+    if (!Comm::IsConnOpen(pinning.serverConnection) || 
+        pinning.serverConnection->fd != pinServer->fd)
+        pinNewConnection(pinServer, request, aPeer, auth);
 
-    if (Comm::IsConnOpen(pinning.serverConnection)) {
-        if (pinning.serverConnection->fd == pinServer->fd)
-            return;
-    }
+    startMonitoringPinnedConnection();
+}
 
-    unpinConnection(); // closes pinned connection, if any, and resets fields
+void
+ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth)
+{
+    unpinConnection(true); // closes pinned connection, if any, and resets fields
 
     pinning.serverConnection = pinServer;
 
     debugs(33, 3, HERE << pinning.serverConnection);
 
+    Must(pinning.serverConnection != NULL);
+
     // when pinning an SSL bumped connection, the request may be NULL
     const char *pinnedHost = "[unknown]";
     if (request) {
@@ -4842,6 +4847,7 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque
         pinning.peer = cbdataReference(aPeer);
     pinning.auth = auth;
     char stmp[MAX_IPSTRLEN];
+    char desc[FD_DESC_SZ];
     snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)",
              (auth || !aPeer) ? pinnedHost : aPeer->name,
              clientConnection->remote.toUrl(stmp,MAX_IPSTRLEN),
@@ -4877,14 +4883,69 @@ ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *a
 
     if (!valid) {
         /* The pinning info is not safe, remove any pinning info */
-        unpinConnection();
+        unpinConnection(true);
     }
 
     return pinning.serverConnection;
 }
 
+Comm::ConnectionPointer
+ConnStateData::borrowPinnedConnection(HttpRequest *request, const CachePeer *aPeer)
+{
+    debugs(33, 7, pinning.serverConnection);
+    if (validatePinnedConnection(request, aPeer) != NULL)
+        stopMonitoringPinnedConnection();
+
+    return pinning.serverConnection; // closed if validation failed
+}
+
+/// [re]start monitoring pinned connection for server closures so that we can
+/// propagate them to an _idle_ client pinned to the server
+void
+ConnStateData::startMonitoringPinnedConnection()
+{
+    if (!pinning.reading) {
+         pinning.reading = true;
+         Comm::SetSelect(pinning.serverConnection->fd, COMM_SELECT_READ,
+                         &ConnStateData::ReadPinnedConnection,
+                         new Pointer(this), 0);
+    }
+}
+
+/// stop or suspend monitoring pinned connection for server closures
 void
-ConnStateData::unpinConnection()
+ConnStateData::stopMonitoringPinnedConnection()
+{
+    if (pinning.reading) {
+         Comm::SetSelect(pinning.serverConnection->fd, COMM_SELECT_READ, NULL, NULL, 0);
+         pinning.reading = false;
+    }
+}
+
+/// read callback for the idle pinned server connection
+void
+ConnStateData::ReadPinnedConnection(int fd, void *data)
+{
+    Pointer *ptr = static_cast<Pointer*>(data);
+    if (ConnStateData *client = dynamic_cast<ConnStateData*>(ptr->valid())) {
+        // get back inside job call protection
+        typedef NullaryMemFunT<ConnStateData> Dialer;
+        AsyncCall::Pointer call = JobCallback(33, 5, Dialer, client,
+                                              ConnStateData::readPinnedConnection);
+        ScheduleCallHere(call);
+    }
+    delete ptr;
+}
+
+void
+ConnStateData::readPinnedConnection()
+{
+    pinning.reading = false; // select loop clears our subscription before cb
+    mustStop("suspected pinned server eof");
+}
+
+void
+ConnStateData::unpinConnection(const bool andClose)
 {
     debugs(33, 3, HERE << pinning.serverConnection);
 
@@ -4896,9 +4957,13 @@ ConnStateData::unpinConnection()
             comm_remove_close_handler(pinning.serverConnection->fd, pinning.closeHandler);
             pinning.closeHandler = NULL;
         }
-        /// also close the server side socket, we should not use it for any future requests...
-        // TODO: do not close if called from our close handler?
-        pinning.serverConnection->close();
+
+        stopMonitoringPinnedConnection();
+
+        // close the server side socket if requested
+        if (andClose)
+            pinning.serverConnection->close();
+        pinning.serverConnection = NULL;
     }
 
     safe_free(pinning.host);
@@ -5751,7 +5816,7 @@ FtpHandleUserRequest(ConnStateData *connState, const String &cmd, String &params
         debugs(11, 3, "reset URI from " << connState->ftp.uri << " to " << uri);
         FtpCloseDataConnection(connState);
         connState->ftp.readGreeting = false;
-        connState->unpinConnection(); // close control connection to the server
+        connState->unpinConnection(true); // close control connection to the server
         FtpChangeState(connState, ConnStateData::FTP_BEGIN, "URI reset");
     }
 

diff --git a/src/client_side.h b/src/client_side.h
index 24fe3cad48bf90f2729873294020f1ce92f4dcf4..f29edfc16057fd03465944d7b0cade47af045fb6 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -267,6 +267,7 @@ public:
         int port;               /* port of pinned connection */
         bool pinned;             /* this connection was pinned */
         bool auth;               /* pinned for www authentication */
+        bool reading;   ///< we are monitoring for server connection closure
         bool zeroReply; ///< server closed w/o response (ERR_ZERO_SIZE_OBJECT)
         CachePeer *peer;             /* CachePeer the connection goes via */
         AsyncCall::Pointer closeHandler; /*The close handler for pinned server side connection*/
@@ -297,14 +298,13 @@ public:
     bool handleReadData(char *buf, size_t size);
     bool handleRequestBodyData();
 
-    /**
-     * Correlate the current ConnStateData object with the pinning_fd socket descriptor.
-     */
+    /// forward future client requests using the given server connection
+    /// monitor pinned server connection for server-side closures
     void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth);
-    /**
-     * Decorrelate the ConnStateData object from its pinned CachePeer
-     */
-    void unpinConnection();
+    /// undo pinConnection() and, optionally, close the pinned connection
+    void unpinConnection(const bool andClose);
+    /// returns validated pinnned server connection (and stops its monitoring)
+    Comm::ConnectionPointer borrowPinnedConnection(HttpRequest *request, const CachePeer *aPeer);
     /**
      * Checks if there is pinning info if it is valid. It can close the server side connection
      * if pinned info is not valid.
@@ -423,6 +423,12 @@ private:
 
     bool concurrentRequestQueueFilled() const;
 
+    void pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth);
+    void startMonitoringPinnedConnection();
+    void stopMonitoringPinnedConnection();
+    static void ReadPinnedConnection(int fd, void *data);
+    void readPinnedConnection();
+
 #if USE_AUTH
     /// some user details that can be used to perform authentication on this connection
     Auth::UserRequest::Pointer auth_;

diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc
index 77d9ff469277edb519b67e4a86645b8305845448..58d1a26e10c6620b249e667aeb015460d417599b 100644 (file)
--- a/src/tests/stub_client_side.cc
+++ b/src/tests/stub_client_side.cc
@@ -57,7 +57,7 @@ void ConnStateData::noteBodyConsumerAborted(BodyPipe::Pointer) STUB
 bool ConnStateData::handleReadData(char *buf, size_t size) STUB_RETVAL(false)
 bool ConnStateData::handleRequestBodyData() STUB_RETVAL(false)
 void ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth) STUB
-void ConnStateData::unpinConnection() STUB
+void ConnStateData::unpinConnection(bool andClose) STUB
 const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *peer) STUB_RETVAL(NULL)
 void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) STUB
 void ConnStateData::clientReadRequest(const CommIoCbParams &io) STUB