BIND 10 trac495, updated. 87308eeddf767dea581b817d1d2ab2aaa4a99dd3 [trac495] update internal documentation of RunningQuery

BIND 10 source code commits bind10-changes at lists.isc.org
Thu Mar 3 10:24:32 UTC 2011


The branch, trac495 has been updated
       via  87308eeddf767dea581b817d1d2ab2aaa4a99dd3 (commit)
      from  e1db14abce5e64985340e4ebb9eb4e7bee56763e (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit 87308eeddf767dea581b817d1d2ab2aaa4a99dd3
Author: Jelte Jansen <jelte at isc.org>
Date:   Thu Mar 3 11:24:17 2011 +0100

    [trac495] update internal documentation of RunningQuery

-----------------------------------------------------------------------

Summary of changes:
 src/lib/resolve/recursive_query.cc                |   81 +++++++++++++++------
 src/lib/resolve/tests/recursive_query_unittest.cc |    1 -
 2 files changed, 60 insertions(+), 22 deletions(-)

-----------------------------------------------------------------------
diff --git a/src/lib/resolve/recursive_query.cc b/src/lib/resolve/recursive_query.cc
index e0474d7..1fd9f9a 100644
--- a/src/lib/resolve/recursive_query.cc
+++ b/src/lib/resolve/recursive_query.cc
@@ -117,11 +117,11 @@ private:
     // we should differentiate between forwarding and resolving
     boost::shared_ptr<AddressVector> upstream_;
 
-    // Buffer to store the result.
+    // Buffer to store the intermediate results.
     OutputBufferPtr buffer_;
 
-    // Server to notify when we succeed or fail
-    //shared_ptr<DNSServer> server_;
+    // The callback will be called when we have either decided we
+    // are done, or when we give up
     isc::resolve::ResolverInterface::CallbackPtr resolvercallback_;
 
     // To prevent both unreasonably long cname chains and cname loops,
@@ -134,7 +134,7 @@ private:
      * TODO Do something more clever with timeouts. In the long term, some
      *     computation of average RTT, increase with each retry, etc.
      */
-    // Timeout information
+    // Timeout information for outgoing queries
     int query_timeout_;
     unsigned retries_;
 
@@ -152,9 +152,9 @@ private:
     // If we timed out ourselves (lookup timeout), stop issuing queries
     bool done_;
 
-    // If we have a client timeout, we send back an answer, but don't
-    // stop. We use this variable to make sure we don't send another
-    // answer if we do find one later (or if we have a lookup_timeout)
+    // If we have a client timeout, we call back with a failure message,
+    // but we do not stop yet. We use this variable to make sure we 
+    // don't call back a second time later
     bool callback_called_;
 
     // Reference to our NSAS
@@ -163,15 +163,38 @@ private:
     // Reference to our cache
     isc::cache::ResolverCache& cache_;
     
-    // the 'current' nameserver we have a query out to
+    // the 'current' zone we are in (i.e.) we start out at the root,
+    // and for each delegation this gets updated with the zone the
+    // delegation points to
     std::string cur_zone_;
+    
+    // This is the handler we pass on to the NSAS; it is called when
+    // the NSAS has an address for us to query
     boost::shared_ptr<ResolverNSASCallback> nsas_callback_;
+
     // this is set to true if we have asked the nsas to give us
-    // an address and we are waiting for it to call us back
+    // an address and we are waiting for it to call us back.
+    // We use is to cancel the outstanding callback in case we
+    // have a lookup timeout and decide to give up
     bool nsas_callback_out_;
+
+    // This is the nameserver we have an outstanding query to.
+    // It is used to update the RTT once the query returns
     isc::nsas::NameserverAddress current_ns_address;
+
+    // The moment in time we sent a query to the nameserver above.
     struct timeval current_ns_qsent_time;
     
+    // RunningQuery deletes itself when it is done. In order for us
+    // to do this safely, we must make sure that there are no events
+    // that might call back to it. There are two types of events in
+    // this sense; the timers we set ourselves (lookup and client),
+    // and outstanding queries to nameservers. When each of these is
+    // started, we increase this value. When they fire, it is decreased
+    // again. We cannot delete ourselves until this value is back to 0.
+    //
+    // Note that the NSAS callback is *not* seen as an outstanding
+    // event; we can cancel the NSAS callback safely.
     size_t outstanding_events_;
 
     // perform a single lookup; first we check the cache to see
@@ -199,6 +222,7 @@ private:
         
     }
 
+    // Send the current question to the given nameserver address
     void sendTo(const isc::nsas::NameserverAddress& address) {
         // We need to keep track of the Address, so that we can update
         // the RTT
@@ -212,6 +236,9 @@ private:
         io_.get_io_service().post(query);
     }
     
+    // 'general' send; if we are in forwarder mode, send a query to
+    // a random nameserver in our forwarders list. If we are in
+    // recursive mode, ask the NSAS to give us an address.
     void send() {
         // If are in forwarder mode, send it to a random
         // forwarder. If not, ask the NSAS for an address
@@ -238,16 +265,20 @@ private:
         }
     }
     
+    // Called by our NSAS callback handler so we know we do not have
+    // an outstanding NSAS call anymore.
     void nsasCallbackCalled() {
         nsas_callback_out_ = false;
     }
 
-    // This function is called by operator() if there is an actual
-    // answer from a server and we are in recursive mode
-    // depending on the contents, we go on recursing or return
+    // This function is called by operator() and lookup();
+    // We have an answer either from a nameserver or the cache, and
+    // we do not know yet if this is a final answer we can send back or
+    // that more recursive processing needs to be done.
+    // Depending on the content, we go on recursing or return
     //
-    // Note that the footprint may change as this function may
-    // need to append data to the answer we are building later.
+    // This method also updates the cache, depending on the content
+    // of the message
     //
     // returns true if we are done (either we have an answer or an
     //              error message)
@@ -361,10 +392,10 @@ private:
             return true;
             break;
         }
-        // should not be reached. assert here?
-        // (since we do not have a default in the switch above,
+
+        // Since we do not have a default in the switch above,
         // the compiler should have errored on any missing case
-        // statements
+        // statements.
         assert(false);
         return true;
     }
@@ -426,6 +457,7 @@ public:
             makeSERVFAIL();
             callCallback(true);
         }
+        assert(outstanding_events_ > 0);
         --outstanding_events_;
         stop();
     }
@@ -437,7 +469,11 @@ public:
             makeSERVFAIL();
             callCallback(true);
         }
+        assert(outstanding_events_ > 0);
         --outstanding_events_;
+        if (outstanding_events_ == 0) {
+            stop();
+        }
     }
 
     // If the callback has not been called yet, call it now
@@ -472,6 +508,8 @@ public:
         }
     }
 
+    // We are done. If there are no more outstanding events, we delete
+    // ourselves. If there are any, we do not.
     void stop() {
         done_ = true;
         if (nsas_callback_out_) {
@@ -490,6 +528,7 @@ public:
     // This function is used as callback from DNSQuery.
     virtual void operator()(IOFetch::Result result) {
         // XXX is this the place for TCP retry?
+        assert(outstanding_events_ > 0);
         --outstanding_events_;
         
         if (!done_ && result != IOFetch::TIME_OUT) {
@@ -528,16 +567,16 @@ public:
                 stop();
             }
         } else if (!done_ && retries_--) {
-            // We timed out, but we have some retries, so send again
+            // Query timed out, but we have some retries, so send again
             dlog("Timeout for " + question_.toText() + " to " + current_ns_address.getAddress().toText() + ", resending query");
             if (recursive_mode()) {
                 current_ns_address.updateRTT(isc::nsas::AddressEntry::UNREACHABLE);
             }
             send();
         } else {
-            // out of retries, give up for now
-            dlog("Timeout for " + question_.toText() + " to " + current_ns_address.getAddress().toText() + ", giving up");
-            if (recursive_mode()) {
+            // We are either already done, or out of retries
+            if (recursive_mode() && result == IOFetch::TIME_OUT) {
+                dlog("Timeout for " + question_.toText() + " to " + current_ns_address.getAddress().toText() + ", giving up");
                 current_ns_address.updateRTT(isc::nsas::AddressEntry::UNREACHABLE);
             }
             if (!callback_called_) {
diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc
index 2cf5963..b005d5b 100644
--- a/src/lib/resolve/tests/recursive_query_unittest.cc
+++ b/src/lib/resolve/tests/recursive_query_unittest.cc
@@ -335,7 +335,6 @@ protected:
             {}
 
             void resume(const bool done) {
-                std::cout << "[XX] RESUME!" << std::endl;
                 *done_ = done;
                 io_.stop();
             }




More information about the bind10-changes mailing list