From: Alex Rousskov Date: Sun, 25 Aug 2013 19:50:44 +0000 (-0600) Subject: Avoid some unnecessary FTP control connection closures; polished FTP failure X-Git-Tag: SQUID_3_5_0_1~117^2~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=719c885c4a523e2426f7a00bc9bb93cf3d4904ff;p=thirdparty%2Fsquid.git 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 50ec3d1e1d..23f9ecbe38 100644 --- 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 612c8d50ae..4c1ca4cd8a 100644 --- 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 dd3c629ec0..cc7f94b10a 100644 --- 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 41d4a36c32..289c46fd14 100644 --- 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 818f3ccd90..d8bed22f70 100644 --- 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 b83e3cb6e1..038996b216 100644 --- 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(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 ¶ms) 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 ¶ms) 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 ¶ms) 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 ¶ms 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; }