use the three types of timeout

Note that client_timeout at this point has the same result as
lookup_timeout.

Changed self-deletion of RunningQuery a bit, as it
cannot delete itselfs until we are sure there are not events coming in
anymore. If we change the clientTimeout() callback to send an answer and
not stop, we shall need to keep track of that fact (i.e. not send back
an answer in the end, and update stop since it won't be called by
clientTimeout() anymore

When adding the asiolink tests for these features, added a few
convenience functions for common code in those tests.

src/bin/resolver/resolver.cc

@@ -80,7 +80,11 @@ public:
     void querySetup(DNSService& dnss) {
         assert(!rec_query_); // queryShutdown must be called first
         dlog("Query setup");
-        rec_query_ = new RecursiveQuery(dnss, upstream_, query_timeout_, retries_);
+        rec_query_ = new RecursiveQuery(dnss, upstream_,
+                                        query_timeout_,
+                                        client_timeout_,
+                                        lookup_timeout_,
+                                        retries_);
     }
 
     void queryShutdown() {

src/lib/asiolink/asiolink.cc

@@ -245,9 +245,11 @@ typedef std::vector<std::pair<std::string, uint16_t> > AddressVector;
 }
 
 RecursiveQuery::RecursiveQuery(DNSService& dns_service,
-    const AddressVector& upstream, int timeout, unsigned retries) :
+    const AddressVector& upstream, int query_timeout,
+    int client_timeout, int lookup_timeout, unsigned retries) :
     dns_service_(dns_service), upstream_(new AddressVector(upstream)),
-    timeout_(timeout), retries_(retries)
+    query_timeout_(query_timeout), client_timeout_(client_timeout),
+    lookup_timeout_(lookup_timeout), retries_(retries)
 {}
 
 namespace {
@@ -311,20 +313,29 @@ private:
      *     computation of average RTT, increase with each retry, etc.
      */
     // Timeout information
-    int timeout_;
+    int query_timeout_;
     unsigned retries_;
 
+    deadline_timer client_timer;
+    deadline_timer lookup_timer;
+
+    size_t queries_out_;
+
+    // If we timed out ourselves (lookup timeout), stop issuing queries
+    bool running_;
+
     // (re)send the query to the server.
     void send() {
         const int uc = upstream_->size();
         if (uc > 0) {
+            ++queries_out_;
             int serverIndex = rand() % uc;
             dlog("Sending upstream query (" + question_.toText() +
                 ") to " + upstream_->at(serverIndex).first);
             UDPQuery query(io_, question_,
                 upstream_->at(serverIndex).first,
                 upstream_->at(serverIndex).second, buffer_, this,
-                timeout_);
+                query_timeout_);
             io_.post(query);
         } else {
             dlog("Error, no upstream servers to send to.");
@@ -333,28 +344,76 @@ private:
 public:
     RunningQuery(asio::io_service& io, const Question &question,
         shared_ptr<AddressVector> upstream,
-        OutputBufferPtr buffer, DNSServer* server, int timeout,
+        OutputBufferPtr buffer, DNSServer* server,
+        int query_timeout, int client_timeout, int lookup_timeout,
         unsigned retries) :
         io_(io),
         question_(question),
         upstream_(upstream),
         buffer_(buffer),
         server_(server->clone()),
-        timeout_(timeout),
-        retries_(retries)
+        query_timeout_(query_timeout),
+        retries_(retries),
+        client_timer(io),
+        lookup_timer(io),
+        queries_out_(0),
+        running_(true)
     {
+        // Setup the timer to stop trying (lookup_timeout)
+        if (lookup_timeout >= 0) {
+            lookup_timer.expires_from_now(
+                boost::posix_time::milliseconds(lookup_timeout));
+            lookup_timer.async_wait(boost::bind(&RunningQuery::stop, this, false));
+        }
+        
+        // Setup the timer to send an answer (client_timeout)
+        if (client_timeout >= 0) {
+            client_timer.expires_from_now(
+                boost::posix_time::milliseconds(client_timeout));
+            client_timer.async_wait(boost::bind(&RunningQuery::clientTimeout, this));
+        }
+        
         send();
     }
 
+    virtual void clientTimeout() {
+        // right now, just stop (should make SERVFAIL and send that
+        // back, but not stop)
+        stop(false);
+    }
+
+    virtual void stop(bool resume) {
+        // if we cancel our timers, we will still get an event for
+        // that, so we cannot delete ourselves just yet (those events
+        // would be bound to a deleted object)
+        // cancel them one by one, both cancels should get us back
+        // here again.
+        // same goes if we have an outstanding query (can't delete
+        // until that one comes back to us)
+        running_ = false;
+        server_->resume(resume);
+        if (lookup_timer.cancel() != 0) {
+            return;
+        }
+        if (client_timer.cancel() != 0) {
+            return;
+        }
+        if (queries_out_ > 0) {
+            return;
+        }
+        delete this;
+    }
+
     // This function is used as callback from DNSQuery.
     virtual void operator()(UDPQuery::Result result) {
-        if (result == UDPQuery::TIME_OUT && retries_ --) {
+        --queries_out_;
+        if (running_ && result == UDPQuery::TIME_OUT && retries_ --) {
             dlog("Resending query");
             // We timed out, but we have some retries, so send again
             send();
         } else {
-            server_->resume(result == UDPQuery::SUCCESS);
-            delete this;
+            // We are done
+            stop(result == UDPQuery::SUCCESS);
         }
     }
 };
@@ -372,7 +431,7 @@ RecursiveQuery::sendQuery(const Question& question, OutputBufferPtr buffer,
     asio::io_service& io = dns_service_.get_io_service();
     // It will delete itself when it is done
     new RunningQuery(io, question, upstream_, buffer, server,
-         timeout_, retries_);
+         query_timeout_, client_timeout_, lookup_timeout_, retries_);
 }
 
 class IntervalTimerImpl {

src/lib/asiolink/asiolink.h

@@ -545,7 +545,10 @@ public:
     ///     and return if it returs).
     RecursiveQuery(DNSService& dns_service,
                    const std::vector<std::pair<std::string, uint16_t> >&
-                   upstream, int timeout = -1, unsigned retries = 0);
+                   upstream, int query_timeout = 2000,
+                   int client_timeout = 4000,
+                   int lookup_timeout = 30000,
+                   unsigned retries = 3);
     //@}
 
     /// \brief Initiates an upstream query in the \c RecursiveQuery object.
@@ -565,7 +568,9 @@ private:
     DNSService& dns_service_;
     boost::shared_ptr<std::vector<std::pair<std::string, uint16_t> > >
         upstream_;
-    int timeout_;
+    int query_timeout_;
+    int client_timeout_;
+    int lookup_timeout_;
     unsigned retries_;
 };
 

src/lib/asiolink/tests/asiolink_unittest.cc

@@ -691,20 +691,59 @@ TEST_F(ASIOLinkTest, recursiveSend) {
     EXPECT_EQ(q.getClass(), q2->getClass());
 }
 
-// Test it tries the correct amount of times before giving up
-TEST_F(ASIOLinkTest, recursiveTimeout) {
-    // Prepare the service (we do not use the common setup, we do not answer
-    setDNSService();
-
-    // Prepare the socket
-    res_ = resolveAddress(AF_INET, IPPROTO_UDP, true);
-    sock_ = socket(res_->ai_family, res_->ai_socktype, res_->ai_protocol);
+int
+createTestSocket()
+{
+    struct addrinfo* res_ = resolveAddress(AF_INET, IPPROTO_UDP, true);
+    int sock_ = socket(res_->ai_family, res_->ai_socktype, res_->ai_protocol);
     if (sock_ < 0) {
         isc_throw(IOError, "failed to open test socket");
     }
     if (bind(sock_, res_->ai_addr, res_->ai_addrlen) < 0) {
         isc_throw(IOError, "failed to bind test socket");
     }
+    return sock_;
+}
+
+int
+setSocketTimeout(int sock_, size_t tv_sec, size_t tv_usec) {
+    const struct timeval timeo = { tv_sec, tv_usec };
+    int recv_options = 0;
+    if (setsockopt(sock_, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo))) {
+        if (errno == ENOPROTOOPT) { // see ASIOLinkTest::recvUDP()
+            recv_options = MSG_DONTWAIT;
+        } else {
+            isc_throw(IOError, "set RCVTIMEO failed: " << strerror(errno));
+        }
+    }
+    return recv_options;
+}
+
+// try to read from the socket max time
+// *num is incremented for every succesfull read
+// returns true if it can read max times, false otherwise
+bool tryRead(int sock_, int recv_options, size_t max, int* num) {
+    size_t i = 0;
+    do {
+        char inbuff[512];
+        if (recv(sock_, inbuff, sizeof(inbuff), recv_options) < 0) {
+            return false;
+        } else {
+            ++i;
+            ++*num;
+        }
+    } while (i < max);
+    return true;
+}
+
+
+// Test it tries the correct amount of times before giving up
+TEST_F(ASIOLinkTest, forwardQueryTimeout) {
+    // Prepare the service (we do not use the common setup, we do not answer
+    setDNSService();
+
+    // Prepare the socket
+    sock_ = createTestSocket();
 
     // Prepare the server
     bool done(true);
@@ -713,7 +752,9 @@ TEST_F(ASIOLinkTest, recursiveTimeout) {
     // Do the answer
     const uint16_t port = boost::lexical_cast<uint16_t>(TEST_CLIENT_PORT);
     RecursiveQuery query(*dns_service_, singleAddress(TEST_IPV4_ADDR, port),
-        10, 2);
+        10, 4000, 3000, 2);
+
+    // Just send some query
     Question question(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
     query.sendQuery(question, buffer, &server);
@@ -723,27 +764,95 @@ TEST_F(ASIOLinkTest, recursiveTimeout) {
 
     // Read up to 3 packets.  Use some ad hoc timeout to prevent an infinite
     // block (see also recvUDP()).
-    const struct timeval timeo = { 10, 0 };
-    int recv_options = 0;
-    if (setsockopt(sock_, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo))) {
-        if (errno == ENOPROTOOPT) { // see ASIOLinkTest::recvUDP()
-            recv_options = MSG_DONTWAIT;
-        } else {
-            isc_throw(IOError, "set RCVTIMEO failed: " << strerror(errno));
-        }
-    }
+    int recv_options = setSocketTimeout(sock_, 10, 0);
     int num = 0;
-    do {
-        char inbuff[512];
-        if (recv(sock_, inbuff, sizeof(inbuff), recv_options) < 0) {
-            num = -1;
-            break;
-        }
-    } while (++num < 3);
+    bool read_success = tryRead(sock_, recv_options, 3, &num);
+
+    // The query should fail
+    EXPECT_FALSE(done);
+    EXPECT_EQ(3, num);
+    EXPECT_EQ(true, read_success);
+}
+
+// If we set client timeout to lower than querytimeout, we should
+// get a failure answer, but still see retries
+// (no actual answer is given here yet)
+TEST_F(ASIOLinkTest, forwardClientTimeout) {
+    // Prepare the service (we do not use the common setup, we do not answer
+    setDNSService();
+
+    sock_ = createTestSocket();
+
+    // Prepare the server
+    bool done(true);
+    MockServerStop server(*io_service_, &done);
+
+    // Do the answer
+    const uint16_t port = boost::lexical_cast<uint16_t>(TEST_CLIENT_PORT);
+    // Set it up to retry twice before client timeout fires
+    // Since the lookup timer has not fired, it should retry
+    // a third time
+    RecursiveQuery query(*dns_service_, singleAddress(TEST_IPV4_ADDR, port),
+        5, 12, 1000, 3);
+    Question question(Name("example.net"), RRClass::IN(), RRType::A());
+    OutputBufferPtr buffer(new OutputBuffer(0));
+    query.sendQuery(question, buffer, &server);
+
+    // Run the test
+    io_service_->run();
+
+    // we know it'll fail, so make it a shorter timeout
+    int recv_options = setSocketTimeout(sock_, 1, 0);
+
+    // Try to read 5 times, should stop after 3 reads
+    int num = 0;
+    bool read_success = tryRead(sock_, recv_options, 5, &num);
+
+    // The query should fail (for resolver it should send back servfail,
+    // but currently, and perhaps for forwarder in general, the effect
+    // will be the same as on a lookup timeout, i.e. no answer is sent
+    // back)
+    EXPECT_FALSE(done);
+    EXPECT_EQ(3, num);
+    EXPECT_EQ(false, read_success);
+}
+
+// If we set lookup timeout to lower than querytimeout*retries, we should
+// fail before the full amount of retries
+TEST_F(ASIOLinkTest, forwardLookupTimeout) {
+    // Prepare the service (we do not use the common setup, we do not answer
+    setDNSService();
+
+    // Prepare the socket
+    sock_ = createTestSocket();
+
+    // Prepare the server
+    bool done(true);
+    MockServerStop server(*io_service_, &done);
+
+    // Do the answer
+    const uint16_t port = boost::lexical_cast<uint16_t>(TEST_CLIENT_PORT);
+    // Set up the test so that it will retry 5 times, but the lookup
+    // timeout will fire after only 3 normal timeouts
+    RecursiveQuery query(*dns_service_, singleAddress(TEST_IPV4_ADDR, port),
+        5, 4000, 12, 5);
+    Question question(Name("example.net"), RRClass::IN(), RRType::A());
+    OutputBufferPtr buffer(new OutputBuffer(0));
+    query.sendQuery(question, buffer, &server);
+
+    // Run the test
+    io_service_->run();
+
+    int recv_options = setSocketTimeout(sock_, 1, 0);
+
+    // Try to read 5 times, should stop after 3 reads
+    int num = 0;
+    bool read_success = tryRead(sock_, recv_options, 5, &num);
 
     // The query should fail
     EXPECT_FALSE(done);
     EXPECT_EQ(3, num);
+    EXPECT_EQ(false, read_success);
 }
 
 // This fixture is for testing IntervalTimer. Some callback functors are