]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Bug #1921: ftp.cc(3309) ICAP cannot keep up with FTP; lost 1448/1448 bytes.
authorhno <>
Thu, 19 Jul 2007 18:07:41 +0000 (18:07 +0000)
committerhno <>
Thu, 19 Jul 2007 18:07:41 +0000 (18:07 +0000)
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.

src/Server.cc
src/Server.h
src/ftp.cc
src/http.cc
src/http.h

index 0617d3ff1c44e5617caa374fb9f9beef3e90e5c2..5046f8cbbc01c1788fa073ebb6fb32e2544b03c3 100644 (file)
@@ -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;
+}
index 1b3f26bf72c0060a141a2cfe065970c831704416..331b22c93c5f15009b7fb72b84e72a2b6ffb0be1 100644 (file)
@@ -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;
index 15f0f1c07cc4497dcd51f6949712c67a3ddd350b..33b853592a355311bf5c7c75f2fe9ccc8103fc84 100644 (file)
@@ -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
 #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
index 41a3b3dcda2076d01982a165e26535870778bd66..a84f927d65597518e7c28ce80a3de2ff57be289c 100644 (file)
@@ -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
 #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
index 258dcfc3770161d7df8b352e7d56ffcf01dfe494..6edca83d1841f825b7c6425b7ce631293ed95eb2 100644 (file)
@@ -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/
 #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);
 };