From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 17 Jun 2011 13:21:15 +0000 (-0600)
Subject: Bug 3153: Prevent ICAP RESPMOD transactions getting stuck with the adapted body.
X-Git-Tag: SQUID_3_1_12_3~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=96bbf0b785eaa607e0ba58b967beabad3261ed53;p=thirdparty%2Fsquid.git

Bug 3153: Prevent ICAP RESPMOD transactions getting stuck with the adapted body.

Part 1.

  Server is expected to receive adapted response headers and then consume the
  adapted response body, if any. If the server receives the headers and then
  aborts, it must notify the ICAP side that nobody will consume the body.
  Otherwise, the ICAP transaction will fill the BodyPipe buffer and get stuck
  waiting for the consumer to free some space.

Part 2:

  This fix still leaves one potential race condition unhandled: The ICAP
  Initiatee disappears right after sending the adapted headers to the Server
  (because there is nothing else for that initiatee to do). After the
  noteAdaptationAnswer() call is scheduled by ICAP and before it is received by
  the Server job, there is no usable link between Server and ICAP.  There is no
  way for the Server to notify the ICAP transaction that the Server job is
  aborting during that time (and there is no Server job at all after it aborts,
  naturally).

  The solutions is to develop a custom AsyncCall which will call the
  expectNoConsumption() on the message pipe if the call cannot be dialed (i.e.,
  the message cannot be delivered to Server).
---

diff --git a/src/Server.cc b/src/Server.cc
index cdc2a2da53..8cd2678f07 100644
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -648,8 +648,17 @@ ServerStateData::noteAdaptationAnswer(HttpMsg *msg)
 {
     clearAdaptation(adaptedHeadSource); // we do not expect more messages
 
-    if (abortOnBadEntry("entry went bad while waiting for adapted headers"))
+    if (abortOnBadEntry("entry went bad while waiting for adapted headers")) {
+        // If the adapted response has a body, the ICAP side needs to know
+        // that nobody will consume that body. We will be destroyed upon
+        // return. Tell the ICAP side that it is on its own.
+        HttpReply *rep = dynamic_cast<HttpReply*>(msg);
+        assert(rep);
+        if (rep->body_pipe != NULL)
+            rep->body_pipe->expectNoConsumption();
+
         return;
+    }
 
     HttpReply *rep = dynamic_cast<HttpReply*>(msg);
     assert(rep);
diff --git a/src/adaptation/Initiate.cc b/src/adaptation/Initiate.cc
index ffba5bc923..83634b85f3 100644
--- a/src/adaptation/Initiate.cc
+++ b/src/adaptation/Initiate.cc
@@ -27,6 +27,22 @@ private:
     AnswerDialer &operator =(const AnswerDialer &); // not implemented
 };
 
+/// Calls expectNoConsumption() if noteAdaptationAnswer async call is
+/// scheduled but never fired (e.g., because the HTTP transaction aborts).
+class AnswerCall: public AsyncCallT<AnswerDialer>{
+    AnswerCall(const char *aName, const AnswerDialer &aDialer) :
+        AsyncCallT<AnswerDialer>(93, 5, aName, aDialer), fired(false) {}
+    virtual void fire() {
+        fired = true; 
+        AsyncCallT<AnswerDialer>::fire();
+    }
+    virtual ~AnswerCall() {
+        if (!fired && dialer.arg1.message != NULL && dialer.arg1.message->body_pipe != NULL)
+            dialer.arg1.message->body_pipe->expectNoConsumption();
+    }
+    bool fired; ///< whether we fired the call
+};
+
 } // namespace Adaptation
 
 
@@ -73,8 +89,9 @@ void Adaptation::Initiate::clearInitiator()
 void Adaptation::Initiate::sendAnswer(HttpMsg *msg)
 {
     assert(msg);
-    CallJob(93, 5, __FILE__, __LINE__, "Initiator::noteAdaptationAnswer",
-            AnswerDialer(theInitiator, &Initiator::noteAdaptationAnswer, msg));
+    AsyncCall::Pointer call = new AnswerCall("Initiator::noteAdaptationAnswer",
+                                             AnswerDialer(theInitiator, &Initiator::noteAdaptationAnswer, answer));
+    ScheduleCallHere(call);
     clearInitiator();
 }