---------------------
PatchSet 4023 
Date: 2007/01/29 17:53:00
Author: rousskov
Branch: squid3-icap
Tag: (none) 
Log:
- When informing ClientHttpRequest that we are done, do not stop() because
  our BodyReader may still be working for the http.cc side.

  ICAPClientReqmodPrecache state does not currently reflect BodyReader
  existence, which is likely to cause problems if ICAPClientReqmodPrecache
  is destroyed or cleaned up while http.cc is still using the BodyReader.
  Refcounting alone does not solve this problem because state is more than
  a valid pointer and because BodyReader does not seem to refcount or
  cbdata-protect ICAPClientReqmodPrecache pointer. This needs to be fixed.

- Deleted custom ICAPClientReqmodPrecache::stop() code. I brought that code in
  during ICAPClientVector merge, but I did not notice that it was #ifdef-ed
  and did not bring the #if guards with the code, essentially enabling it.
  Since the code and its comments caused many questions and doubts, it is
  probably better to keep it disabled for now, until the presense of an ICAP
  BodyReader is fully incorporated into ICAPClientReqmodPrecache state
  maintenance.

- Polished debugging.

Members: 
	src/ICAP/ICAPClientReqmodPrecache.cc:1.1.2.5->1.1.2.6 

Index: squid3/src/ICAP/ICAPClientReqmodPrecache.cc
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPClientReqmodPrecache.cc,v
retrieving revision 1.1.2.5
retrieving revision 1.1.2.6
diff -u -r1.1.2.5 -r1.1.2.6
--- squid3/src/ICAP/ICAPClientReqmodPrecache.cc	7 Oct 2006 05:07:18 -0000	1.1.2.5
+++ squid3/src/ICAP/ICAPClientReqmodPrecache.cc	29 Jan 2007 17:53:00 -0000	1.1.2.6
@@ -78,10 +78,10 @@
 void ICAPClientReqmodPrecache::tellDoneAdapting()
 {
     debug(93,3)("ICAPClientReqmodPrecache::tellDoneAdapting() called\n");
-    //tell ClientHttpRequest that we expect no more response data
-    http->doneAdapting(); // does not delete us (yet?)
-    stop(notifyNone);
-    // we should be eventually deleted by owner in ~ClientHttpRequest()
+    // tell ClientHttpRequest that we wil not call it any more
+    http->doneAdapting(); // deletes us (XXX: currently cannot: see below)
+    // We do not stop() here because our BodyReader may still need us.
+    // We need to revise our lifetime management to reflect BodyReader needs.
 }
 
 void ICAPClientReqmodPrecache::tellAbortAdapting()
@@ -94,8 +94,9 @@
 // internal cleanup
 void ICAPClientReqmodPrecache::stop(Notify notify)
 {
+#ifdef DONT_FREE_ADAPTED
     /*
-     * NOTE: We do not clean up "adapted->sink" here because it may
+     * NOTE: We tell clean to ignore the "adapted" pipe because it may
      * have an HTTP message body that needs to stay around a little
      * while longer so that the HTTP server-side can forward it on.
      */
@@ -110,6 +111,9 @@
     // refer to adapted and adapted->data->body?
 
     ICAPClientVector::clean(notify, false);
+#else
+    ICAPClientVector::stop(notify);
+#endif
 }
 
 /*
@@ -122,12 +126,12 @@
 ICAPClientReqmodPrecache::readBody(void *data, MemBuf &mb, size_t size)
 {
     ICAPClientReqmodPrecache *icap = static_cast<ICAPClientReqmodPrecache *>(data);
+    debugs(93,3,HERE << icap << " readBody requested size " << size);
     assert(icap != NULL);
     assert(icap->adapted != NULL);
     assert(icap->adapted->data != NULL);
     MemBuf *bodybuf = icap->adapted->data->body;
     assert(bodybuf != NULL);
-    debugs(93,3,HERE << "readBody requested size " << size);
     debugs(93,3,HERE << "readBody bodybuf size " << bodybuf->contentSize());
 
     if ((mb_size_t) size > bodybuf->contentSize())
@@ -154,6 +158,10 @@
 
     ICAPClientReqmodPrecache *icap = static_cast<ICAPClientReqmodPrecache *>(data);
     icap->stop(notifyIcap);
+    // XXX: Should probably notify the owner as well:
+    // It was not the owner who called abortBody, so the owner probably
+    // does not know what has happened. The owner calls ownerAbort() when
+    // it knows.
 }
 
 /*