Avoid some unnecessary FTP control connection closures; polished FTP failure

code.

Do not close FTP server control connection just because FTP response
adaptation is done. We still close FTP connections if we are receiving the
virgin response (because we have no place to store it), but if we are done
receiving, there is no need to terminate FTP server connections. The former
happens when an ICAP service responds before receiving the entire virgin FTP
response. The latter, when the ICAP service responds after the FTP data
connection is closed but before the control 226 response comes in.

Do not close FTP client control connection just because we served a non-OK
control response. The client may still send us commands if our ClientStream is
still in a good state. This may happen when an FTP download is prohibited by
an adaptation service via an HTTP 403 Forbidden response, for example.

Avoid STORE_PENDING assertion in FwdState::reforward() due to double
forwarding completion by FtpGatewayServer. Similar code exists in regular FTP
server and is needed for gatewaying as well because FTP may keep open (and
expect a control response on) the control connection after adaptation is
completed, creating two avenues for a FwdState::complete() call.

Ftp::ServerStateData::failed() should always call FwdState::fail(), to supply
forwarding code with ErrorState details. And we should not create the error
HttpReply in the FTP code. FwdState code does that, probably because it may
reforward the request and, hence, bypass some errors. For now, FTP gateway
code still creates an HttpReply (in addition to calling FwdState::fail) to be
able to supply custom gatewaying error information. Eventually, that should be
done via custom ErrorState object (that will later create an appropriate
HttpReply when/if needed).

Added debugging.

diff --git a/src/FtpGatewayServer.cc b/src/FtpGatewayServer.cc
index 50ec3d1e1db32fce9e1fe8e05734f8b2ea1f7d6b..23f9ecbe38fd9aa15e345f8d69878236143a35b2 100644 (file)
--- a/src/FtpGatewayServer.cc
+++ b/src/FtpGatewayServer.cc
@@ -31,14 +31,16 @@ protected:
     virtual void start();
 
     ConnStateData::FtpState clientState() const;
-    void clientState(ConnStateData::FtpState s) const;
+    void clientState(ConnStateData::FtpState newState);
     virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
-    virtual void failedErrorMessage(err_type error, int xerrno);
     virtual void handleControlReply();
     virtual void handleRequestBodyProducerAborted();
     virtual bool doneWithServer() const;
+    virtual bool mayReadVirginReplyBody() const;
+    virtual void completeForwarding();
     void forwardReply();
     void forwardError(err_type error = ERR_NONE, int xerrno = 0);
+    void failedErrorMessage(err_type error, int xerrno);
     HttpReply *createHttpReply(const Http::StatusCode httpStatus, const int clen = 0);
     void handleDataRequest();
     void startDataDownload();
@@ -72,6 +74,8 @@ protected:
     virtual void dataChannelConnected(const Comm::ConnectionPointer &conn, comm_err_t err, int xerrno);
     void scheduleReadControlReply();
 
+    bool forwardingCompleted; ///< completeForwarding() has been called
+
     CBDATA_CLASS2(ServerStateData);
 };
 
@@ -89,7 +93,8 @@ const ServerStateData::SM_FUNC ServerStateData::SM_FUNCS[] = {
 };
 
 ServerStateData::ServerStateData(FwdState *const fwdState):
-    AsyncJob("Ftp::Gateway::ServerStateData"), Ftp::ServerStateData(fwdState)
+    AsyncJob("Ftp::Gateway::ServerStateData"), Ftp::ServerStateData(fwdState),
+    forwardingCompleted(false)
 {
 }
 
@@ -121,9 +126,30 @@ ServerStateData::clientState() const
 }
 
 void
-ServerStateData::clientState(ConnStateData::FtpState s) const
+ServerStateData::clientState(ConnStateData::FtpState newState)
 {
-    fwd->request->clientConnectionManager->ftp.state = s;
+    ConnStateData::FtpState &cltState =
+        fwd->request->clientConnectionManager->ftp.state;
+    debugs(9, 3, "client state was " << cltState << " now: " << newState);
+    cltState = newState;
+}
+
+/**
+ * Ensure we do not double-complete on the forward entry.
+ * We complete forwarding when the response adaptation is over 
+ * (but we may still be waiting for 226 from the FTP server) and
+ * also when we get that 226 from the server (and adaptation is done).
+ *
+ \todo Rewrite FwdState to ignore double completion?
+ */
+void
+ServerStateData::completeForwarding()
+{
+    debugs(9, 5, forwardingCompleted);
+    if (forwardingCompleted)
+        return;
+    forwardingCompleted = true;
+    Ftp::ServerStateData::completeForwarding();
 }
 
 void
@@ -132,6 +158,10 @@ ServerStateData::failed(err_type error, int xerrno)
     if (!doneWithServer())
         clientState(ConnStateData::FTP_ERROR);
 
+    // TODO: we need to customize ErrorState instead
+    if (entry->isEmpty())
+        failedErrorMessage(error, xerrno); // as a reply
+
     Ftp::ServerStateData::failed(error, xerrno);
 }
 
@@ -205,6 +235,13 @@ ServerStateData::doneWithServer() const
     return state == DONE || Ftp::ServerStateData::doneWithServer();
 }
 
+bool
+ServerStateData::mayReadVirginReplyBody() const
+{
+    // TODO: move this method to the regular FTP server?
+    return Comm::IsConnOpen(data.conn);
+}
+
 void
 ServerStateData::forwardReply()
 {

diff --git a/src/FtpServer.cc b/src/FtpServer.cc
index 612c8d50ae49c10f182c1be0c12cabdcfa0bbaec..4c1ca4cd8a93d315e374f91c2f9b640dd7e66fb5 100644 (file)
--- a/src/FtpServer.cc
+++ b/src/FtpServer.cc
@@ -193,15 +193,7 @@ void
 ServerStateData::failed(err_type error, int xerrno)
 {
     debugs(9,3,HERE << "entry-null=" << (entry?entry->isEmpty():0) << ", entry=" << entry);
-    if (entry->isEmpty())
-        failedErrorMessage(error, xerrno);
 
-    serverComplete();
-}
-
-void
-ServerStateData::failedErrorMessage(err_type error, int xerrno)
-{
     const char *command, *reply;
     const Http::StatusCode httpStatus = failedHttpStatus(error);
     ErrorState *const ftperr = new ErrorState(error, httpStatus, fwd->request);
@@ -229,8 +221,9 @@ ServerStateData::failedErrorMessage(err_type error, int xerrno)
     if (reply)
         ftperr->ftp.reply = xstrdup(reply);
 
-    entry->replaceHttpReply( ftperr->BuildHttpReply() );
-    delete ftperr;
+    fwd->fail(ftperr);
+
+    closeServer(); // we failed, so no serverComplete()
 }
 
 Http::StatusCode

diff --git a/src/FtpServer.h b/src/FtpServer.h
index dd3c629ec080a9882aa3deb5ee1b3b2f65bf06be..cc7f94b10a94625a633676facf765eaf57a4713a 100644 (file)
--- a/src/FtpServer.h
+++ b/src/FtpServer.h
@@ -89,7 +89,6 @@ protected:
     void initReadBuf();
     virtual void closeServer();
     virtual bool doneWithServer() const;
-    virtual void failedErrorMessage(err_type error, int xerrno);
     virtual Http::StatusCode failedHttpStatus(err_type &error);
     void ctrlClosed(const CommCloseCbParams &io);
     void scheduleReadControlReply(int buffered_ok);

diff --git a/src/Server.cc b/src/Server.cc
index 41d4a36c32bc8f9013c86148e282efcf19a90918..289c46fd14eeb73b8152f4be5a2072f62b6883c0 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -803,10 +803,11 @@ ServerStateData::handleAdaptationCompleted()
     debugs(11,5, HERE << "handleAdaptationCompleted");
     cleanAdaptation();
 
-    // We stop reading origin response because we have no place to put it and
+    // We stop reading origin response because we have no place to put it(*) and
     // cannot use it. If some origin servers do not like that or if we want to
     // reuse more pconns, we can add code to discard unneeded origin responses.
-    if (!doneWithServer()) {
+    // (*) TODO: Is it possible that the adaptation xaction is still running?
+    if (mayReadVirginReplyBody()) {
         debugs(11,3, HERE << "closing origin conn due to ICAP completion");
         closeServer();
     }

diff --git a/src/Server.h b/src/Server.h
index 818f3ccd9026917ee027f8eedb816157ae623f97..d8bed22f7021566c7f95358312bea675f09e78d6 100644 (file)
--- a/src/Server.h
+++ b/src/Server.h
@@ -127,6 +127,8 @@ protected:
 
     virtual void closeServer() = 0;            /**< end communication with the server */
     virtual bool doneWithServer() const = 0;   /**< did we end communication? */
+    /// whether we may receive more virgin response body bytes
+    virtual bool mayReadVirginReplyBody() const { return !doneWithServer(); }
 
     /// Entry-dependent callbacks use this check to quit if the entry went bad
     bool abortOnBadEntry(const char *abortReason);

diff --git a/src/client_side.cc b/src/client_side.cc
index b83e3cb6e1324a2e83cb549fe6039fcc3ebb9cab..038996b21693880e0859442d1ff9c059b2303d38 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -4956,6 +4956,15 @@ FtpWriteCustomReply(ClientSocketContext *context, const int code, const char *ms
     FtpWriteReply(context, mb);
 }
 
+static void 
+FtpChangeState(ConnStateData *connState, const ConnStateData::FtpState newState, const char *reason)
+{
+    assert(connState);
+    debugs(33, 3, "client state was " << connState->ftp.state << ", now " <<
+           newState << " because " << reason);
+    connState->ftp.state = newState;
+}
+
 /** Parse an FTP request
  *
  *  \note Sets result->flags.parsed_ok to 0 if failed to parse the request,
@@ -4979,7 +4988,7 @@ FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::Pro
     const size_t req_sz = eor + 1 - connState->in.buf;
 
     if (eor == NULL && connState->in.notYetUsed >= Config.maxRequestHeaderSize) {
-        connState->ftp.state = ConnStateData::FTP_ERROR;
+        FtpChangeState(connState, ConnStateData::FTP_ERROR, "huge req");
         FtpWriteEarlyReply(connState, 421, "Too large request");
         return NULL;
     }
@@ -5194,15 +5203,19 @@ FtpHandleErrorReply(ClientSocketContext *context, const HttpReply *reply, StoreI
 static void
 FtpHandleDataReply(ClientSocketContext *context, const HttpReply *reply, StoreIOBuffer data)
 {
+    ConnStateData *const conn = context->getConn();
+
     if (reply != NULL && reply->sline.status() != Http::scOkay) {
         FtpWriteForwardedReply(context, reply);
+        if (conn && Comm::IsConnOpen(conn->ftp.dataConn)) {
+            debugs(33, 3, "closing " << conn->ftp.dataConn << " on KO reply");
+            conn->ftp.dataConn->close();
+        }
         return;
     }
 
     debugs(33, 7, HERE << data.length);
 
-    ConnStateData *const conn = context->getConn();
-
     if (data.length <= 0) {
         FtpWroteReplyData(conn->clientConnection, NULL, 0, COMM_OK, 0, context);
         return;
@@ -5244,6 +5257,7 @@ FtpWroteReplyData(const Comm::ConnectionPointer &conn, char *bufnotused, size_t
 
     switch (context->socketState()) {
     case STREAM_NONE:
+        debugs(33, 3, "Keep going");
         context->pullData();
         return;
     case STREAM_COMPLETE:
@@ -5283,7 +5297,7 @@ static void
 FtpWriteForwardedForeign(ClientSocketContext *context, const HttpReply *reply)
 {
     ConnStateData *const connState = context->getConn();
-    connState->ftp.state = ConnStateData::FTP_ERROR;
+    FtpChangeState(connState, ConnStateData::FTP_CONNECTED, "foreign reply");
 
     assert(context->http);
     const HttpRequest *request = context->http->request;
@@ -5418,18 +5432,29 @@ FtpWroteReply(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size
         static_cast<ClientSocketContext*>(data);
     ConnStateData *const connState = context->getConn();
 
-    if (connState->ftp.state == ConnStateData::FTP_ERROR ||
-        context->socketState() != STREAM_COMPLETE) {
+    if (connState->ftp.state == ConnStateData::FTP_ERROR) {
+        debugs(33, 5, "closing on FTP server error");
         conn->close();
         return;
     }
 
-    assert(context->socketState() == STREAM_COMPLETE);
-    connState->flags.readMore = true;
-    connState->ftp.state = ConnStateData::FTP_CONNECTED;
-    if (connState->in.bodyParser)
-        connState->finishDechunkingRequest(false);
-    context->keepaliveNextRequest();
+    const clientStream_status_t socketState = context->socketState();
+    debugs(33, 5, "FTP client stream state " << socketState);
+    switch (socketState) {
+    case STREAM_UNPLANNED_COMPLETE:
+    case STREAM_FAILED:
+         conn->close();
+         return;
+
+    case STREAM_NONE:
+    case STREAM_COMPLETE:
+        connState->flags.readMore = true;
+        FtpChangeState(connState, ConnStateData::FTP_CONNECTED, "FtpWroteReply");
+        if (connState->in.bodyParser)
+            connState->finishDechunkingRequest(false);
+        context->keepaliveNextRequest();
+        return;
+    }
 }
 
 bool
@@ -5515,7 +5540,7 @@ FtpHandlePasvRequest(ClientSocketContext *context, String &cmd, String &params)
         return false;
     }
 
-    context->getConn()->ftp.state = ConnStateData::FTP_HANDLE_PASV;
+    FtpChangeState(context->getConn(), ConnStateData::FTP_HANDLE_PASV, "FtpHandlePasvRequest");
 
     return true;
 }
@@ -5554,7 +5579,7 @@ FtpHandlePortRequest(ClientSocketContext *context, String &cmd, String &params)
     context->getConn()->ftp.dataConn = conn;
     context->getConn()->ftp.uploadAvailSize = 0; // XXX: FtpCloseDataConnection should do that
 
-    context->getConn()->ftp.state = ConnStateData::FTP_HANDLE_PORT;
+    FtpChangeState(context->getConn(), ConnStateData::FTP_HANDLE_PORT, "FtpHandlePortRequest");
 
     // convert client PORT command to Squid PASV command because Squid
     // does not support active FTP transfers on the server side (yet?)
@@ -5576,7 +5601,7 @@ FtpHandleDataRequest(ClientSocketContext *context, String &cmd, String &params)
     if (!FtpCheckDataConnection(context))
         return false;
 
-    context->getConn()->ftp.state = ConnStateData::FTP_HANDLE_DATA_REQUEST;
+    FtpChangeState(context->getConn(), ConnStateData::FTP_HANDLE_DATA_REQUEST, "FtpHandleDataRequest");
 
     return true;
 }
@@ -5587,7 +5612,7 @@ FtpHandleUploadRequest(ClientSocketContext *context, String &cmd, String &params
     if (!FtpCheckDataConnection(context))
         return false;
 
-    context->getConn()->ftp.state = ConnStateData::FTP_HANDLE_UPLOAD_REQUEST;
+    FtpChangeState(context->getConn(), ConnStateData::FTP_HANDLE_UPLOAD_REQUEST, "FtpHandleDataRequest");
 
     return true;
 }