From: hno <> Date: Thu, 19 Jul 2007 18:07:41 +0000 (+0000) Subject: Bug #1921: ftp.cc(3309) ICAP cannot keep up with FTP; lost 1448/1448 bytes. X-Git-Tag: SQUID_3_0_PRE7~155 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7dc79973203a64975faa3c9746f50e97420abdd2;p=thirdparty%2Fsquid.git Bug #1921: ftp.cc(3309) ICAP cannot keep up with FTP; lost 1448/1448 bytes. This patch adds a little buffering before the BodyPipe, allowing more data to be temporarily stuffed than what fits in the pipe. Needed for FTP directory listings and other sources with no byte-exact flow control. The patch also moves most ICAP dependencies from the protocol implementations to the shared ServerStateData parent, allowing them all to share logics. --- diff --git a/src/Server.cc b/src/Server.cc index 0617d3ff1c..5046f8cbbc 100644 --- a/src/Server.cc +++ b/src/Server.cc @@ -1,5 +1,5 @@ /* - * $Id: Server.cc,v 1.14 2007/06/29 08:31:59 amosjeffries Exp $ + * $Id: Server.cc,v 1.15 2007/07/19 12:07:41 hno Exp $ * * DEBUG: * AUTHOR: Duane Wessels @@ -41,6 +41,8 @@ #if ICAP_CLIENT #include "ICAP/ICAPModXact.h" +#include "ICAP/ICAPConfig.h" +extern ICAPConfig TheICAPConfig; #endif ServerStateData::ServerStateData(FwdState *theFwdState): requestSender(NULL) @@ -71,6 +73,11 @@ ServerStateData::~ServerStateData() #if ICAP_CLIENT cleanIcap(); #endif + + if (responseBodyBuffer != NULL) { + delete responseBodyBuffer; + responseBodyBuffer = NULL; + } } // called when no more server communication is expected; may quit @@ -84,9 +91,22 @@ ServerStateData::serverComplete() assert(doneWithServer()); } + completed = true; + if (requestBodySource != NULL) stopConsumingFrom(requestBodySource); + if (responseBodyBuffer != NULL) + return; + + serverComplete2(); +} + +void +ServerStateData::serverComplete2() +{ + debugs(11,5,HERE << "serverComplete2 " << this); + #if ICAP_CLIENT if (virginBodyDestination != NULL) stopProducingFor(virginBodyDestination, true); @@ -296,6 +316,11 @@ ServerStateData::haveParsedReplyHeaders() // default does nothing } +HttpRequest * +ServerStateData::originalRequest() +{ + return request; +} #if ICAP_CLIENT /* @@ -358,6 +383,13 @@ ServerStateData::doneWithIcap() const { void ServerStateData::noteMoreBodySpaceAvailable(BodyPipe &) { + if (responseBodyBuffer) { + addReplyBody(NULL, 0); // Hack to kick the buffered fragment alive again + if (completed && !responseBodyBuffer) { + serverComplete2(); + return; + } + } maybeReadVirginBody(); } @@ -484,12 +516,6 @@ ServerStateData::handleIcapAborted(bool bypassable) abortTransaction("ICAP failure"); } -HttpRequest * -ServerStateData::originalRequest() -{ - return request; -} - void ServerStateData::icapAclCheckDone(ICAPServiceRep::Pointer service) { @@ -524,4 +550,115 @@ ServerStateData::icapAclCheckDone(ICAPServiceRep::Pointer service) processReplyBody(); } +void +ServerStateData::icapAclCheckDoneWrapper(ICAPServiceRep::Pointer service, void *data) +{ + ServerStateData *state = (ServerStateData *)data; + state->icapAclCheckDone(service); +} #endif + +void +ServerStateData::setReply(HttpReply *reply) +{ + this->reply = reply; + +#if ICAP_CLIENT + + if (TheICAPConfig.onoff) { + ICAPAccessCheck *icap_access_check = + new ICAPAccessCheck(ICAP::methodRespmod, ICAP::pointPreCache, request, reply, icapAclCheckDoneWrapper, this); + + icapAccessCheckPending = true; + icap_access_check->check(); // will eventually delete self + return; + } + +#endif + + entry->replaceHttpReply(reply); + + haveParsedReplyHeaders(); +} + +void +ServerStateData::addReplyBody(const char *data, ssize_t len) +{ + +#if ICAP_CLIENT + + if (virginBodyDestination != NULL) { + if (responseBodyBuffer) { + responseBodyBuffer->append(data, len); + data = responseBodyBuffer->content(); + len = responseBodyBuffer->contentSize(); + } + + const size_t putSize = virginBodyDestination->putMoreData(data, len); + data += putSize; + len -= putSize; + if (responseBodyBuffer) { + responseBodyBuffer->consume(putSize); + if (responseBodyBuffer->contentSize() == 0) { + delete responseBodyBuffer; + responseBodyBuffer = NULL; + } + } else if (len > 0) { + if (!responseBodyBuffer) { + responseBodyBuffer = new MemBuf; + responseBodyBuffer->init(4096, SQUID_TCP_SO_RCVBUF * 10); + } + responseBodyBuffer->append(data, len); + } + return; + } + + // Even if we are done with sending the virgin body to ICAP, we may still + // be waiting for adapted headers. We need them before writing to store. + if (adaptedHeadSource != NULL) { + debugs(11,5, HERE << "need adapted head from " << adaptedHeadSource); + return; + } + +#endif + + if (!len) + return; + + entry->write (StoreIOBuffer(len, currentOffset, (char*)data)); + + currentOffset += len; +} + +size_t ServerStateData::replyBodySpace(size_t space) +{ +#if ICAP_CLIENT + if (responseBodyBuffer) { + return 0; // Stop reading if already overflowed waiting for ICAP to catch up + } + + if (virginBodyDestination != NULL) { + /* + * BodyPipe buffer has a finite size limit. We + * should not read more data from the network than will fit + * into the pipe buffer or we _lose_ what did not fit if + * the response ends sooner that BodyPipe frees up space: + * There is no code to keep pumping data into the pipe once + * response ends and serverComplete() is called. + * + * If the pipe is totally full, don't register the read handler. + * The BodyPipe will call our noteMoreBodySpaceAvailable() method + * when it has free space again. + */ + size_t icap_space = virginBodyDestination->buf().potentialSpaceSize(); + + debugs(11,9, "ServerStateData may read up to min(" << icap_space << + ", " << space << ") bytes"); + + if (icap_space < space) + space = icap_space; + } +#endif + + return space; +} diff --git a/src/Server.h b/src/Server.h index 1b3f26bf72..331b22c93c 100644 --- a/src/Server.h +++ b/src/Server.h @@ -1,6 +1,6 @@ /* - * $Id: Server.h,v 1.5 2007/06/25 22:34:24 rousskov Exp $ + * $Id: Server.h,v 1.6 2007/07/19 12:07:41 hno Exp $ * * AUTHOR: Duane Wessels * @@ -85,11 +85,13 @@ public: // abnormal transaction termination; reason is for debugging only virtual void abortTransaction(const char *reason) = 0; -#if ICAP_CLIENT - void icapAclCheckDone(ICAPServiceRep::Pointer); // a hack to reach HttpStateData::orignal_request virtual HttpRequest *originalRequest(); +#if ICAP_CLIENT + void icapAclCheckDone(ICAPServiceRep::Pointer); + static void icapAclCheckDoneWrapper(ICAPServiceRep::Pointer service, void *data); + // ICAPInitiator: start an ICAP transaction and receive adapted headers. virtual void noteIcapAnswer(HttpMsg *message); virtual void noteIcapQueryAbort(bool final); @@ -103,6 +105,10 @@ public: public: // should be protected void serverComplete(); // call when no server communication is expected +private: + void serverComplete2(); // Continuation of serverComplete + bool completed; // serverComplete() has been called + protected: // kids customize these virtual void haveParsedReplyHeaders(); // default does nothing @@ -140,6 +146,16 @@ protected: void handleIcapAborted(bool bypassable = false); #endif +protected: + // Kids use these to stuff data into the response instead of messing with the entry directly + void setReply(HttpReply *); + void addReplyBody(const char *buf, ssize_t len); + size_t replyBodySpace(size_t space = 4096 * 10); + + // These should be private + off_t currentOffset; // Our current offset in the StoreEntry + MemBuf *responseBodyBuffer; // Data temporarily buffered for ICAP + public: // should not be StoreEntry *entry; FwdState::Pointer fwd; diff --git a/src/ftp.cc b/src/ftp.cc index 15f0f1c07c..33b853592a 100644 --- a/src/ftp.cc +++ b/src/ftp.cc @@ -1,6 +1,6 @@ /* - * $Id: ftp.cc,v 1.428 2007/06/28 14:31:58 rousskov Exp $ + * $Id: ftp.cc,v 1.429 2007/07/19 12:07:41 hno Exp $ * * DEBUG: section 9 File Transfer Protocol (FTP) * AUTHOR: Harvest Derived @@ -55,13 +55,6 @@ #include "SquidTime.h" #include "URLScheme.h" -#if ICAP_CLIENT -#include "ICAP/ICAPConfig.h" -#include "ICAP/ICAPModXact.h" -extern ICAPConfig TheICAPConfig; -static void icapAclCheckDoneWrapper(ICAPServiceRep::Pointer service, void *data); -#endif - static const char *const crlf = "\r\n"; static char cbuf[1024]; @@ -1153,17 +1146,6 @@ FtpStateData::parseListing() assert(t != NULL); -#if ICAP_CLIENT - if (virginBodyDestination != NULL) { - // XXX: There are other places where writeReplyBody may overflow! - if ((int)strlen(t) > virginBodyDestination->buf().potentialSpaceSize()) { - debugs(0,0,HERE << "WARNING avoid overwhelming ICAP with data!"); - usable = s - sbuf; - break; - } - } -#endif - writeReplyBody(t, strlen(t)); } @@ -1221,32 +1203,7 @@ FtpStateData::maybeReadVirginBody() if (data.read_pending) return; - int read_sz = data.readBuf->spaceSize(); - -#if ICAP_CLIENT - // TODO: merge with the same code in HttpStateData::maybeReadVirginBody() - if (virginBodyDestination != NULL) { - /* - * BodyPipe buffer has a finite size limit. We - * should not read more data from the network than will fit - * into the pipe buffer or we _lose_ what did not fit if - * the response ends sooner that BodyPipe frees up space: - * There is no code to keep pumping data into the pipe once - * response ends and serverComplete() is called. - * - * If the pipe is totally full, don't register the read handler. - * The BodyPipe will call our noteMoreBodySpaceAvailable() method - * when it has free space again. - */ - int icap_space = virginBodyDestination->buf().potentialSpaceSize(); - - debugs(11,9, "FTP may read up to min(" << icap_space << - ", " << read_sz << ") bytes"); - - if (icap_space < read_sz) - read_sz = icap_space; - } -#endif + int read_sz = replyBodySpace(data.readBuf->spaceSize()); debugs(11,9, HERE << "FTP may read up to " << read_sz << " bytes"); @@ -3232,25 +3189,7 @@ FtpStateData::appendSuccessHeader() if (mime_enc) reply->header.putStr(HDR_CONTENT_ENCODING, mime_enc); -#if ICAP_CLIENT - - if (TheICAPConfig.onoff) { - ICAPAccessCheck *icap_access_check = new ICAPAccessCheck(ICAP::methodRespmod, - ICAP::pointPreCache, - request, - reply, - icapAclCheckDoneWrapper, - this); - - icapAccessCheckPending = true; - icap_access_check->check(); // will eventually delete self - return; - } - -#endif - - e->replaceHttpReply(reply); - haveParsedReplyHeaders(); + setReply(reply); } void @@ -3341,25 +3280,8 @@ FtpStateData::printfReplyBody(const char *fmt, ...) void FtpStateData::writeReplyBody(const char *data, int len) { -#if ICAP_CLIENT - if (virginBodyDestination != NULL) { - debugs(9,5,HERE << "writing " << len << " bytes to ICAP"); - const size_t putSize = virginBodyDestination->putMoreData(data, len); - if (putSize != (size_t)len) { - // XXX: FTP writing should be rewritten to avoid temporary buffers - // because temporary buffers cannot handle overflows. - debugs(0,0,HERE << "ICAP cannot keep up with FTP; lost " << - (len - putSize) << '/' << len << " bytes."); - } - return; - } -#endif - - debugs(9,5,HERE << "writing " << len << " bytes to StoreEntry"); - - //debugs(9,5,HERE << data); - - entry->append(data, len); + debugs(9,5,HERE << "writing " << len << " bytes to the reply"); + addReplyBody(data, len); } // called after we wrote the last byte of the request body @@ -3429,14 +3351,3 @@ FtpStateData::abortTransaction(const char *reason) fwd->handleUnregisteredServerEnd(); delete this; } - -#if ICAP_CLIENT - -static void -icapAclCheckDoneWrapper(ICAPServiceRep::Pointer service, void *data) -{ - FtpStateData *ftpState = (FtpStateData *)data; - ftpState->icapAclCheckDone(service); -} - -#endif diff --git a/src/http.cc b/src/http.cc index 41a3b3dcda..a84f927d65 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1,6 +1,6 @@ /* - * $Id: http.cc,v 1.531 2007/06/26 00:35:24 rousskov Exp $ + * $Id: http.cc,v 1.532 2007/07/19 12:07:41 hno Exp $ * * DEBUG: section 11 Hypertext Transfer Protocol (HTTP) * AUTHOR: Harvest Derived @@ -55,10 +55,6 @@ #if DELAY_POOLS #include "DelayPools.h" #endif -#if ICAP_CLIENT -#include "ICAP/ICAPConfig.h" -extern ICAPConfig TheICAPConfig; -#endif #include "SquidTime.h" CBDATA_CLASS_INIT(HttpStateData); @@ -70,9 +66,6 @@ static PF httpTimeout; static void httpMaybeRemovePublic(StoreEntry *, http_status); static void copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, String strConnection, HttpRequest * request, HttpRequest * orig_request, HttpHeader * hdr_out, int we_do_ranges, http_state_flags); -#if ICAP_CLIENT -static void icapAclCheckDoneWrapper(ICAPServiceRep::Pointer service, void *data); -#endif HttpStateData::HttpStateData(FwdState *theFwdState) : ServerStateData(theFwdState), header_bytes_read(0), reply_bytes_read(0) @@ -761,25 +754,10 @@ HttpStateData::processReplyHeader() * Parse the header and remove all referenced headers */ -#if ICAP_CLIENT - - if (TheICAPConfig.onoff) { - ICAPAccessCheck *icap_access_check = - new ICAPAccessCheck(ICAP::methodRespmod, ICAP::pointPreCache, request, reply, icapAclCheckDoneWrapper, this); - - icapAccessCheckPending = true; - icap_access_check->check(); // will eventually delete self - ctx_exit(ctx); - return; - } - -#endif - - entry->replaceHttpReply(reply); - - haveParsedReplyHeaders(); + setReply(reply); ctx_exit(ctx); + } // Called when we parsed (and possibly adapted) the headers but @@ -1118,28 +1096,9 @@ HttpStateData::writeReplyBody() const char *data = readBuf->content(); int len = readBuf->contentSize(); -#if ICAP_CLIENT - - if (virginBodyDestination != NULL) { - const size_t putSize = virginBodyDestination->putMoreData(data, len); - readBuf->consume(putSize); - return; - } - - // Even if we are done with sending the virgin body to ICAP, we may still - // be waiting for adapted headers. We need them before writing to store. - if (adaptedHeadSource != NULL) { - debugs(11,5, HERE << "need adapted head from " << adaptedHeadSource); - return; - } - -#endif - - entry->write (StoreIOBuffer(len, currentOffset, (char*)data)); - + addReplyBody(data, len); readBuf->consume(len); - currentOffset += len; } /* @@ -1238,31 +1197,7 @@ HttpStateData::processReplyBody() void HttpStateData::maybeReadVirginBody() { - int read_sz = readBuf->spaceSize(); - -#if ICAP_CLIENT - if (virginBodyDestination != NULL) { - /* - * BodyPipe buffer has a finite size limit. We - * should not read more data from the network than will fit - * into the pipe buffer or we _lose_ what did not fit if - * the response ends sooner that BodyPipe frees up space: - * There is no code to keep pumping data into the pipe once - * response ends and serverComplete() is called. - * - * If the pipe is totally full, don't register the read handler. - * The BodyPipe will call our noteMoreBodySpaceAvailable() method - * when it has free space again. - */ - int icap_space = virginBodyDestination->buf().potentialSpaceSize(); - - debugs(11,9, "HttpStateData may read up to min(" << icap_space << - ", " << read_sz << ") bytes"); - - if (icap_space < read_sz) - read_sz = icap_space; - } -#endif + int read_sz = replyBodySpace(readBuf->spaceSize()); debugs(11,9, HERE << (flags.do_next_read ? "may" : "wont") << " read up to " << read_sz << " bytes from FD " << fd); @@ -1943,20 +1878,8 @@ httpBuildVersion(HttpVersion * version, unsigned int major, unsigned int minor) version->minor = minor; } -#if ICAP_CLIENT - -static void -icapAclCheckDoneWrapper(ICAPServiceRep::Pointer service, void *data) -{ - HttpStateData *http = (HttpStateData *)data; - http->icapAclCheckDone(service); -} - -// TODO: why does FtpStateData not need orig_request? HttpRequest * HttpStateData::originalRequest() { return orig_request; } - -#endif diff --git a/src/http.h b/src/http.h index 258dcfc377..6edca83d18 100644 --- a/src/http.h +++ b/src/http.h @@ -1,6 +1,6 @@ /* - * $Id: http.h,v 1.29 2007/06/25 22:34:24 rousskov Exp $ + * $Id: http.h,v 1.30 2007/07/19 12:07:41 hno Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -39,12 +39,6 @@ #include "forward.h" #include "Server.h" -#if ICAP_CLIENT -#include "ICAP/ICAPServiceRep.h" - -class ICAPAccessCheck; -#endif - class HttpStateData : public ServerStateData { @@ -74,7 +68,6 @@ public: HttpRequest *orig_request; int fd; http_state_flags flags; - off_t currentOffset; size_t read_sz; int header_bytes_read; // to find end of response, int reply_bytes_read; // without relying on StoreEntry @@ -91,9 +84,7 @@ public: const HttpReply * getReply() const { assert(reply); return reply; } protected: -#if ICAP_CLIENT virtual HttpRequest *originalRequest(); -#endif private: enum ConnectionStatus { @@ -127,9 +118,6 @@ private: http_state_flags flags); static bool decideIfWeDoRanges (HttpRequest * orig_request); -#if ICAP_CLIENT -#endif - private: CBDATA_CLASS2(HttpStateData); };