---------------------
PatchSet 4050 
Date: 2007/02/14 06:49:37
Author: rousskov
Branch: squid3-icap
Tag: (none) 
Log:
- The minimum options update gap (currently hard-coded) must be smaller than
  the default options TTL. Otherwise, we get stale options and down ICAP
  services around the update time because we cannot update soon enough.

- Synced with ICAPOptXact interface changes. The transaction now sends us
  the options and may send NULL options to indicate a failure.

- Polished debugging.

Members: 
	src/ICAP/ICAPServiceRep.cc:1.1.2.12->1.1.2.13 
	src/ICAP/ICAPServiceRep.h:1.1.2.8->1.1.2.9 

Index: squid3/src/ICAP/ICAPServiceRep.cc
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPServiceRep.cc,v
retrieving revision 1.1.2.12
retrieving revision 1.1.2.13
diff -u -r1.1.2.12 -r1.1.2.13
--- squid3/src/ICAP/ICAPServiceRep.cc	14 Dec 2006 05:17:54 -0000	1.1.2.12
+++ squid3/src/ICAP/ICAPServiceRep.cc	14 Feb 2007 06:49:37 -0000	1.1.2.13
@@ -300,6 +300,9 @@
 
 void ICAPServiceRep::callWhenReady(Callback *cb, void *data)
 {
+    debugs(93,5, HERE << "ICAPService is asked to call " << data <<
+        " when ready " << status());
+
     Must(cb);
     Must(self != NULL);
     Must(!broken()); // we do not wait for a broken service
@@ -408,22 +411,21 @@
 }
 
 static
-void ICAPServiceRep_noteNewOptions(ICAPOptXact *x, void *data)
+void ICAPServiceRep_noteNewOptions(ICAPOptions *newOptions, void *data)
 {
     ICAPServiceRep *service = static_cast<ICAPServiceRep*>(data);
     Must(service);
-    service->noteNewOptions(x);
+    service->noteNewOptions(newOptions);
 }
 
-void ICAPServiceRep::noteNewOptions(ICAPOptXact *x)
+void ICAPServiceRep::noteNewOptions(ICAPOptions *newOptions)
 {
-    Must(x);
+    // newOptions may be NULL
+
     Must(waiting);
     waiting = false;
 
-    changeOptions(x->options);
-    x->options = NULL;
-    delete x;
+    changeOptions(newOptions);
 
     debugs(93,3, "ICAPService got new options and is now " << status());
 
@@ -437,8 +439,8 @@
     debugs(93,6, "ICAPService will get new options " << status());
     waiting = true;
 
-    ICAPOptXact *x = new ICAPOptXact;
-    x->start(self, &ICAPServiceRep_noteNewOptions, this);
+    ICAPOptXact::AsyncStart(
+        new ICAPOptXact(self, &ICAPServiceRep_noteNewOptions, this));
     // TODO: timeout in case ICAPOptXact never calls us back?
 }
 
@@ -458,10 +460,10 @@
         const time_t expire = theOptions->expire();
         debugs(93,7, "ICAPService options expire on " << expire << " >= " << squid_curtime);
 
-        if (expire < 0) // unknown expiration time
-            when = squid_curtime + 60*60;
-        else
-        if (expire < expectedWait) // invalid expiration time
+        // 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;
         else
             when = expire - expectedWait; // before the current options expire
@@ -470,20 +472,20 @@
         when = squid_curtime + TheICAPConfig.service_revival_delay;
     }
 
-    debugs(93,7, "ICAPService options raw update on " << when << " or " << (when - squid_curtime));
+    debugs(93,7, "ICAPService options raw update at " << when << " or in " <<
+        (when - squid_curtime) << " sec");
+
+    /* adjust update time to prevent too-frequent updates */
+
     if (when < squid_curtime)
         when = squid_curtime;
 
-    const int minUpdateGap = 1*60; // seconds
+    const int minUpdateGap = expectedWait + 10; // seconds
     if (when < theLastUpdate + minUpdateGap)
         when = theLastUpdate + minUpdateGap;
 
-    // TODO: keep the time of the last update to prevet too-frequent updates
-
     const int delay = when - squid_curtime;
-
     debugs(93,5, "ICAPService will update options in " << delay << " sec");
-
     eventAdd("ICAPServiceRep::noteTimeToUpdate",
              &ICAPServiceRep_noteTimeToUpdate, this, delay, 0, true);
     updateScheduled = true;
Index: squid3/src/ICAP/ICAPServiceRep.h
===================================================================
RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPServiceRep.h,v
retrieving revision 1.1.2.8
retrieving revision 1.1.2.9
diff -u -r1.1.2.8 -r1.1.2.9
--- squid3/src/ICAP/ICAPServiceRep.h	30 Oct 2006 18:18:48 -0000	1.1.2.8
+++ squid3/src/ICAP/ICAPServiceRep.h	14 Feb 2007 06:49:37 -0000	1.1.2.9
@@ -1,6 +1,6 @@
 
 /*
- * $Id: ICAPServiceRep.h,v 1.1.2.8 2006/10/30 18:18:48 rousskov Exp $
+ * $Id: ICAPServiceRep.h,v 1.1.2.9 2007/02/14 06:49:37 rousskov Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -114,7 +114,7 @@
 public: // treat these as private, they are for callbacks only
     void noteTimeToUpdate();
     void noteTimeToNotify();
-    void noteNewOptions(ICAPOptXact *x);
+    void noteNewOptions(ICAPOptions *newOptions);
 
 private:
     // stores Prepare() callback info