Bug 4946: client_side_request.cc: "request != newRequest" (#1000)

... assertion when preceded by the following error message:

    ERROR: Inconsistent service method ... in dynamic adaptation chain

The assertion is triggered by the following chain of events. During
SslBump step1, a REQMOD adaptation service returns a dynamic
(X-Next-Services) plan containing a RESPMOD service P. Then, during
SslBump step2 (after obtaining TLS client SNI):

* Adaptation::AccessCheck::start() discovers P in the "future services"
  storage (Adaptation::History::theFutureServices) and returns it.

* The adaptation routing code correctly concludes that P is not
  applicable to the current vectoring point, logs the above ERROR, and
  returns the untouched virgin message object to the adaptation
  initiator. See thePlan.exhausted() in Adaptation::Iterator::step().

* ClientHttpRequest asserts because it expects a new message object.

Fixed Adaptation::AccessCheck code no longer assumes that it cannot be
activated twice for the same vectoring point. It leaves services
applicable to future vectoring points in theFutureServices instead of
always suggesting them for the current vectoring point.

TODO: We can and should optimize adaptation requesting code to stop
requiring a new message object when no adaptation is necessary, but that
change is difficult (we tried!) and independent from the bug fixed here.

diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc
index e32b507843b89b469999a579b15e181a70371737..e3b0db5d77a1347d7a1f74000f09d1c917989128 100644 (file)
--- a/src/adaptation/AccessCheck.cc
+++ b/src/adaptation/AccessCheck.cc
@@ -55,8 +55,7 @@ Adaptation::AccessCheck::AccessCheck(const ServiceFilter &aFilter,
         h->start("ACL");
 #endif
 
-    debugs(93, 5, "AccessCheck constructed for " <<
-           methodStr(filter.method) << " " << vectPointStr(filter.point));
+    debugs(93, 5, "AccessCheck constructed for " << filter);
 }
 
 Adaptation::AccessCheck::~AccessCheck()
@@ -85,10 +84,10 @@ Adaptation::AccessCheck::usedDynamicRules()
     if (!ah)
         return false; // dynamic rules not enabled or not triggered
 
-    DynamicGroupCfg services;
-    if (!ah->extractFutureServices(services)) { // clears history
-        debugs(85,9, "no service-proposed rules stored");
-        return false; // earlier service did not plan for the future
+    const auto services = ah->extractCurrentServices(filter); // updates history
+    if (services.empty()) {
+        debugs(85, 5, "no service-proposed rules for " << filter);
+        return false;
     }
 
     debugs(85,3, "using stored service-proposed rules: " << services);

diff --git a/src/adaptation/DynamicGroupCfg.h b/src/adaptation/DynamicGroupCfg.h
index 1bb1163698a25d75d4a5d55a16ffec33c011cc7e..51eb55d601b2356c2b2b1fe5036b7bf8a4361c07 100644 (file)
--- a/src/adaptation/DynamicGroupCfg.h
+++ b/src/adaptation/DynamicGroupCfg.h
@@ -27,6 +27,10 @@ public:
     Store services; ///< services in the group
 
     bool empty() const { return services.empty(); } ///< no services added
+
+    /// configured service IDs in X-Next-Services value (comma-separated) format
+    const String &serviceIds() const { return id; }
+
     void add(const String &item); ///< updates group id and services
     void clear(); ///< makes the config empty
 };

diff --git a/src/adaptation/History.cc b/src/adaptation/History.cc
index 1381cf4a1333a179f1e3c0dc06684536c23b5b49..e5f4be2b4401edafd26cfb2bf105a0aac15b5bb9 100644 (file)
--- a/src/adaptation/History.cc
+++ b/src/adaptation/History.cc
@@ -9,6 +9,7 @@
 #include "squid.h"
 #include "adaptation/Config.h"
 #include "adaptation/History.h"
+#include "adaptation/ServiceGroups.h"
 #include "base/TextException.h"
 #include "debug/Stream.h"
 #include "globals.h"
@@ -160,13 +161,12 @@ Adaptation::History::setFutureServices(const DynamicGroupCfg &services)
     theFutureServices = services; // may be empty
 }
 
-bool Adaptation::History::extractFutureServices(DynamicGroupCfg &value)
+Adaptation::DynamicGroupCfg
+Adaptation::History::extractCurrentServices(const ServiceFilter &filter)
 {
-    if (theFutureServices.empty())
-        return false;
-
-    value = theFutureServices;
-    theFutureServices.clear();
-    return true;
+    DynamicGroupCfg current, future;
+    DynamicServiceChain::Split(filter, theFutureServices.serviceIds(), current, future);
+    theFutureServices = future; // may already be the same
+    return current; // may be empty
 }
 

diff --git a/src/adaptation/History.h b/src/adaptation/History.h
index 045dd01a7e0914d49c9a931025361169d00df547..27f8903780fdd04101531ee96d6e836eb192acb4 100644 (file)
--- a/src/adaptation/History.h
+++ b/src/adaptation/History.h
@@ -10,6 +10,7 @@
 #define SQUID_ADAPT_HISTORY_H
 
 #include "adaptation/DynamicGroupCfg.h"
+#include "adaptation/forward.h"
 #include "base/RefCount.h"
 #include "HttpHeader.h"
 #include "Notes.h"
@@ -70,8 +71,8 @@ public:
     /// sets future services for the Adaptation::AccessCheck to notice
     void setFutureServices(const DynamicGroupCfg &services);
 
-    /// returns true, fills the value, and resets iff future services were set
-    bool extractFutureServices(DynamicGroupCfg &services);
+    /// returns and forgets planned/future services matching the given filter
+    DynamicGroupCfg extractCurrentServices(const ServiceFilter &);
 
 private:
     /// single Xaction stats (i.e., a historical record entry)

diff --git a/src/adaptation/ServiceFilter.cc b/src/adaptation/ServiceFilter.cc
index 3d24133b074577b2e9cc184d29d6a0264c013b02..1c425842d73d96e7f7d2adbdf7ea7ca4dcabc39e 100644 (file)
--- a/src/adaptation/ServiceFilter.cc
+++ b/src/adaptation/ServiceFilter.cc
@@ -63,3 +63,10 @@ Adaptation::ServiceFilter &Adaptation::ServiceFilter::operator =(const ServiceFi
     return *this;
 }
 
+std::ostream &
+Adaptation::operator <<(std::ostream &os, const ServiceFilter &filter)
+{
+    os << methodStr(filter.method) << ' ' << vectPointStr(filter.point);
+    return os;
+}
+

diff --git a/src/adaptation/ServiceFilter.h b/src/adaptation/ServiceFilter.h
index 8af65ed6da1239f6750d5f58e7b407c5a38ac55b..7c16c979d72f36965c5429aad4ee0561889b3e21 100644 (file)
--- a/src/adaptation/ServiceFilter.h
+++ b/src/adaptation/ServiceFilter.h
@@ -36,6 +36,8 @@ public:
     AccessLogEntry::Pointer al; ///< info for the future access.log entry
 };
 
+std::ostream &operator <<(std::ostream &, const ServiceFilter &);
+
 } // namespace Adaptation
 
 #endif /* SQUID_ADAPTATION__SERVICE_FILTER_H */