Management of {adapated,virgin}->data->header was becoming a problem.

One side might delete the header before the other side was done with it.
This patch moves deletion of ->data->header to MsgPipeData class so that
it goes away only when the MsgPipe goes away.

diff --git a/src/ICAP/ICAPClientReqmodPrecache.cc b/src/ICAP/ICAPClientReqmodPrecache.cc
index ae91578eca1e06d77139d9a7c0266278c7d0354c..fd81eef4764a2afd0b3f46ab6d34baba6c72d90c 100644 (file)
--- a/src/ICAP/ICAPClientReqmodPrecache.cc
+++ b/src/ICAP/ICAPClientReqmodPrecache.cc
@@ -39,7 +39,7 @@ void ICAPClientReqmodPrecache::startReqMod(ClientHttpRequest *aHttp, HttpRequest
     virgin->source = this;
     virgin->data = new MsgPipeData;
     virgin->data->cause = NULL;
-    virgin->data->header = requestLink(request);
+    virgin->data->setHeader(request);
     virgin->data->body = new MemBuf;
     virgin->data->body->init(ICAP::MsgPipeBufSizeMin, ICAP::MsgPipeBufSizeMax);
 
@@ -182,21 +182,10 @@ void ICAPClientReqmodPrecache::stop(Notify notify)
 void ICAPClientReqmodPrecache::freeVirgin()
 {
     // virgin->data->cause should be NULL;
-
-    if (virgin->data->header) {
-        requestUnlink(dynamic_cast<HttpRequest*>(virgin->data->header));
-        virgin->data->header = NULL;
-    }
-
     virgin = NULL;     // refcounted
 }
 
 void ICAPClientReqmodPrecache::freeAdapted()
 {
-    if (adapted->data->header) {
-        requestUnlink(dynamic_cast<HttpRequest*>(adapted->data->header));
-        adapted->data->header = NULL;
-    }
-
     adapted = NULL;    // refcounted
 }

diff --git a/src/ICAP/ICAPClientRespmodPrecache.cc b/src/ICAP/ICAPClientRespmodPrecache.cc
index f3d8518339cb36f8a3ee4b879d16e03e42633485..c2e3b0ed7b333596c1e8956b6029ba7dbad7900b 100644 (file)
--- a/src/ICAP/ICAPClientRespmodPrecache.cc
+++ b/src/ICAP/ICAPClientRespmodPrecache.cc
@@ -39,8 +39,8 @@ void ICAPClientRespmodPrecache::startRespMod(HttpStateData *anHttpState, HttpReq
     virgin = new MsgPipe("virgin"); // this is the place to create a refcount ptr
     virgin->source = this;
     virgin->data = new MsgPipeData;
-    virgin->data->cause = requestLink(request);
-    virgin->data->header = reply;
+    virgin->data->setCause(request);
+    virgin->data->setHeader(reply);
     virgin->data->body = new MemBuf;
     virgin->data->body->init(ICAP::MsgPipeBufSizeMin, ICAP::MsgPipeBufSizeMax);
 
@@ -49,7 +49,7 @@ void ICAPClientRespmodPrecache::startRespMod(HttpStateData *anHttpState, HttpReq
 #if ICAP_ANCHOR_LOOPBACK
 
     adapted->data = new MsgPipeData;
-    adapted->data->cause = request; // should not hurt
+    adapted->data->setCause(request); // should not hurt
 #else
 
     ICAPInitXaction(service, virgin, adapted);
@@ -88,7 +88,7 @@ void ICAPClientRespmodPrecache::doneSending()
 
 #if ICAP_ANCHOR_LOOPBACK
     /* simple assignments are not the right way to do this */
-    adapted->data->header = virgin->data->header;
+    adapted->data->setHeader(virgin->data->header);
     adapted->data->body = virgin->data->body;
     noteSourceFinish(adapted);
     return;
@@ -125,11 +125,7 @@ void ICAPClientRespmodPrecache::noteSinkAbort(MsgPipe *p)
 // ICAP client has received new HTTP headers (if any) at this point
 void ICAPClientRespmodPrecache::noteSourceStart(MsgPipe *p)
 {
-    debug(93,5)("ICAPClientRespmodPrecache::noteSourceStart() called\n");
-
-    /*
-     * May want to assert that adapted != NULL here
-     */
+    debugs(93,5, HERE << "ICAPClientRespmodPrecache::noteSourceStart() called");
 
     HttpReply *reply = dynamic_cast<HttpReply*>(adapted->data->header);
     /*
@@ -223,26 +219,10 @@ void ICAPClientRespmodPrecache::stop(Notify notify)
 
 void ICAPClientRespmodPrecache::freeVirgin()
 {
-    requestUnlink(virgin->data->cause);
-    virgin->data->cause = NULL;
-    virgin->data->header = NULL;
     virgin = NULL;     // refcounted
 }
 
 void ICAPClientRespmodPrecache::freeAdapted()
 {
-    /*
-     * Note on adapted->data->header.  ICAPXaction-side created it
-     * but gave control of it to us.  Normally we give it to
-     * HttpStateData::takeAdaptedHeader.  If not, we have to
-     * make sure it gets deleted;
-     */
-
-    if (adapted->data->header != NULL) {
-        debug(93,3)("hey, adapted->data->header is still set!\n");
-        delete adapted->data->header;
-        adapted->data->header = NULL;
-    }
-
     adapted = NULL;    // refcounted
 }

diff --git a/src/ICAP/ICAPModXact.cc b/src/ICAP/ICAPModXact.cc
index c1382a991ce08c1a7ebff241157458a128e1c6c1..d5f22a9241a03f3ed09e9497ea98c3024e488820 100644 (file)
--- a/src/ICAP/ICAPModXact.cc
+++ b/src/ICAP/ICAPModXact.cc
@@ -500,15 +500,6 @@ void ICAPModXact::stopSending(bool nicely)
 
     state.sending = State::sendingDone;
 
-    /*
-     * adapted->data->header should be a link_count'ed HttpRequest
-     */
-
-    if (adapted->data->header) {
-        requestUnlink(dynamic_cast<HttpRequest*>(adapted->data->header));
-        adapted->data->header = NULL;
-    }
-
     adapted = NULL; // refcounted
 }
 
@@ -550,9 +541,9 @@ void ICAPModXact::maybeAllocateHttpMsg()
         return;
 
     if (gotEncapsulated("res-hdr")) {
-        adapted->data->header = new HttpReply;
+        adapted->data->setHeader(new HttpReply);
     } else if (gotEncapsulated("req-hdr")) {
-        adapted->data->header = requestLink(new HttpRequest);
+        adapted->data->setHeader(new HttpRequest);
     } else
         throw TexcHere("Neither res-hdr nor req-hdr in maybeAllocateHttpMsg()");
 }

diff --git a/src/ICAP/MsgPipeData.h b/src/ICAP/MsgPipeData.h
index d4949d86cc76b82a01bbdbffbb3b36bf2e843210..99d4fee82660440faf22ab2dd7d3df5572ab6755 100644 (file)
--- a/src/ICAP/MsgPipeData.h
+++ b/src/ICAP/MsgPipeData.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: MsgPipeData.h,v 1.3 2005/12/22 22:26:31 wessels Exp $
+ * $Id: MsgPipeData.h,v 1.4 2006/01/11 22:40:39 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -35,6 +35,8 @@
 #define SQUID_MSGPIPEDATA_H
 
 #include "HttpMsg.h"
+#include "HttpRequest.h"
+#include "HttpReply.h"
 #include "MemBuf.h"
 
 // MsgPipeData contains information about the HTTP message being sent
@@ -52,8 +54,8 @@ public:
 
     ~MsgPipeData()
     {
-        assert(NULL == cause);
-        assert(NULL == header);
+        clearCause();
+        clearHeader();
 
         if (body) {
             body->clean();
@@ -61,6 +63,21 @@ public:
         }
     };
 
+    void setCause(HttpRequest *r)
+    {
+        cause = requestLink(r);
+    };
+
+    void setHeader(HttpMsg *msg)
+    {
+        clearHeader();
+
+        if (HttpRequest *req = dynamic_cast<HttpRequest*>(msg))
+            header = requestLink(req);
+        else if (HttpReply *rep = dynamic_cast<HttpReply*>(msg))
+            header = rep;
+    };
+
 public:
     typedef HttpMsg Header;
     typedef MemBuf Body;
@@ -71,6 +88,23 @@ public:
 
     // HTTP request header for piped responses (the cause of the response)
     HttpRequest *cause;
+
+private:
+
+    void clearCause()
+    {
+        requestUnlink(cause);
+        cause = NULL;
+    };
+
+    void clearHeader()
+    {
+        if (HttpRequest *req = dynamic_cast<HttpRequest*>(header))
+            requestUnlink(req);
+
+        header = NULL;
+    };
+
 };
 
 #endif /* SQUID_MSGPIPEDATA_H */