Do not close FTP gw server control connection on more server-unrelated errors.

FtpGatewayServer now forgets about its control connection to the server once
it is done communicating with that server. This allows us to preserve that
connection on FTP transaction errors (e.g., forbidden responses or broken
ICAP servers) that are not related to the server communication and can be
ignored as far as the next FTP gatewayed transaction is concerned.

Removed special DONE state because it is now identical to doneWithServer().

Fixed ServerChannel::forget() to prevent subsequent forgotten connection
closure.

diff --git a/src/FtpGatewayServer.cc b/src/FtpGatewayServer.cc
index 23f9ecbe38fd9aa15e345f8d69878236143a35b2..94e134e88f37e6f5002ac068624fb7b0d76f0fc0 100644 (file)
--- a/src/FtpGatewayServer.cc
+++ b/src/FtpGatewayServer.cc
@@ -32,10 +32,10 @@ protected:
 
     ConnStateData::FtpState clientState() const;
     void clientState(ConnStateData::FtpState newState);
+    virtual void serverComplete();
     virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
     virtual void handleControlReply();
     virtual void handleRequestBodyProducerAborted();
-    virtual bool doneWithServer() const;
     virtual bool mayReadVirginReplyBody() const;
     virtual void completeForwarding();
     void forwardReply();
@@ -59,7 +59,7 @@ protected:
         SENT_DATA_REQUEST,
         READING_DATA,
         UPLOADING_DATA,
-        DONE
+        END
     };
     typedef void (ServerStateData::*SM_FUNC)();
     static const SM_FUNC SM_FUNCS[];
@@ -89,7 +89,7 @@ const ServerStateData::SM_FUNC ServerStateData::SM_FUNCS[] = {
     &ServerStateData::readDataReply, // SENT_DATA_REQUEST
     &ServerStateData::readTransferDoneReply, // READING_DATA
     &ServerStateData::readReply, // UPLOADING_DATA
-    NULL // DONE
+    NULL // END
 };
 
 ServerStateData::ServerStateData(FwdState *const fwdState):
@@ -100,10 +100,7 @@ ServerStateData::ServerStateData(FwdState *const fwdState):
 
 ServerStateData::~ServerStateData()
 {
-    if (Comm::IsConnOpen(ctrl.conn)) {
-        fwd->unregister(ctrl.conn);
-        ctrl.forget();
-    }
+    closeServer(); // TODO: move to Server.cc?
 }
 
 void
@@ -119,6 +116,20 @@ ServerStateData::start()
         sendCommand();
 }
 
+/// Keep control connection for future requests, after we are done with it.
+/// Similar to COMPLETE_PERSISTENT_MSG handling in http.cc.
+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
+    }
+    Ftp::ServerStateData::serverComplete();
+}
+
 ConnStateData::FtpState
 ServerStateData::clientState() const
 {
@@ -217,7 +228,7 @@ ServerStateData::handleControlReply()
     if (ctrl.message == NULL)
         return; // didn't get complete reply yet
 
-    assert(state < DONE);
+    assert(state < END);
     (this->*SM_FUNCS[state])();
 }
 
@@ -229,12 +240,6 @@ ServerStateData::handleRequestBodyProducerAborted()
     failed(ERR_READ_ERROR);
 }
 
-bool
-ServerStateData::doneWithServer() const
-{
-    return state == DONE || Ftp::ServerStateData::doneWithServer();
-}
-
 bool
 ServerStateData::mayReadVirginReplyBody() const
 {
@@ -253,7 +258,6 @@ ServerStateData::forwardReply()
     setVirginReply(reply);
     adaptOrFinalizeReply();
 
-    state = DONE;
     serverComplete();
 }
 
@@ -290,7 +294,6 @@ ServerStateData::proceedAfterPreliminaryReply()
 void
 ServerStateData::forwardError(err_type error, int xerrno)
 {
-    state = DONE;
     failed(error, xerrno);
 }
 
@@ -504,7 +507,6 @@ ServerStateData::readTransferDoneReply()
                " after reading data");
     }
 
-    state = DONE;
     serverComplete();
 }
 

diff --git a/src/FtpServer.cc b/src/FtpServer.cc
index 4c1ca4cd8a93d315e374f91c2f9b640dd7e66fb5..995d221ea7f1e78350ff6b9694494465fbe3a5c0 100644 (file)
--- a/src/FtpServer.cc
+++ b/src/FtpServer.cc
@@ -85,7 +85,7 @@ ServerChannel::forget()
 {
     if (Comm::IsConnOpen(conn))
         comm_remove_close_handler(conn->fd, closer);
-    closer = NULL;
+    clear();
 }
 
 void