author: Alex Rousskov <rousskov@measurement-factory.com>, Christos Tsantilas <christo...

Bug 3153 fix: 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 63292c70ad2cd6d566a88b69bfd2928160fcd712..f206555714c778f00b06d4928d987accdf774d23 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -684,8 +684,17 @@ ServerStateData::noteAdaptationAnswer(const Adaptation::Answer &answer)
 void
 ServerStateData::handleAdaptedHeader(HttpMsg *msg)
 {
-    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 84a760f0424b06a8e615df7531408b4fd62fdab8..3d034acaf183e17d1867a49dac6ebaa34829ef03 100644 (file)
--- a/src/adaptation/Initiate.cc
+++ b/src/adaptation/Initiate.cc
@@ -9,6 +9,28 @@
 #include "adaptation/Initiate.h"
 #include "base/AsyncJobCalls.h"
 
+namespace Adaptation
+{
+typedef UnaryMemFunT<Initiator, Answer, const Answer &> AnswerDialer;
+/// Calls expectNoConsumption() if noteAdaptationAnswer async call is
+/// scheduled but never fired (e.g., because the HTTP transaction aborts).
+class AnswerCall: public AsyncCallT<AnswerDialer>{
+public:
+    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();
+    }
+
+private:
+    bool fired; ///< whether we fired the call
+};
+}
 
 Adaptation::Initiate::Initiate(const char *aTypeName): AsyncJob(aTypeName)
 {
@@ -50,9 +72,9 @@ void Adaptation::Initiate::clearInitiator()
 
 void Adaptation::Initiate::sendAnswer(const Answer &answer)
 {
-    typedef UnaryMemFunT<Initiator, Answer, const Answer &> MyDialer;
-    CallJob(93, 5, __FILE__, __LINE__, "Initiator::noteAdaptationAnswer",
-            MyDialer(theInitiator, &Initiator::noteAdaptationAnswer, answer));
+    AsyncCall::Pointer call = new AnswerCall("Initiator::noteAdaptationAnswer",
+                                             AnswerDialer(theInitiator, &Initiator::noteAdaptationAnswer, answer));
+    ScheduleCallHere(call);
     clearInitiator();
 }