From: rousskov <>
Date: Wed, 20 Jun 2007 03:13:49 +0000 (+0000)
Subject: Partial bug #1973 fix: Try to revive a suspended service after the
X-Git-Tag: SQUID_3_0_PRE7~209
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1299ecbf9d2e38bc6cbfa3ebb50d1ba28d7716ee;p=thirdparty%2Fsquid.git

Partial bug #1973 fix: Try to revive a suspended service after the
icap_service_revival_delay.

When a service gets suspended, schedule an OPTIONS update after the
icap_service_revival_delay. If there is already an OPTIONS fetch scheduled,
that outdated event will be deleted using eventDelete().

Eventually, we may want to have a separate option to control suspended
service revival.

Polished code, comments, and debugging.

Merged from the squid3-icap branch.
---

diff --git a/src/ICAP/ICAPServiceRep.cc b/src/ICAP/ICAPServiceRep.cc
index 2f8b0414bd..8553fca819 100644
--- a/src/ICAP/ICAPServiceRep.cc
+++ b/src/ICAP/ICAPServiceRep.cc
@@ -175,8 +175,11 @@ void ICAPServiceRep::invalidate()
 
 void ICAPServiceRep::noteFailure() {
     ++theSessionFailures;
-    debugs(93,4, "ICAPService failure " << theSessionFailures <<
-        ", out of " << TheICAPConfig.service_failure_limit << " allowed");
+    debugs(93,4, theSessionFailures << " ICAPService failures, out of " << 
+        TheICAPConfig.service_failure_limit << " allowed " << status());
+
+    if (isSuspended)
+        return;
 
     if (TheICAPConfig.service_failure_limit >= 0 &&
         theSessionFailures > TheICAPConfig.service_failure_limit)
@@ -194,6 +197,7 @@ void ICAPServiceRep::suspend(const char *reason) {
     } else {
         isSuspended = reason;
         debugs(93,1, "suspending ICAPService for " << reason);
+        scheduleUpdate(squid_curtime + TheICAPConfig.service_revival_delay);
         announceStatusChange("suspended", true);
     }
 }
@@ -453,7 +457,7 @@ void ICAPServiceRep::handleNewOptions(ICAPOptions *newOptions)
 
     debugs(93,3, "ICAPService got new options and is now " << status());
 
-    scheduleUpdate();
+    scheduleUpdate(optionsFetchTime());
     scheduleNotification();
 }
 
@@ -468,53 +472,67 @@ void ICAPServiceRep::startGettingOptions()
     // Such a timeout should probably be a generic AsyncStart feature.
 }
 
-void ICAPServiceRep::scheduleUpdate()
+void ICAPServiceRep::scheduleUpdate(time_t when)
 {
-    if (updateScheduled)
-        return; // already scheduled
-
-    // XXX: move hard-coded constants from here to TheICAPConfig
-
-    // conservative estimate of how long the OPTIONS transaction will take
-    const int expectedWait = 20; // seconds
-
-    time_t when = 0;
-
-    if (theOptions && theOptions->valid()) {
-        const time_t expire = theOptions->expire();
-        debugs(93,7, "ICAPService options expire on " << expire << " >= " << squid_curtime);
-
-        // Unknown or invalid (too small) expiration times should not happen.
-        // ICAPOptions should use the default TTL, and ICAP servers should not
-        // send invalid TTLs, but bugs and attacks happen.
-        if (expire < expectedWait)
-            when = squid_curtime + 60*60;
+    if (updateScheduled) {
+        debugs(93,7, "ICAPService reschedules update");
+        // XXX: check whether the event is there because AR saw
+		// an unreproducible eventDelete assertion on 2007/06/18
+        if (eventFind(&ICAPServiceRep_noteTimeToUpdate, this))
+			eventDelete(&ICAPServiceRep_noteTimeToUpdate, this);
         else
-            when = expire - expectedWait; // before the current options expire
-    } else {
-        // delay for a down service
-        when = squid_curtime + TheICAPConfig.service_revival_delay;
+            debugs(93,1, "XXX: ICAPService lost an update event.");
+        updateScheduled = false;
     }
 
-    debugs(93,7, "ICAPService options raw update at " << when << " or in " <<
+    debugs(93,7, HERE << "raw OPTIONS fetch at " << when << " or in " <<
         (when - squid_curtime) << " sec");
+    debugs(93,9, HERE << "last fetched at " << theLastUpdate << " or " <<
+        (squid_curtime - theLastUpdate) << " sec ago");
 
     /* adjust update time to prevent too-frequent updates */
 
     if (when < squid_curtime)
         when = squid_curtime;
 
-    const int minUpdateGap = expectedWait + 10; // seconds
+    // XXX: move hard-coded constants from here to TheICAPConfig
+    const int minUpdateGap = 30; // seconds
     if (when < theLastUpdate + minUpdateGap)
         when = theLastUpdate + minUpdateGap;
 
     const int delay = when - squid_curtime;
-    debugs(93,5, "ICAPService will update options in " << delay << " sec");
+    debugs(93,5, "ICAPService will fetch OPTIONS in " << delay << " sec");
+
     eventAdd("ICAPServiceRep::noteTimeToUpdate",
              &ICAPServiceRep_noteTimeToUpdate, this, delay, 0, true);
     updateScheduled = true;
 }
 
+// returns absolute time when OPTIONS should be fetched
+time_t
+ICAPServiceRep::optionsFetchTime() const
+{
+    if (theOptions && theOptions->valid()) {
+        const time_t expire = theOptions->expire();
+        debugs(93,7, "ICAPService options expire on " << expire << " >= " << squid_curtime);
+
+        // conservative estimate of how long the OPTIONS transaction will take
+        // XXX: move hard-coded constants from here to TheICAPConfig
+        const int expectedWait = 20; // seconds
+
+        // Unknown or invalid (too small) expiration times should not happen.
+        // ICAPOptions should use the default TTL, and ICAP servers should not
+        // send invalid TTLs, but bugs and attacks happen.
+        if (expire < expectedWait)
+            return squid_curtime;
+        else
+            return expire - expectedWait; // before the current options expire
+    }
+
+    // use revival delay as "expiration" time for a service w/o valid options
+    return squid_curtime + TheICAPConfig.service_revival_delay;
+}
+
 // returns a temporary string depicting service status, for debugging
 const char *ICAPServiceRep::status() const
 {
diff --git a/src/ICAP/ICAPServiceRep.h b/src/ICAP/ICAPServiceRep.h
index 128ea76c78..755f26f9ac 100644
--- a/src/ICAP/ICAPServiceRep.h
+++ b/src/ICAP/ICAPServiceRep.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: ICAPServiceRep.h,v 1.9 2007/05/29 13:31:44 amosjeffries Exp $
+ * $Id: ICAPServiceRep.h,v 1.10 2007/06/19 21:13:49 rousskov Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -47,10 +47,13 @@ class ICAPOptXact;
    host many ICAP services. */
 
 /*
- * A service is "up" if there is a fresh cached OPTIONS response and is
- * "down" otherwise. A service is "probed" if we tried to get an OPTIONS
- * response from it and succeeded or failed. A probed down service is
- * called "broken".
+ * A service with a fresh cached OPTIONS response and without many failures
+ * is an "up" service. All other services are "down". A service is "probed"
+ * if we tried to get an OPTIONS response from it and succeeded or failed.
+ * A probed down service is called "broken".
+ *
+ * The number of failures required to bring an up service down is determined
+ * by icap_service_failure_limit in squid.conf.
  *
  * As a bootstrapping mechanism, ICAP transactions wait for an unprobed
  * service to get a fresh OPTIONS response (see the callWhenReady method).
@@ -151,8 +154,9 @@ private:
 
     bool hasOptions() const;
     bool needNewOptions() const;
+    time_t optionsFetchTime() const;
 
-    void scheduleUpdate();
+    void scheduleUpdate(time_t when);
     void scheduleNotification();
 
     void startGettingOptions();