Bug #2029 fix: When ICAP produces a response before the entire

virgin response is read, do not abort the transaction.

The code needed to be re-arranged to make sure that virgin response body
is not appended to the store when adapted body is written there. The
changes resulted in a cleaner code, where virgin data never enters the
store if ICAP adaptation was started, and adapted data is always
written to the store. More polishing work is needed here.

diff --git a/src/Server.cc b/src/Server.cc
index 291720922095ed5752a8991439ca6dbe7a834bbc..70b07f02a240cfc25fc58edb28a59f12927fc245 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -1,5 +1,5 @@
 /*
- * $Id: Server.cc,v 1.17 2007/07/23 19:58:46 rousskov Exp $
+ * $Id: Server.cc,v 1.18 2007/08/01 04:16:00 rousskov Exp $
  *
  * DEBUG:
  * AUTHOR: Duane Wessels
@@ -381,26 +381,72 @@ ServerStateData::doneWithIcap() const {
         !virginBodyDestination && !adaptedHeadSource && !adaptedBodySource;
 }
 
+// sends virgin reply body to ICAP, buffering excesses if needed
+void
+ServerStateData::adaptVirginReplyBody(const char *data, ssize_t len)
+{
+    assert(startedIcap);
+
+    if (!virginBodyDestination) {
+        debugs(11,3, HERE << "ICAP does not want more virgin body");
+        return;
+    }
+
+    // grow overflow area if already overflowed
+    if (responseBodyBuffer) {
+        responseBodyBuffer->append(data, len);
+        data = responseBodyBuffer->content();
+        len = responseBodyBuffer->contentSize();
+    }
+
+    const ssize_t putSize = virginBodyDestination->putMoreData(data, len);
+    data += putSize;
+    len -= putSize;
+
+    // if we had overflow area, shrink it as necessary
+    if (responseBodyBuffer) {
+        if (putSize == responseBodyBuffer->contentSize()) {
+            delete responseBodyBuffer;
+            responseBodyBuffer = NULL;
+        } else {
+            responseBodyBuffer->consume(putSize);
+               }
+        return;
+    }
+
+    // if we did not have an overflow area, create it as needed
+    if (len > 0) {
+        assert(!responseBodyBuffer);
+        responseBodyBuffer = new MemBuf;
+        responseBodyBuffer->init(4096, SQUID_TCP_SO_RCVBUF * 10);
+        responseBodyBuffer->append(data, len);
+    }
+}
+
 // can supply more virgin response body data
 void
 ServerStateData::noteMoreBodySpaceAvailable(BodyPipe &)
 {
     if (responseBodyBuffer) {
-       addReplyBody(NULL, 0); // Hack to kick the buffered fragment alive again
-       if (completed && !responseBodyBuffer) {
-           serverComplete2();
-           return;
-       }
+        addVirginReplyBody(NULL, 0); // kick the buffered fragment alive again
+        if (completed && !responseBodyBuffer) {
+            serverComplete2();
+            return;
+        }
     }
     maybeReadVirginBody();
 }
 
-// the consumer of our virgin response body aborted, we should too
+// the consumer of our virgin response body aborted
 void
 ServerStateData::noteBodyConsumerAborted(BodyPipe &bp)
 {
     stopProducingFor(virginBodyDestination, false);
-    handleIcapAborted();
+
+    // do not force closeServer here in case we need to bypass IcapQueryAbort
+
+    if (doneWithIcap()) // we may still be receiving adapted response
+        handleIcapCompleted();
 }
 
 // received adapted response headers (body may follow)
@@ -432,7 +478,8 @@ ServerStateData::noteIcapAnswer(HttpMsg *msg)
         assert(adaptedBodySource->setConsumerIfNotLate(this));
     } else {
         // no body
-        handleIcapCompleted();
+        if (doneWithIcap()) // we may still be sending virgin response
+            handleIcapCompleted();
     }
 
 }
@@ -490,10 +537,21 @@ ServerStateData::handleIcapCompleted()
 {
     debugs(11,5, HERE << "handleIcapCompleted");
     cleanIcap();
+
+    // 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()) {
+        debugs(11,3, HERE << "closing origin conn due to ICAP completion");
+        closeServer();
+    }
+
     completeForwarding();
+
     quitIfAllDone();
 }
 
+
 // common part of noteIcap*Aborted and noteBodyConsumerAborted methods
 void
 ServerStateData::handleIcapAborted(bool bypassable)
@@ -526,7 +584,7 @@ ServerStateData::icapAclCheckDone(ICAPServiceRep::Pointer service)
     if (abortOnBadEntry("entry went bad while waiting for ICAP ACL check"))
         return;
 
-    const bool startedIcap = startIcap(service, originalRequest());
+    startedIcap = startIcap(service, originalRequest());
 
     if (!startedIcap && (!service || service->bypass)) {
         // handle ICAP start failure when no service was selected
@@ -582,46 +640,22 @@ ServerStateData::setReply()
 }
 
 void
-ServerStateData::addReplyBody(const char *data, ssize_t len)
+ServerStateData::addVirginReplyBody(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);
+    assert(!icapAccessCheckPending); // or would need to buffer while waiting
+    if (startedIcap) {
+        adaptVirginReplyBody(data, len);
         return;
     }
-
 #endif
+    storeReplyBody(data, len);
+}
 
+// writes virgin or adapted reply body to store
+void
+ServerStateData::storeReplyBody(const char *data, ssize_t len)
+{
     if (!len)
        return;
 

diff --git a/src/Server.h b/src/Server.h
index dab12b0dc0a66d59ea127f73544459092c865922..2fe8ed05501324714c456f7cbb4571f89b5ac61f 100644 (file)
--- a/src/Server.h
+++ b/src/Server.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: Server.h,v 1.7 2007/07/23 16:55:31 rousskov Exp $
+ * $Id: Server.h,v 1.8 2007/08/01 04:16:00 rousskov Exp $
  *
  * AUTHOR: Duane Wessels
  *
@@ -134,6 +134,7 @@ protected:
 
 #if ICAP_CLIENT
     bool startIcap(ICAPServiceRep::Pointer, HttpRequest *cause);
+    void adaptVirginReplyBody(const char *buf, ssize_t len);
     void cleanIcap();
     virtual bool doneWithIcap() const; // did we end ICAP communication?
 
@@ -149,7 +150,8 @@ protected:
 protected:
     // Kids use these to stuff data into the response instead of messing with the entry directly
     void setReply();
-    void addReplyBody(const char *buf, ssize_t len);
+    void addVirginReplyBody(const char *buf, ssize_t len);
+    void storeReplyBody(const char *buf, ssize_t len);
     size_t replyBodySpace(size_t space = 4096 * 10);
 
     // These should be private
@@ -172,6 +174,7 @@ protected:
     BodyPipe::Pointer adaptedBodySource; // to consume adated response body
 
     bool icapAccessCheckPending;
+    bool startedIcap;
 #endif
 
 private:

diff --git a/src/ftp.cc b/src/ftp.cc
index 28250ec188bb28c3115d33c36bb3a83b25027aa9..369fb1d618f99ca92be7b651e318bbe9f7757c1a 100644 (file)
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: ftp.cc,v 1.430 2007/07/23 16:55:31 rousskov Exp $
+ * $Id: ftp.cc,v 1.431 2007/08/01 04:16:00 rousskov Exp $
  *
  * DEBUG: section 9     File Transfer Protocol (FTP)
  * AUTHOR: Harvest Derived
@@ -3281,7 +3281,7 @@ void
 FtpStateData::writeReplyBody(const char *data, int len)
 {
     debugs(9,5,HERE << "writing " << len << " bytes to the reply");
-    addReplyBody(data, len);
+    addVirginReplyBody(data, len);
 }
 
 // called after we wrote the last byte of the request body

diff --git a/src/http.cc b/src/http.cc
index 74539ac5c9344d0ca3e4f03e671ea154935fe5ae..c19e1731c59d1d209210c8d1914f88cbbfb97dba 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: http.cc,v 1.534 2007/07/23 19:58:46 rousskov Exp $
+ * $Id: http.cc,v 1.535 2007/08/01 04:16:00 rousskov Exp $
  *
  * DEBUG: section 11    Hypertext Transfer Protocol (HTTP)
  * AUTHOR: Harvest Derived
@@ -1099,7 +1099,7 @@ HttpStateData::writeReplyBody()
     const char *data = readBuf->content();
     int len = readBuf->contentSize();
 
-    addReplyBody(data, len);
+    addVirginReplyBody(data, len);
     readBuf->consume(len);
 
 }