[2349] Revert pimpl classes, make abstract base class

saves a second dynamic allocation (and more importantly, one that couldn't be freed)

src/lib/resolve/recursive_query.cc

@@ -158,6 +158,7 @@ RecursiveQuery::setRttRecorder(boost::shared_ptr<RttRecorder>& recorder) {
     rtt_recorder_ = recorder;
 }
 
+namespace {
 typedef std::pair<std::string, uint16_t> addr_t;
 
 /*
@@ -167,11 +168,11 @@ typedef std::pair<std::string, uint16_t> addr_t;
  *
  * Used by RecursiveQuery::sendQuery.
  */
-class RunningQuery::RunningQueryImpl : public IOFetch::Callback {
+class RunningQuery : public IOFetch::Callback, public AbstractRunningQuery {
 
 class ResolverNSASCallback : public isc::nsas::AddressRequestCallback {
 public:
-    ResolverNSASCallback(RunningQueryImpl* rq) : rq_(rq) {}
+    ResolverNSASCallback(RunningQuery* rq) : rq_(rq) {}
 
     void success(const isc::nsas::NameserverAddress& address) {
         // Success callback, send query to found namesever
@@ -191,7 +192,7 @@ public:
     }
 
 private:
-    RunningQueryImpl* rq_;
+    RunningQuery* rq_;
 };
 
 
@@ -292,7 +293,7 @@ private:
     // The moment in time we sent a query to the nameserver above.
     struct timeval current_ns_qsent_time;
 
-    // RunningQueryImpl deletes itself when it is done. In order for us
+    // 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),
@@ -669,7 +670,7 @@ SERVFAIL:
     }
 
 public:
-    RunningQueryImpl(IOService& io,
+    RunningQuery(IOService& io,
         const Question& question,
         MessagePtr answer_message,
         std::pair<std::string, uint16_t>& test_server,
@@ -712,7 +713,7 @@ public:
             lookup_timer.expires_from_now(
                 boost::posix_time::milliseconds(lookup_timeout));
             ++outstanding_events_;
-            lookup_timer.async_wait(boost::bind(&RunningQueryImpl::lookupTimeout, this));
+            lookup_timer.async_wait(boost::bind(&RunningQuery::lookupTimeout, this));
         }
 
         // Setup the timer to send an answer (client_timeout)
@@ -720,12 +721,14 @@ public:
             client_timer.expires_from_now(
                 boost::posix_time::milliseconds(client_timeout));
             ++outstanding_events_;
-            client_timer.async_wait(boost::bind(&RunningQueryImpl::clientTimeout, this));
+            client_timer.async_wait(boost::bind(&RunningQuery::clientTimeout, this));
         }
 
         doLookup();
     }
 
+    virtual ~RunningQuery() {};
+
     // called if we have a lookup timeout; if our callback has
     // not been called, call it now. Then stop.
     void lookupTimeout() {
@@ -896,7 +899,7 @@ public:
     }
 };
 
-class ForwardQuery::ForwardQueryImpl : public IOFetch::Callback {
+class ForwardQuery : public IOFetch::Callback, public AbstractRunningQuery {
 private:
     // The io service to handle async calls
     IOService& io_;
@@ -928,7 +931,7 @@ private:
     asio::deadline_timer lookup_timer;
 
     // Make FowardQuery deletes itself safely. for more information see
-    // the comments of outstanding_events in RunningQueryImpl.
+    // the comments of outstanding_events in RunningQuery.
     size_t outstanding_events_;
 
     // If we have a client timeout, we call back with a failure message,
@@ -959,7 +962,7 @@ private:
     }
 
 public:
-    ForwardQueryImpl(IOService& io,
+    ForwardQuery(IOService& io,
         ConstMessagePtr query_message,
         MessagePtr answer_message,
         boost::shared_ptr<AddressVector> upstream,
@@ -983,7 +986,7 @@ public:
             lookup_timer.expires_from_now(
                 boost::posix_time::milliseconds(lookup_timeout));
             ++outstanding_events_;
-            lookup_timer.async_wait(boost::bind(&ForwardQueryImpl::lookupTimeout, this));
+            lookup_timer.async_wait(boost::bind(&ForwardQuery::lookupTimeout, this));
         }
 
         // Setup the timer to send an answer (client_timeout)
@@ -991,12 +994,14 @@ public:
             client_timer.expires_from_now(
                 boost::posix_time::milliseconds(client_timeout));
             ++outstanding_events_;
-            client_timer.async_wait(boost::bind(&ForwardQueryImpl::clientTimeout, this));
+            client_timer.async_wait(boost::bind(&ForwardQuery::clientTimeout, this));
         }
 
         send();
     }
 
+    virtual ~ForwardQuery() {};
+
     virtual void lookupTimeout() {
         if (!callback_called_) {
             makeSERVFAIL();
@@ -1072,7 +1077,9 @@ public:
     }
 };
 
-RunningQuery*
+}
+
+AbstractRunningQuery*
 RecursiveQuery::resolve(const QuestionPtr& question,
     const isc::resolve::ResolverInterface::CallbackPtr callback)
 {
@@ -1125,7 +1132,7 @@ RecursiveQuery::resolve(const QuestionPtr& question,
     return (NULL);
 }
 
-RunningQuery*
+AbstractRunningQuery*
 RecursiveQuery::resolve(const Question& question,
                         MessagePtr answer_message,
                         OutputBufferPtr buffer,
@@ -1187,7 +1194,7 @@ RecursiveQuery::resolve(const Question& question,
     return (NULL);
 }
 
-ForwardQuery*
+AbstractRunningQuery*
 RecursiveQuery::forward(ConstMessagePtr query_message,
     MessagePtr answer_message,
     OutputBufferPtr buffer,
@@ -1213,47 +1220,9 @@ 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
-    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_;
-}
-
-RunningQuery::RunningQuery(isc::asiolink::IOService& io,
-        const isc::dns::Question question,
-        isc::dns::MessagePtr answer_message,
-        std::pair<std::string, uint16_t>& test_server,
-        isc::util::OutputBufferPtr buffer,
-        isc::resolve::ResolverInterface::CallbackPtr cb,
-        int query_timeout, int client_timeout, int lookup_timeout,
-        unsigned retries,
-        isc::nsas::NameserverAddressStore& nsas,
-        isc::cache::ResolverCache& cache,
-        boost::shared_ptr<RttRecorder>& recorder)
-{
-    rqi_ = new RunningQueryImpl(io, question, answer_message, test_server,
-                                buffer, cb, query_timeout, client_timeout,
-                                lookup_timeout, retries, nsas, cache,
-                                recorder);
-}
-
-RunningQuery::~RunningQuery() {
-        delete rqi_;
+    return (new ForwardQuery(io, query_message, answer_message,
+                             upstream_, buffer, callback, query_timeout_,
+                             client_timeout_, lookup_timeout_));
 }
 
 } // namespace asiodns

src/lib/resolve/recursive_query.h

@@ -54,63 +54,23 @@ private:
 
 typedef std::vector<std::pair<std::string, uint16_t> > AddressVector;
 
-/// \brief A forwarded query
-///
-/// This class represents an active forwarded query object;
-/// i.e. an outstanding query to a different server when operating
-/// in forwarder mode.
-///
-/// It can not be instantiated directly, but is created by
-/// RecursiveQuery::forward().
-///
-/// Its only public method is its destructor, and that should in theory
-/// not be called either except in some unit tests. Instances should
-/// delete themselves when the query is finished.
-class ForwardQuery {
-    friend class RecursiveQuery;
-private:
-    class ForwardQueryImpl;
-    ForwardQueryImpl* fqi_;
-    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);
-public:
-    ~ForwardQuery();
-};
-
 /// \brief A Running query
 ///
-/// This class represents an active running query object;
-/// i.e. an outstanding query to an authoritative name server.
+/// This base class represents an active running query object;
+/// i.e. an outstanding query to an authoritative name server or
+/// upstream server (when running in forwarder mode).
 ///
 /// It can not be instantiated directly, but is created by
-/// RecursiveQuery::resolve().
+/// RecursiveQuery::resolve() and RecursiveQuery::forward().
 ///
 /// Its only public method is its destructor, and that should in theory
 /// not be called either except in some unit tests. Instances should
 /// delete themselves when the query is finished.
-class RunningQuery {
-    friend class RecursiveQuery;
-private:
-    class RunningQueryImpl;
-    RunningQueryImpl* rqi_;
-    RunningQuery(isc::asiolink::IOService& io,
-        const isc::dns::Question question,
-        isc::dns::MessagePtr answer_message,
-        std::pair<std::string, uint16_t>& test_server,
-        isc::util::OutputBufferPtr buffer,
-        isc::resolve::ResolverInterface::CallbackPtr cb,
-        int query_timeout, int client_timeout, int lookup_timeout,
-        unsigned retries,
-        isc::nsas::NameserverAddressStore& nsas,
-        isc::cache::ResolverCache& cache,
-        boost::shared_ptr<RttRecorder>& recorder);
+class AbstractRunningQuery {
+protected:
+    AbstractRunningQuery() {};
 public:
-    ~RunningQuery();
+    virtual ~AbstractRunningQuery() {};
 };
 
 /// \brief Recursive Query
@@ -186,7 +146,7 @@ public:
     /// \param question The question being answered <qname/qclass/qtype>
     /// \param callback Callback object. See
     ///        \c ResolverInterface::Callback for more information
-    RunningQuery* resolve(const isc::dns::QuestionPtr& question,
+    AbstractRunningQuery* resolve(const isc::dns::QuestionPtr& question,
         const isc::resolve::ResolverInterface::CallbackPtr callback);
 
 
@@ -202,13 +162,14 @@ public:
     /// \param buffer An output buffer into which the intermediate responses will
     ///        be copied.
     /// \param server A pointer to the \c DNSServer object handling the client
-    /// \return A pointer to the active RunningQuery object created by this
-    ///         call (if any); this object should delete itself in normal
-    ///         circumstances, and can normally be ignored by the caller,
-    ///         but a pointer is returned for use-cases such as unit tests.
+    /// \return A pointer to the active AbstractRunningQuery object
+    ///         created by this call (if any); this object should delete
+    ///         itself in normal circumstances, and can normally be ignored
+    ///         by the caller, but a pointer is returned for use-cases
+    ///         such as unit tests.
     ///         Returns NULL if the data was found internally and no actual
     ///         query was sent.
-    RunningQuery* resolve(const isc::dns::Question& question,
+    AbstractRunningQuery* resolve(const isc::dns::Question& question,
                           isc::dns::MessagePtr answer_message,
                           isc::util::OutputBufferPtr buffer,
                           DNSServer* server);
@@ -228,7 +189,7 @@ public:
     ///         this object should delete itself in normal circumstances,
     ///         and can normally be ignored by the caller, but a pointer
     ///         is returned for use-cases such as unit tests.
-    ForwardQuery* forward(isc::dns::ConstMessagePtr query_message,
+    AbstractRunningQuery* 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

@@ -510,12 +510,13 @@ protected:
     vector<uint8_t> callback_data_;
     ScopedSocket sock_;
     boost::shared_ptr<isc::util::unittests::TestResolver> resolver_;
-    RunningQuery* running_query_;
+    AbstractRunningQuery* running_query_;
 };
 
 RecursiveQueryTest::RecursiveQueryTest() :
     dns_service_(NULL), callback_(NULL), callback_protocol_(0),
-    callback_native_(-1), resolver_(new isc::util::unittests::TestResolver()), running_query_(NULL)
+    callback_native_(-1), resolver_(new isc::util::unittests::TestResolver()),
+    running_query_(NULL)
 {
     nsas_.reset(new isc::nsas::NameserverAddressStore(resolver_));
 }
@@ -661,7 +662,7 @@ TEST_F(RecursiveQueryTest, forwarderSend) {
 
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    ForwardQuery* fq = rq.forward(query_message, answer, buffer, &server);
+    AbstractRunningQuery* fq = rq.forward(query_message, answer, buffer, &server);
 
     char data[4096];
     size_t size = sizeof(data);
@@ -759,7 +760,7 @@ TEST_F(RecursiveQueryTest, forwardQueryTimeout) {
     isc::resolve::initResponseMessage(question, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
+    AbstractRunningQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
@@ -795,7 +796,7 @@ TEST_F(RecursiveQueryTest, forwardClientTimeout) {
     isc::resolve::initResponseMessage(q, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
+    AbstractRunningQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
@@ -832,7 +833,7 @@ TEST_F(RecursiveQueryTest, forwardLookupTimeout) {
     isc::resolve::initResponseMessage(question, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
+    AbstractRunningQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);
@@ -868,7 +869,7 @@ TEST_F(RecursiveQueryTest, lowtimeouts) {
     isc::resolve::initResponseMessage(question, *query_message);
 
     boost::shared_ptr<MockResolverCallback> callback(new MockResolverCallback(&server));
-    ForwardQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
+    AbstractRunningQuery* fq = query.forward(query_message, answer, buffer, &server, callback);
     // Run the test
     io_service_.run();
     EXPECT_EQ(callback->result, MockResolverCallback::FAILURE);

src/lib/resolve/tests/recursive_query_unittest_2.cc

@@ -150,7 +150,7 @@ public:
     udp::socket     udp_socket_;                ///< Socket used by UDP server
 
     /// TODO: doc
-    RunningQuery* running_query_;
+    AbstractRunningQuery* running_query_;
 
     /// \brief Constructor
     RecursiveQueryTest2() :

src/lib/resolve/tests/recursive_query_unittest_3.cc

@@ -133,7 +133,7 @@ public:
     udp::socket     udp_socket_;                ///< Socket used by UDP server
 
     /// TODO
-    RunningQuery* running_query_;
+    AbstractRunningQuery* running_query_;
 
     /// \brief Constructor
     RecursiveQueryTest3() :