Bug #1974 fix: Bypass failures of optional ICAP services.

To bypass various failures, ICAPModXact catches its own exceptions and enables
the "echo" mode instead of quitting. Exceptions are not overruled if the
transaction is retriable. The code disables bypass for essential ICAP services
and when something makes clean echoing impossible (e.g., some buffered HTTP
body content was consumed).

This design allows the same bypass mechanism to be used for moth REQMOD and
RESPMOD, regardless of the vectoring point. Its implementation did not require
changing any Squid core files.

Previously many if not most ICAP failures were not bypassed and users were
receiving cryptic "ICAP protocol error" messages when an optional ICAP
service went down. With the current code, the service may go up and down many
times but only the transactions with large message bodies (that were executing
when the service went down) should be affected.

When deciding on whether to consume HTTP content written to the ICAP server,
Squid has to resolve a trade-off: postponing consumption longer increases
process footprint and may slow the HTTP side down, but consuming sooner
increases the chance of that "ICAP protocol" error being returned to the user.
Since Squid cannot buffer very large messages, some errors are inevitable. We
may want to add knobs to control this tradeoff.

The entries below are only indirectly related to the bug #1974 fix.

virginConsume() does not call or imply checkConsuming(). We must call
checkConsuming() even if we called virginConsume(). Otherwise, we may leave
and get destroyed while the pipe object still holds a [consumer] pointer to
us.

Polished VirginBodyAct so that we can distinguish disabled from undecided
states without using magical negative size constants.

Do not stop writing before throwing an exception. If the exception kills the
transaction, the transaction should cleanup the writer anyway. To bypass an
exception, we need all virgin content intact. Stopping writes before throwing
an exception may consume virgin content because the code does not know that
the exception is about to be thrown and may perceive content as "no longer
needed".

Added debugging.

Merged from the squid3-icap branch.

diff --git a/src/ICAP/ICAPModXact.cc b/src/ICAP/ICAPModXact.cc
index 1732e4026f12a852a66d5cff2dc4ca70f07b1336..3e8466b7456b8c9edc2ece4f2634042674be5098 100644 (file)
--- a/src/ICAP/ICAPModXact.cc
+++ b/src/ICAP/ICAPModXact.cc
@@ -42,7 +42,8 @@ ICAPModXact::ICAPModXact(ICAPInitiator *anInitiator, HttpMsg *virginHeader,
     ICAPXaction("ICAPModXact", anInitiator, aService),
     icapReply(NULL),
     virginConsumed(0),
-    bodyParser(NULL)
+    bodyParser(NULL),
+    canStartBypass(false) // too early
 {
     assert(virginHeader);
 
@@ -70,6 +71,8 @@ void ICAPModXact::start()
 
     estimateVirginBody(); // before virgin disappears!
 
+    canStartBypass = service().bypass;
+
     // it is an ICAP violation to send request to a service w/o known OPTIONS
 
     if (service().up())
@@ -109,7 +112,7 @@ void ICAPModXact::noteServiceReady()
         startWriting();
     } else {
         disableRetries();
-        mustStop("ICAP service unusable");
+        throw TexcHere("ICAP service is unusable");
     }
 
     ICAPXaction_Exit();
@@ -348,6 +351,8 @@ const char *ICAPModXact::virginContentData(const VirginBodyAct &act) const
 
 void ICAPModXact::virginConsume()
 {
+    debugs(93, 9, "consumption guards: " << !virgin.body_pipe << isRetriable);
+
     if (!virgin.body_pipe)
         return; // nothing to consume
 
@@ -355,10 +360,28 @@ void ICAPModXact::virginConsume()
         return; // do not consume if we may have to retry later
 
     BodyPipe &bp = *virgin.body_pipe;
+
+    // Why > 2? HttpState does not use the last bytes in the buffer
+    // because delayAwareRead() is arguably broken. See 
+    // HttpStateData::maybeReadVirginBody for more details.
+    if (canStartBypass && bp.buf().spaceSize() > 2) {
+        // Postponing may increase memory footprint and slow the HTTP side
+        // down. Not postponing may increase the number of ICAP errors 
+        // if the ICAP service fails. We may also use "potential" space to
+        // postpone more aggressively. Should the trade-off be configurable?
+        debugs(93, 8, HERE << "postponing consumption from " << bp.status());
+        return;
+    }
+
     const size_t have = static_cast<size_t>(bp.buf().contentSize());
     const size_t end = virginConsumed + have;
     size_t offset = end;
 
+    debugs(93, 9, HERE << "max virgin consumption offset=" << offset <<
+        " acts " << virginBodyWriting.active() << virginBodySending.active() <<
+        " consumed=" << virginConsumed << 
+        " from " << virgin.body_pipe->status());
+
     if (virginBodyWriting.active())
         offset = XMIN(virginBodyWriting.offset(), offset);
 
@@ -373,6 +396,7 @@ void ICAPModXact::virginConsume()
         bp.consume(size);
         virginConsumed += size;
         Must(!isRetriable); // or we should not be consuming
+        disableBypass("consumed content");
     }
 }
 
@@ -404,15 +428,16 @@ void ICAPModXact::stopWriting(bool nicely)
         // call at any time, usually in the middle of the destruction sequence!
         // Somebody should add comm_remove_write_handler() to comm API.
         reuseConnection = false;
+        ignoreLastWrite = true;
     }
 
     debugs(93, 7, HERE << "will no longer write" << status());
-    state.writing = State::writingReallyDone;
-
     if (virginBodyWriting.active()) {
         virginBodyWriting.disable();
         virginConsume();
     }
+    state.writing = State::writingReallyDone;
+    checkConsuming();
 }
 
 void ICAPModXact::stopBackup()
@@ -489,6 +514,7 @@ void ICAPModXact::echoMore()
            " bytes");
         virginBodySending.progress(size);
         virginConsume();
+        disableBypass("echoed content");
     }
 
     if (virginBodyEndReached(virginBodySending)) {
@@ -507,6 +533,7 @@ bool ICAPModXact::doneSending() const
     return state.sending == State::sendingDone;
 }
 
+// stop (or do not start) sending adapted message body
 void ICAPModXact::stopSending(bool nicely)
 {
     if (doneSending())
@@ -554,6 +581,56 @@ void ICAPModXact::parseMore()
         parseBody();
 }
 
+void ICAPModXact::callException(const TextException &e)
+{
+    if (!canStartBypass || isRetriable) {
+        ICAPXaction::callException(e);
+        return;
+    }
+
+    try {
+        debugs(93, 2, "bypassing ICAPModXact::" << inCall << " exception: " <<
+           e.message << ' ' << status());
+        bypassFailure();
+    }
+    catch (const TextException &bypassE) {
+        ICAPXaction::callException(bypassE);
+    }
+}
+
+void ICAPModXact::bypassFailure()
+{
+    disableBypass("already started to bypass");
+
+    Must(!isRetriable); // or we should not be bypassing
+
+    prepEchoing();
+
+    startSending();
+
+    // end all activities associated with the ICAP server
+
+    stopParsing();
+
+    stopWriting(true); // or should we force it?
+    if (connection >= 0) {
+        reuseConnection = false; // be conservative
+        cancelRead(); // may not work; and we cannot stop connecting either
+        if (!doneWithIo())
+            debugs(93, 7, "Warning: bypass failed to stop I/O" << status());
+    }
+}
+
+void ICAPModXact::disableBypass(const char *reason)
+{
+    if (canStartBypass) {
+        debugs(93,7, HERE << "will never start bypass because " << reason);
+        canStartBypass = false;
+    }
+}
+
+
+
 // note that allocation for echoing is done in handle204NoContent()
 void ICAPModXact::maybeAllocateHttpMsg()
 {
@@ -587,6 +664,13 @@ void ICAPModXact::parseHeaders()
         return;
     }
 
+    startSending();
+}
+
+// called after parsing all headers or when bypassing an exception
+void ICAPModXact::startSending()
+{
+    disableBypass("sent headers");
     sendAnswer(adapted.header);
 
     if (state.sending == State::sendingVirgin)
@@ -687,6 +771,15 @@ void ICAPModXact::handle200Ok()
 void ICAPModXact::handle204NoContent()
 {
     stopParsing();
+    prepEchoing();
+}
+
+// Called when we receive a 204 No Content response and
+// when we are trying to bypass a service failure.
+// We actually start sending (echoig or not) in startSending.
+void ICAPModXact::prepEchoing()
+{
+    disableBypass("preparing to echo content");
 
     // We want to clone the HTTP message, but we do not want
     // to copy some non-HTTP state parts that HttpMsg kids carry in them.
@@ -732,9 +825,11 @@ void ICAPModXact::handle204NoContent()
     if (oldHead->body_pipe != NULL) {
         debugs(93, 7, HERE << "will echo virgin body from " <<
             oldHead->body_pipe);
+        if (!virginBodySending.active())
+            virginBodySending.plan(); // will throw if not possible
         state.sending = State::sendingVirgin;
         checkConsuming();
-        Must(virginBodySending.active());
+
         // TODO: optimize: is it possible to just use the oldHead pipe and
         // remove ICAP from the loop? This echoing is probably a common case!
         makeAdaptedBodyPipe("echoed virgin response");
@@ -850,6 +945,10 @@ void ICAPModXact::parseBody()
     debugs(93, 5, HERE << "have " << readBuf.contentSize() << " body bytes after " <<
            "parse; parsed all: " << parsed);
 
+    // TODO: expose BodyPipe::putSize() to make this check simpler and clearer
+    if (adapted.body_pipe->buf().contentSize() > 0) // parsed something sometime
+        disableBypass("sent adapted content");
+
     if (parsed) {
         stopParsing();
         stopSending(true); // the parser succeeds only if all parsed data fits
@@ -1208,6 +1307,9 @@ void ICAPModXact::fillPendingStatus(MemBuf &buf) const
 
     if (!doneSending() && state.sending != State::sendingUndecided)
         buf.Printf("S(%d)", state.sending);
+
+    if (canStartBypass)
+       buf.append("Y", 1);
 }
 
 void ICAPModXact::fillDoneStatus(MemBuf &buf) const
@@ -1325,31 +1427,32 @@ size_t SizedEstimate::size() const
 
 
 
-VirginBodyAct::VirginBodyAct(): theStart(-1)
+VirginBodyAct::VirginBodyAct(): theStart(0), theState(stUndecided)
 {}
 
 void VirginBodyAct::plan()
 {
-    if (theStart < 0)
-        theStart = 0;
+    Must(!disabled());
+    Must(!theStart); // not started
+    theState = stActive;
 }
 
 void VirginBodyAct::disable()
 {
-    theStart = -2;
+    theState = stDisabled;
 }
 
 void VirginBodyAct::progress(size_t size)
 {
     Must(active());
     Must(size >= 0);
-    theStart += static_cast<ssize_t>(size);
+    theStart += size;
 }
 
 size_t VirginBodyAct::offset() const
 {
     Must(active());
-    return static_cast<size_t>(theStart);
+    return theStart;
 }
 
 

diff --git a/src/ICAP/ICAPModXact.h b/src/ICAP/ICAPModXact.h
index d2739d8af7d9e3902fcd7f82439dfa40bbe049ed..e80729970e28798314206950c4ff1498ba333320 100644 (file)
--- a/src/ICAP/ICAPModXact.h
+++ b/src/ICAP/ICAPModXact.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: ICAPModXact.h,v 1.8 2007/05/08 16:32:11 rousskov Exp $
+ * $Id: ICAPModXact.h,v 1.9 2007/06/19 21:12:15 rousskov Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -81,11 +81,13 @@ class VirginBodyAct
 {
 
 public:
-    VirginBodyAct(); // disabled by default
+    VirginBodyAct();
 
     void plan(); // the activity may happen; do not consume at or above offset
     void disable(); // the activity wont continue; no consumption restrictions
-    bool active() const { return theStart >= 0; } // planned and not disabled
+
+    bool active() const { return theState == stActive; }
+    bool disabled() const { return theState == stDisabled; }
 
     // methods below require active()
 
@@ -93,7 +95,10 @@ public:
     void progress(size_t size); // note processed body bytes
 
 private:
-    ssize_t theStart; // offset, unless negative.
+    size_t theStart; // unprocessed virgin body data offset
+
+    typedef enum { stUndecided, stActive, stDisabled } State;
+    State theState;
 };
 
 
@@ -153,6 +158,10 @@ public:
     ICAPInOut virgin;
     ICAPInOut adapted;
 
+protected:
+    // bypasses exceptions if needed and possible
+    virtual void callException(const TextException &e);
+
 private:
     virtual void start();
 
@@ -213,6 +222,12 @@ private:
     void handle204NoContent();
     void handleUnknownScode();
 
+    void bypassFailure();
+
+    void startSending();
+    void disableBypass(const char *reason);
+
+    void prepEchoing();
     void echoMore();
 
     virtual bool doneAll() const;
@@ -245,6 +260,8 @@ private:
 
     ChunkedCodingParser *bodyParser; // ICAP response body parser
 
+    bool canStartBypass; // enables bypass of transaction failures
+
     class State
     {