---------------------
PatchSet 4022 
Date: 2007/01/29 17:34:22
Author: rousskov
Branch: squid3-icap
Tag: (none) 
Log:
- When destroying ClientHttpRequest, if ICAPClientReqmodPrecache expects
  ClientHttpRequest to continue, abort the ICAP transaction before destroying
  it. An explicit abort is required to prevent ICAP server-side transaction
  from getting stuck waiting for ICAPClientReqmodPrecache progress, causing
  what looks like a memory leak. Explicit ICAP abort may also be needed to
  correctly handle ICAP BodyReader destruction in the future.

  ICAP classes were written to notify nobody when they are deleted because it
  is not known which side should be notified. An appropriate "done" or "abort"
  method must be called first, for the other side to be notified about the
  complition of the transaction. This does not match the strategy of some
  other Squid classes where the destructor does a lot of notifications and
  other complex work (e.g., see BodyReader). Difference in approaches confuses
  developers (regardless of which strategy is "better").


- When ICAPClientReqmodPrecache calls ClientHttpRequest::doneAdapting, mark
  the icap object as "done" so that we do not try to abort the ICAP
  transaction when destructing ClientHttpRequest. We cannot destroy
  ICAPClientReqmodPrecache at this point because its BodyReader may
  still be needed by the http.cc side.

  It is not clear whether ClientHttpRequest::abortAdapting() should do the
  same. It probably should not because if ICAP is aborting, it should abort
  its BodyReader as well, so it should be safe to delete it.

- Polished debugging.

Members: 
	src/client_side_request.cc:1.34.4.21->1.34.4.22 
	src/client_side_request.h:1.17.12.7->1.17.12.8 

Index: squid3/src/client_side_request.cc
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.cc,v
retrieving revision 1.34.4.21
retrieving revision 1.34.4.22
diff -u -r1.34.4.21 -r1.34.4.22
--- squid3/src/client_side_request.cc	26 Oct 2006 05:40:53 -0000	1.34.4.21
+++ squid3/src/client_side_request.cc	29 Jan 2007 17:34:22 -0000	1.34.4.22
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_request.cc,v 1.34.4.21 2006/10/26 05:40:53 rousskov Exp $
+ * $Id: client_side_request.cc,v 1.34.4.22 2007/01/29 17:34:22 rousskov Exp $
  * 
  * DEBUG: section 85    Client-side Request Routines
  * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c)
@@ -156,7 +156,8 @@
     setConn(aConn);
     dlinkAdd(this, &active, &ClientActiveRequests);
 #if ICAP_CLIENT
-
+    icap = NULL;
+    icapDone = false;
     request_satisfaction_mode = false;
 #endif
 }
@@ -243,8 +244,8 @@
      */
 
     if (request && request->body_reader != NULL) {
+        debugs(32, 3, HERE << "setting body_reader " << request->body_reader.getRaw() << " to NULL for request " << request);
         request->body_reader = NULL;	// refcounted, triggers abort if needed.
-        debugs(32, 3, HERE << "setting body_reader = NULL for request " << request);
     }
 
     /* the ICP check here was erroneous
@@ -260,8 +261,11 @@
     freeResources();
 
 #if ICAP_CLIENT
-    if (icap)
+    if (icap) {
+        if (!icapDone)
+            icap->ownerAbort();
         delete icap;
+    }
 #endif
     if (calloutContext)
         delete calloutContext;
@@ -1262,12 +1266,20 @@
 ClientHttpRequest::doneAdapting()
 {
     debug(85,3)("ClientHttpRequest::doneAdapting() called\n");
+    // Do not delete icap here because the http (server) side
+    // may still need body_reader and body_reader may call
+    // ICAPClientReqmodPrecache for more body.
+    // Hopefully, something will trigger our destruction and we
+    // we will delete icap then.
+    icapDone = true;
 }
 
 void
 ClientHttpRequest::abortAdapting()
 {
     debug(85,3)("ClientHttpRequest::abortAdapting() called\n");
+    delete icap; // we do not mimic doneAdapting here because icap is aborting
+    icap = NULL;
 
     if ((NULL == storeEntry()) || storeEntry()->isEmpty()) {
         debug(85,3)("WARNING: ICAP REQMOD callout failed, proceeding with original request\n");
Index: squid3/src/client_side_request.h
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.h,v
retrieving revision 1.17.12.7
retrieving revision 1.17.12.8
diff -u -r1.17.12.7 -r1.17.12.8
--- squid3/src/client_side_request.h	29 Sep 2006 23:27:11 -0000	1.17.12.7
+++ squid3/src/client_side_request.h	29 Jan 2007 17:34:22 -0000	1.17.12.8
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_request.h,v 1.17.12.7 2006/09/29 23:27:11 dwsquid Exp $
+ * $Id: client_side_request.h,v 1.17.12.8 2007/01/29 17:34:22 rousskov Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -169,6 +169,7 @@
     void abortAdapting();
     bool request_satisfaction_mode;
     off_t request_satisfaction_offset;
+    bool icapDone; // ICAPClientVector has told us that it is finished.
 #endif
 };