---------------------
PatchSet 4109 
Date: 2007/03/21 20:05:14
Author: rousskov
Branch: squid3-icap
Tag: (none) 
Log:
- Removed unreachable code after the doIcap() call. The latter never
  fails.

- Replaced never-failing doIcap() with startIcap() that fails if we
  cannot select an ICAP service or the selected service is not usable.
  Rearranged ClientRequestContext::icapAclCheckDone() to bypass ICAP
  errors when possible.

  Now, ClientRequestContext::startIcap() is very similar to
  Server::startIcap(). Same for icapAclCheckDone().

  Made ClientHttpRequest::handleIcapFailure() public because
  ClientRequestContext::icapAclCheckDone() calls it.

- Fixed "is it too late to bypass?" conditions in
  ClientHttpRequest::handleIcapFailure(). We should be able to bypass
  more often now. However, handleIcapFailure() still has the old bug: it
  does not check whether the service is optional. The current fix
  implies that now Squid may bypass essential services more often.

Members: 
	src/client_side_request.cc:1.34.4.26->1.34.4.27 
	src/client_side_request.h:1.17.12.9->1.17.12.10 

Index: squid3/src/client_side_request.cc
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.cc,v
retrieving revision 1.34.4.26
retrieving revision 1.34.4.27
diff -u -r1.34.4.26 -r1.34.4.27
--- squid3/src/client_side_request.cc	8 Mar 2007 22:09:35 -0000	1.34.4.26
+++ squid3/src/client_side_request.cc	21 Mar 2007 20:05:14 -0000	1.34.4.27
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_request.cc,v 1.34.4.26 2007/03/08 22:09:35 rousskov Exp $
+ * $Id: client_side_request.cc,v 1.34.4.27 2007/03/21 20:05:14 rousskov Exp $
  * 
  * DEBUG: section 85    Client-side Request Routines
  * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c)
@@ -511,35 +511,20 @@
 ClientRequestContext::icapAclCheckDone(ICAPServiceRep::Pointer service)
 {
     debugs(93,3,HERE << this << " icapAclCheckDone called");
-    /*
-     * No matching ICAP service in the config file
-     */
-
-    if (service == NULL) {
-        http->doCallouts();
-        return;
-    }
-
-    /*
-     * Setup ICAP state and such.  If successful, just return.
-     * We'll get back to doCallouts() after REQMOD is done.
-     */
     assert(http);
 
-    if (0 == http->doIcap(service))
+    if (http->startIcap(service))
         return;
 
-    /*
-     * If doIcap() fails, then we have to either return an error
-     * to the user, or keep going without ICAP.
-     */
-    fatal("Fix this case in ClientRequestContext::icapAclCheckDone()");
-
-    // And when fixed, check whether the service is down in doIcap and
-    // if it is, abort early, without creating ICAPModXact.
-    // See Server::startIcap() and its use.
+    if (!service || service->bypass) {
+        // handle ICAP start failure when no service was selected
+        // or where the selected service was optional
+        http->doCallouts();
+        return;
+    }
 
-    http->doCallouts();
+    // handle start failure for an essential ICAP service
+    http->handleIcapFailure();
 }
 
 #endif
@@ -1085,19 +1070,27 @@
 
 #if ICAP_CLIENT
 /*
- * Initiate an ICAP transaction.  Return 0 if all is well, or -1 upon error.
- * Caller will handle error condition by generating a Squid error message
- * or take other action.
+ * Initiate an ICAP transaction.  Return false on errors. 
+ * The caller must handle errors.
  */
-int
-ClientHttpRequest::doIcap(ICAPServiceRep::Pointer service)
+bool
+ClientHttpRequest::startIcap(ICAPServiceRep::Pointer service)
 {
-    debugs(85, 3, HERE << this << " ClientHttpRequest::doIcap() called");
+    debugs(85, 3, HERE << this << " ClientHttpRequest::startIcap() called");
+    if (!service) {
+        debug(85,3)("ClientHttpRequest::startIcap fails: lack of service\n");
+        return false;
+    }
+    if (service->broken()) {
+        debug(85,3)("ClientHttpRequest::startIcap fails: broken service\n");
+        return false;
+    }
+
     assert(!icapHeadSource);
     assert(!icapBodySource);
     icapHeadSource = new ICAPModXact(this, request, NULL, service);
     ICAPModXact::AsyncStart(icapHeadSource.getRaw());
-    return 0;
+    return true;
 }
 
 void
@@ -1212,9 +1205,9 @@
 {
     debugs(85,3, HERE << "handleIcapFailure");
 
-    const bool usedStore = !storeEntry() || storeEntry()->isEmpty();
-    const bool usedPipe = !request->body_pipe ||
-        !request->body_pipe->consumedSize();
+    const bool usedStore = storeEntry() && !storeEntry()->isEmpty();
+    const bool usedPipe = request->body_pipe != NULL &&
+        request->body_pipe->consumedSize() > 0;
 
     // XXX: we must not try to recover if the ICAP service is not bypassable!
 
Index: squid3/src/client_side_request.h
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.h,v
retrieving revision 1.17.12.9
retrieving revision 1.17.12.10
diff -u -r1.17.12.9 -r1.17.12.10
--- squid3/src/client_side_request.h	14 Feb 2007 07:19:43 -0000	1.17.12.9
+++ squid3/src/client_side_request.h	21 Mar 2007 20:05:15 -0000	1.17.12.10
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_request.h,v 1.17.12.9 2007/02/14 07:19:43 rousskov Exp $
+ * $Id: client_side_request.h,v 1.17.12.10 2007/03/21 20:05:15 rousskov Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -162,7 +162,8 @@
 #if ICAP_CLIENT
 
 public:
-    int doIcap(ICAPServiceRep::Pointer);
+    bool startIcap(ICAPServiceRep::Pointer);
+    void handleIcapFailure(); // private but exposed for ClientRequestContext
 
 private:
     // ICAPInitiator API, called by ICAPXaction
@@ -175,7 +176,6 @@
     virtual void noteBodyProducerAborted(BodyPipe &);
 
     void endRequestSatisfaction();
-    void handleIcapFailure();
 
 private:
     ICAPModXact::Pointer icapHeadSource;