[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.

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

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,

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,