[2349] clean up ForwardQuery objects in tests

an attempt to clean up memory; ForwardQuery objects are supposed to delete themselves when they are no longer needed, but due to the direct calls in tests this often does not happen.

To keep internals in recursive_query to a minimum, I did add something to the API; ForwardQuery is now a public class, a pointer of which is returned by RecursiveQuery::forward. Tests that mess around can then delete the query themselves, and the way these are handled does not have to be changed.

Ideally, we should however not need unscoped allocated memory in the first place; we either need to use a centrally managed pool of objects to (re)use, or we need to change the entire design that needs these objects in the first place.

However, these are both non-trivial, and probably better suited in the main resolver work that has been planned.

diff --git a/src/lib/resolve/recursive_query.cc b/src/lib/resolve/recursive_query.cc
index c5b280f5b2727f42a38a607af90331587c93c11d..4f098b93ac33e705cd2dd16eae539deb71be373f 100644 (file)
--- a/src/lib/resolve/recursive_query.cc
+++ b/src/lib/resolve/recursive_query.cc
@@ -122,8 +122,6 @@ deepestDelegation(Name name, RRClass rrclass,
     return (".");
 }
 
-typedef std::vector<std::pair<std::string, uint16_t> > AddressVector;
-
 // Here we do not use the typedef above, as the SunStudio compiler
 // mishandles this in its name mangling, and wouldn't compile.
 // We can probably use a typedef, but need to move it to a central
@@ -898,7 +896,9 @@ public:
     }
 };
 
-class ForwardQuery : public IOFetch::Callback {
+}
+
+class ForwardQuery::ForwardQueryImpl : public IOFetch::Callback {
 private:
     // The io service to handle async calls
     IOService& io_;
@@ -961,7 +961,7 @@ private:
     }
 
 public:
-    ForwardQuery(IOService& io,
+    ForwardQueryImpl(IOService& io,
         ConstMessagePtr query_message,
         MessagePtr answer_message,
         boost::shared_ptr<AddressVector> upstream,
@@ -985,7 +985,7 @@ public:
             lookup_timer.expires_from_now(
                 boost::posix_time::milliseconds(lookup_timeout));
             ++outstanding_events_;
-            lookup_timer.async_wait(boost::bind(&ForwardQuery::lookupTimeout, this));
+            lookup_timer.async_wait(boost::bind(&ForwardQueryImpl::lookupTimeout, this));
         }
 
         // Setup the timer to send an answer (client_timeout)
@@ -993,7 +993,7 @@ public:
             client_timer.expires_from_now(
                 boost::posix_time::milliseconds(client_timeout));
             ++outstanding_events_;
-            client_timer.async_wait(boost::bind(&ForwardQuery::clientTimeout, this));
+            client_timer.async_wait(boost::bind(&ForwardQueryImpl::clientTimeout, this));
         }
 
         send();
@@ -1074,8 +1074,6 @@ public:
     }
 };
 
-}
-
 void
 RecursiveQuery::resolve(const QuestionPtr& question,
     const isc::resolve::ResolverInterface::CallbackPtr callback)
@@ -1189,7 +1187,7 @@ RecursiveQuery::resolve(const Question& question,
     }
 }
 
-void
+ForwardQuery*
 RecursiveQuery::forward(ConstMessagePtr query_message,
     MessagePtr answer_message,
     OutputBufferPtr buffer,
@@ -1215,9 +1213,23 @@ RecursiveQuery::forward(ConstMessagePtr query_message,
     // everything throught without interpretation, except
     // QID, port number. The response will not be cached.
     // It will delete itself when it is done
-    new ForwardQuery(io, query_message, answer_message,
-                      upstream_, buffer, callback, query_timeout_,
-                      client_timeout_, lookup_timeout_);
+    return new ForwardQuery(io, query_message, answer_message,
+                            upstream_, buffer, callback, query_timeout_,
+                            client_timeout_, lookup_timeout_);
+}
+
+ForwardQuery::ForwardQuery(IOService& io,
+        ConstMessagePtr query_message,
+        MessagePtr answer_message,
+        boost::shared_ptr<AddressVector> upstream,
+        OutputBufferPtr buffer,
+        isc::resolve::ResolverInterface::CallbackPtr cb,
+        int query_timeout, int client_timeout, int lookup_timeout) {
+    fqi_ = new ForwardQueryImpl(io, query_message, answer_message, upstream, buffer, cb, query_timeout, client_timeout, lookup_timeout);
+}
+
+ForwardQuery::~ForwardQuery() {
+    delete fqi_;
 }
 
 } // namespace asiodns

diff --git a/src/lib/resolve/recursive_query.h b/src/lib/resolve/recursive_query.h
index a819a94fa8c208d93b1a4a253da3ca3cfd71e91e..2b81145bbc338069a36aa3d16754ccf99804f122 100644 (file)
--- a/src/lib/resolve/recursive_query.h
+++ b/src/lib/resolve/recursive_query.h
@@ -52,6 +52,22 @@ private:
     std::vector<uint32_t>   rtt_;   ///< Stored round-trip times
 };
 
+typedef std::vector<std::pair<std::string, uint16_t> > AddressVector;
+
+class ForwardQuery {
+private:
+    class ForwardQueryImpl;
+    ForwardQueryImpl* fqi_;
+public:
+    ForwardQuery(isc::asiolink::IOService& io,
+                 isc::dns::ConstMessagePtr query_message,
+                 isc::dns::MessagePtr answer_message,
+                 boost::shared_ptr<AddressVector> upstream,
+                 isc::util::OutputBufferPtr buffer,
+                 isc::resolve::ResolverInterface::CallbackPtr cb,
+                 int query_timeout, int client_timeout, int lookup_timeout);
+    ~ForwardQuery();
+};
 
 /// \brief Recursive Query
 ///
@@ -158,7 +174,7 @@ public:
     /// \param server Server object that handles receipt and processing of the
     ///               received messages.
     /// \param callback callback object
-    void forward(isc::dns::ConstMessagePtr query_message,
+    ForwardQuery* forward(isc::dns::ConstMessagePtr query_message,
                  isc::dns::MessagePtr answer_message,
                  isc::util::OutputBufferPtr buffer,
                  DNSServer* server,

diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc
index 4513458ceb24634cd897547a8a64e919ff347f3e..0c9f60e3b1bf5d9940651d6b09fc54ed45ecfe9f 100644 (file)
--- a/src/lib/resolve/tests/recursive_query_unittest.cc
+++ b/src/lib/resolve/tests/recursive_query_unittest.cc
@@ -652,12 +652,12 @@ TEST_F(RecursiveQueryTest, forwarderSend) {
                       singleAddress(TEST_IPV4_ADDR, port));
 
     Question q(Name("example.com"), RRClass::IN(), RRType::TXT());
-    Message query_message(Message::RENDER);
-    isc::resolve::initResponseMessage(q, query_message);
+    MessagePtr query_message(new Message(Message::RENDER));
+    isc::resolve::initResponseMessage(q, *query_message);
 
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    rq.forward(ConstMessagePtr(&query_message), answer, buffer, &server);
+    ForwardQuery* fq = rq.forward(query_message, answer, buffer, &server);
 
     char data[4096];
     size_t size = sizeof(data);
@@ -675,6 +675,8 @@ TEST_F(RecursiveQueryTest, forwarderSend) {
     EXPECT_EQ(q.getName(), q2->getName());
     EXPECT_EQ(q.getType(), q2->getType());
     EXPECT_EQ(q.getClass(), q2->getClass());
+
+    delete fq;
 }
 
 int
@@ -749,14 +751,15 @@ TEST_F(RecursiveQueryTest, forwardQueryTimeout) {
     Question question(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    Message query_message(Message::RENDER);
-    isc::resolve::initResponseMessage(question, query_message);
+    MessagePtr query_message(new Message(Message::RENDER));
+    isc::resolve::initResponseMessage(question, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    query.forward(ConstMessagePtr(&query_message), answer, buffer, &server, callback);
+    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
+    delete fq;
 }
 
 // If we set client timeout to lower than querytimeout, we should
@@ -784,14 +787,15 @@ TEST_F(RecursiveQueryTest, forwardClientTimeout) {
                          1000, 10, 4000, 4);
     Question q(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
-    Message query_message(Message::RENDER);
-    isc::resolve::initResponseMessage(q, query_message);
+    MessagePtr query_message(new Message(Message::RENDER));
+    isc::resolve::initResponseMessage(q, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    query.forward(ConstMessagePtr(&query_message), answer, buffer, &server, callback);
+    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
+    delete fq;
 }
 
 // If we set lookup timeout to lower than querytimeout, the lookup
@@ -819,14 +823,17 @@ TEST_F(RecursiveQueryTest, forwardLookupTimeout) {
     Question question(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
 
-    Message query_message(Message::RENDER);
-    isc::resolve::initResponseMessage(question, query_message);
+    //Message query_message(Message::RENDER);
+    MessagePtr query_message(new Message(Message::RENDER));
+    isc::resolve::initResponseMessage(question, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    query.forward(ConstMessagePtr(&query_message), answer, buffer, &server, callback);
+    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
+    delete fq;
+    std::cout << "[XX] DONE" << std::endl;
 }
 
 // Set everything very low and see if this doesn't cause weird
@@ -854,14 +861,15 @@ TEST_F(RecursiveQueryTest, lowtimeouts) {
     Question question(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
 
-    Message query_message(Message::RENDER);
-    isc::resolve::initResponseMessage(question, query_message);
+    MessagePtr query_message(new Message(Message::RENDER));
+    isc::resolve::initResponseMessage(question, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    query.forward(ConstMessagePtr(&query_message), answer, buffer, &server, callback);
+    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
+    delete fq;
 }
 
 // as mentioned above, we need a more better framework for this,