[master] Merge branch 'trac2946'

src/lib/asiodns/dns_service.cc

@@ -64,8 +64,9 @@ public:
     // SyncUDPServer has different constructor signature so it cannot be
     // templated.
     void addSyncUDPServerFromFD(int fd, int af) {
-        SyncUDPServerPtr server(new SyncUDPServer(io_service_.get_io_service(),
-                                                  fd, af, lookup_));
+        SyncUDPServerPtr server(SyncUDPServer::create(
+                                    io_service_.get_io_service(), fd, af,
+                                    lookup_));
         startServer(server);
     }
 

src/lib/asiodns/sync_udp_server.cc

@@ -26,6 +26,8 @@
 
 #include <boost/bind.hpp>
 
+#include <cassert>
+
 #include <sys/types.h>
 #include <netinet/in.h>
 #include <sys/socket.h>
@@ -38,13 +40,19 @@ using namespace isc::asiolink;
 namespace isc {
 namespace asiodns {
 
+SyncUDPServerPtr
+SyncUDPServer::create(asio::io_service& io_service, const int fd,
+                      const int af, DNSLookup* lookup)
+{
+    return (SyncUDPServerPtr(new SyncUDPServer(io_service, fd, af, lookup)));
+}
+
 SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
                              const int af, DNSLookup* lookup) :
     output_buffer_(new isc::util::OutputBuffer(0)),
     query_(new isc::dns::Message(isc::dns::Message::PARSE)),
     udp_endpoint_(sender_), lookup_callback_(lookup),
-    resume_called_(false), done_(false), stopped_(false),
-    recv_callback_(boost::bind(&SyncUDPServer::handleRead, this, _1, _2))
+    resume_called_(false), done_(false), stopped_(false)
 {
     if (af != AF_INET && af != AF_INET6) {
         isc_throw(InvalidParameter, "Address family must be either AF_INET "
@@ -69,19 +77,18 @@ SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
 
 void
 SyncUDPServer::scheduleRead() {
-    socket_->async_receive_from(asio::mutable_buffers_1(data_, MAX_LENGTH),
-                                sender_, recv_callback_);
+    socket_->async_receive_from(
+        asio::mutable_buffers_1(data_, MAX_LENGTH), sender_,
+        boost::bind(&SyncUDPServer::handleRead, shared_from_this(), _1, _2));
 }
 
 void
 SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
-    // If the server has been stopped, it could even have been destroyed
-    // by the time of this call.  We'll solve this problem in #2946, but
-    // until then we exit as soon as possible without accessing any other
-    // invalidated fields (note that referencing stopped_ is also incorrect,
-    // but experiments showed it often keeps the original value in practice,
-    // so we live with it until the complete fix).
     if (stopped_) {
+        // stopped_ can be set to true only after the socket object is closed.
+        // checking this would also detect premature destruction of 'this'
+        // object.
+        assert(socket_ && !socket_->is_open());
         return;
     }
     if (ec) {

src/lib/asiodns/sync_udp_server.h

@@ -33,23 +33,43 @@
 #include <boost/function.hpp>
 #include <boost/noncopyable.hpp>
 #include <boost/scoped_ptr.hpp>
+#include <boost/enable_shared_from_this.hpp>
 
 #include <stdint.h>
 
 namespace isc {
 namespace asiodns {
 
+class SyncUDPServer;
+typedef boost::shared_ptr<SyncUDPServer> SyncUDPServerPtr;
+
 /// \brief An UDP server that doesn't asynchronous lookup handlers.
 ///
 /// That means, the lookup handler must provide the answer right away.
 /// This allows for implementation with less overhead, compared with
 /// the \c UDPServer class.
-class SyncUDPServer : public DNSServer, public boost::noncopyable {
+///
+/// This class inherits from boost::enable_shared_from_this so a shared
+/// pointer of this object can be passed in an ASIO callback and won't be
+/// accidentally destroyed while waiting for events.  To enforce this style
+/// of creation, a static factory method is provided, and the constructor is
+/// hidden as a private.
+class SyncUDPServer : public DNSServer,
+                      public boost::enable_shared_from_this<SyncUDPServer>,
+                      boost::noncopyable
+{
+private:
+    /// \brief Constructor.
+    ///
+    /// This is hidden as private (see the class description).
+    SyncUDPServer(asio::io_service& io_service, const int fd, const int af,
+                  DNSLookup* lookup);
+
 public:
-    /// \brief Constructor
+    /// \brief Factory of SyncUDPServer object in the form of shared_ptr.
     ///
     /// Due to the nature of this server, it's meaningless if the lookup
-    /// callback is NULL.  So the constructor explicitly rejects that case
+    /// callback is NULL.  So this method explicitly rejects that case
     /// with an exception.  Likewise, it doesn't take "checkin" or "answer"
     /// callbacks.  In fact, calling "checkin" from receive callback does not
     /// make sense for any of the DNSServer variants (see Trac #2935);
@@ -67,8 +87,8 @@ public:
     /// \throw isc::InvalidParameter lookup is NULL
     /// \throw isc::asiolink::IOError when a low-level error happens, like the
     ///     fd is not a valid descriptor.
-    SyncUDPServer(asio::io_service& io_service, const int fd, const int af,
-                  DNSLookup* lookup);
+    static SyncUDPServerPtr create(asio::io_service& io_service, const int fd,
+                                   const int af, DNSLookup* lookup);
 
     /// \brief Start the SyncUDPServer.
     ///
@@ -133,7 +153,7 @@ private:
     // requires it.
     isc::dns::MessagePtr query_, answer_;
     // The socket used for the communication
-    std::auto_ptr<asio::ip::udp::socket> socket_;
+    boost::scoped_ptr<asio::ip::udp::socket> socket_;
     // Wrapper of socket_ in the form of asiolink::IOSocket.
     // "DummyIOCallback" is not necessary for this class, but using the
     // template is the easiest way to create a UDP instance of IOSocket.
@@ -156,24 +176,6 @@ private:
     // Placeholder for error code object.  It will be passed to ASIO library
     // to have it set in case of error.
     asio::error_code ec_;
-    // The callback functor for internal asynchronous read event.  This is
-    // stateless (and it will be copied in the ASIO library anyway), so
-    // can be const.
-    // SunStudio doesn't like a boost::function object to be passed, so
-    // we use the wrapper class as a workaround.
-    class CallbackWrapper {
-    public:
-        CallbackWrapper(boost::function<void(const asio::error_code&, size_t)>
-                        callback) :
-            callback_(callback)
-        {}
-        void operator()(const asio::error_code& error, size_t len) {
-            callback_(error, len);
-        }
-    private:
-        boost::function<void(const asio::error_code&, size_t)> callback_;
-    };
-    const CallbackWrapper recv_callback_;
 
     // Auxiliary functions
 

src/lib/asiodns/tcp_server.cc

@@ -77,18 +77,18 @@ TCPServer::TCPServer(io_service& io_service, int fd, int af,
 }
 
 namespace {
-    // Called by the timeout_ deadline timer if the client takes too long.
-    // If not aborted, cancels the given socket
-    // (in which case TCPServer::operator() will be called to continue,
-    // with an 'aborted' error code
-    void do_timeout(asio::ip::tcp::socket& socket,
-                    const asio::error_code& error)
-    {
-        if (error != asio::error::operation_aborted) {
-            socket.cancel();
-        }
+// Called by the timeout_ deadline timer if the client takes too long.
+// If not aborted, cancels the given socket
+// (in which case TCPServer::operator() will be called to continue,
+// with an 'aborted' error code.)
+void doTimeOut(boost::shared_ptr<asio::ip::tcp::socket> socket,
+               const asio::error_code& error)
+{
+    if (error != asio::error::operation_aborted) {
+        socket->cancel();
     }
 }
+}
 
 void
 TCPServer::operator()(asio::error_code ec, size_t length) {
@@ -149,13 +149,16 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
         /// asynchronous read call.
         data_.reset(new char[MAX_LENGTH]);
 
-        /// Start a timer to drop the connection if it is idle
+        /// Start a timer to drop the connection if it is idle.  note that
+        // we pass a shared_ptr of the socket object so that it won't be
+        // destroyed at least until the timeout callback (including abort)
+        // is called.
         if (*tcp_recv_timeout_ > 0) {
             timeout_.reset(new asio::deadline_timer(io_)); // shouldn't throw
             timeout_->expires_from_now( // consider any exception fatal.
                 boost::posix_time::milliseconds(*tcp_recv_timeout_));
-            timeout_->async_wait(boost::bind(&do_timeout, boost::ref(*socket_),
-                                 asio::placeholders::error));
+            timeout_->async_wait(boost::bind(&doTimeOut, socket_,
+                                             asio::placeholders::error));
         }
 
         /// Read the message, in two parts.  First, the message length:

src/lib/asiodns/tests/dns_server_unittest.cc

@@ -89,8 +89,8 @@ class ServerStopper {
         ServerStopper() : server_to_stop_(NULL) {}
         virtual ~ServerStopper(){}
 
-        void setServerToStop(DNSServer* server) {
-            server_to_stop_ = server;
+        void setServerToStop(DNSServer& server) {
+            server_to_stop_ = &server;
         }
 
         void stopServer() const {
@@ -378,40 +378,36 @@ class DNSServerTestBase : public::testing::Test {
                                                          server_port))),
             tcp_client_(new TCPClient(service,
                                       ip::tcp::endpoint(server_address_,
-                                                        server_port))),
-            udp_server_(NULL),
-            tcp_server_(NULL)
+                                                        server_port)))
         {
             current_service = &service;
         }
 
         ~ DNSServerTestBase() {
-            if (udp_server_ != NULL) {
+            if (udp_server_) {
                 udp_server_->stop();
             }
-            if (tcp_server_ != NULL) {
+            if (tcp_server_) {
                 tcp_server_->stop();
             }
             delete checker_;
             delete lookup_;
             delete sync_lookup_;
             delete answer_;
-            delete udp_server_;
             delete udp_client_;
-            delete tcp_server_;
             delete tcp_client_;
             // No delete here. The service is not allocated by new, but as our
             // member. This only references it, so just cleaning the pointer.
             current_service = NULL;
         }
 
-        void testStopServerByStopper(DNSServer* server, SimpleClient* client,
-                ServerStopper* stopper)
+        void testStopServerByStopper(DNSServer& server, SimpleClient* client,
+                                     ServerStopper* stopper)
         {
             static const unsigned int IO_SERVICE_TIME_OUT = 5;
             io_service_is_time_out = false;
             stopper->setServerToStop(server);
-            (*server)();
+            server();
             client->sendDataThenWaitForFeedback(query_message);
             // Since thread hasn't been introduced into the tool box, using
             // signal to make sure run function will eventually return even
@@ -446,8 +442,8 @@ class DNSServerTestBase : public::testing::Test {
         SimpleAnswer* const answer_;
         UDPClient*    const udp_client_;
         TCPClient*    const tcp_client_;
-        UDPServerClass* udp_server_;
-        TCPServer*    tcp_server_;
+        boost::shared_ptr<UDPServerClass> udp_server_;
+        boost::shared_ptr<TCPServer> tcp_server_;
 
         // To access them in signal handle function, the following
         // variables have to be static.
@@ -508,27 +504,30 @@ protected:
         this->udp_server_ = createServer(fd_udp, AF_INET6);
         const int fd_tcp(getFd(SOCK_STREAM));
         ASSERT_NE(-1, fd_tcp) << strerror(errno);
-        this->tcp_server_ = new TCPServer(this->service, fd_tcp, AF_INET6,
-                                          this->checker_, this->lookup_,
-                                          this->answer_);
+        this->tcp_server_ =
+            boost::shared_ptr<TCPServer>(new TCPServer(
+                                             this->service, fd_tcp, AF_INET6,
+                                             this->checker_, this->lookup_,
+                                             this->answer_));
     }
 
     // A helper factory of the tested UDP server class: allow customization
     // by template specialization.
-    UDPServerClass* createServer(int fd, int af) {
-        return (new UDPServerClass(this->service, fd, af,
-                                   this->checker_, this->lookup_,
-                                   this->answer_));
+    boost::shared_ptr<UDPServerClass> createServer(int fd, int af) {
+        return (boost::shared_ptr<UDPServerClass>(
+                    new UDPServerClass(this->service, fd, af,
+                                       this->checker_, this->lookup_,
+                                       this->answer_)));
     }
 };
 
 // Specialization for SyncUDPServer.  It needs to use SyncDummyLookup.
 template<>
-SyncUDPServer*
+boost::shared_ptr<SyncUDPServer>
 FdInit<SyncUDPServer>::createServer(int fd, int af) {
     delete this->lookup_;
     this->lookup_ = new SyncDummyLookup;
-    return (new SyncUDPServer(this->service, fd, af, this->lookup_));
+    return (SyncUDPServer::create(this->service, fd, af, this->lookup_));
 }
 
 // This makes it the template as gtest wants it.
@@ -557,7 +556,7 @@ asio::io_service* DNSServerTestBase<UDPServerClass>::current_service(NULL);
 // get response, UDP server will be stopped, the io service won't quit
 // if udp server doesn't stop successfully.
 TYPED_TEST(DNSServerTest, stopUDPServerAfterOneQuery) {
-    this->testStopServerByStopper(this->udp_server_, this->udp_client_,
+    this->testStopServerByStopper(*this->udp_server_, this->udp_client_,
                                   this->udp_client_);
     EXPECT_EQ(query_message, this->udp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -566,18 +565,17 @@ TYPED_TEST(DNSServerTest, stopUDPServerAfterOneQuery) {
 // Test whether udp server stopped successfully before server start to serve
 TYPED_TEST(DNSServerTest, stopUDPServerBeforeItStartServing) {
     this->udp_server_->stop();
-    this->testStopServerByStopper(this->udp_server_, this->udp_client_,
+    this->testStopServerByStopper(*this->udp_server_, this->udp_client_,
                                   this->udp_client_);
     EXPECT_EQ(std::string(""), this->udp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
 }
 
-
 // Test whether udp server stopped successfully during message check.
 // This only works for non-sync server; SyncUDPServer doesn't use checkin
 // callback.
 TEST_F(AsyncServerTest, stopUDPServerDuringMessageCheck) {
-    this->testStopServerByStopper(this->udp_server_, this->udp_client_,
+    this->testStopServerByStopper(*this->udp_server_, this->udp_client_,
                                   this->checker_);
     EXPECT_EQ(std::string(""), this->udp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -585,7 +583,7 @@ TEST_F(AsyncServerTest, stopUDPServerDuringMessageCheck) {
 
 // Test whether udp server stopped successfully during query lookup
 TYPED_TEST(DNSServerTest, stopUDPServerDuringQueryLookup) {
-    this->testStopServerByStopper(this->udp_server_, this->udp_client_,
+    this->testStopServerByStopper(*this->udp_server_, this->udp_client_,
                                   this->lookup_);
     EXPECT_EQ(std::string(""), this->udp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -595,7 +593,7 @@ TYPED_TEST(DNSServerTest, stopUDPServerDuringQueryLookup) {
 // Only works for (non-sync) server because SyncUDPServer doesn't use answer
 // callback.
 TEST_F(AsyncServerTest, stopUDPServerDuringPrepareAnswer) {
-    testStopServerByStopper(udp_server_, udp_client_, answer_);
+    testStopServerByStopper(*udp_server_, udp_client_, answer_);
     EXPECT_EQ(std::string(""), udp_client_->getReceivedData());
     EXPECT_TRUE(serverStopSucceed());
 }
@@ -612,18 +610,17 @@ stopServerManyTimes(DNSServer *server, unsigned int times) {
 TYPED_TEST(DNSServerTest, stopUDPServerMoreThanOnce) {
     ASSERT_NO_THROW({
         boost::function<void()> stop_server_3_times
-            = boost::bind(stopServerManyTimes, this->udp_server_, 3);
+            = boost::bind(stopServerManyTimes, this->udp_server_.get(), 3);
         this->udp_client_->setGetFeedbackCallback(stop_server_3_times);
-        this->testStopServerByStopper(this->udp_server_,
+        this->testStopServerByStopper(*this->udp_server_,
                                       this->udp_client_, this->udp_client_);
         EXPECT_EQ(query_message, this->udp_client_->getReceivedData());
     });
     EXPECT_TRUE(this->serverStopSucceed());
 }
 
-
 TYPED_TEST(DNSServerTest, stopTCPServerAfterOneQuery) {
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->tcp_client_);
     EXPECT_EQ(query_message, this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -632,7 +629,7 @@ TYPED_TEST(DNSServerTest, stopTCPServerAfterOneQuery) {
 TYPED_TEST(DNSServerTest, TCPTimeoutOnLen) {
     this->tcp_server_->setTCPRecvTimeout(100);
     this->tcp_client_->setSendDataLenDelay(2);
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->tcp_client_);
     EXPECT_EQ("", this->tcp_client_->getReceivedData());
     EXPECT_FALSE(this->serverStopSucceed());
@@ -642,7 +639,7 @@ TYPED_TEST(DNSServerTest, TCPTimeout) {
     // set delay higher than timeout
     this->tcp_server_->setTCPRecvTimeout(100);
     this->tcp_client_->setSendDataDelay(2);
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->tcp_client_);
     EXPECT_EQ("", this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -652,7 +649,7 @@ TYPED_TEST(DNSServerTest, TCPNoTimeout) {
     // set delay lower than timeout
     this->tcp_server_->setTCPRecvTimeout(3000);
     this->tcp_client_->setSendDataDelay(1);
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->tcp_client_);
     EXPECT_EQ("BIND10 is awesome", this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -661,7 +658,7 @@ TYPED_TEST(DNSServerTest, TCPNoTimeout) {
 // Test whether tcp server stopped successfully before server start to serve
 TYPED_TEST(DNSServerTest, stopTCPServerBeforeItStartServing) {
     this->tcp_server_->stop();
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->tcp_client_);
     EXPECT_EQ(std::string(""), this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -670,7 +667,7 @@ TYPED_TEST(DNSServerTest, stopTCPServerBeforeItStartServing) {
 
 // Test whether tcp server stopped successfully during message check
 TYPED_TEST(DNSServerTest, stopTCPServerDuringMessageCheck) {
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->checker_);
     EXPECT_EQ(std::string(""), this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -678,7 +675,7 @@ TYPED_TEST(DNSServerTest, stopTCPServerDuringMessageCheck) {
 
 // Test whether tcp server stopped successfully during query lookup
 TYPED_TEST(DNSServerTest, stopTCPServerDuringQueryLookup) {
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->lookup_);
     EXPECT_EQ(std::string(""), this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -686,7 +683,7 @@ TYPED_TEST(DNSServerTest, stopTCPServerDuringQueryLookup) {
 
 // Test whether tcp server stopped successfully during composing answer
 TYPED_TEST(DNSServerTest, stopTCPServerDuringPrepareAnswer) {
-    this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+    this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->answer_);
     EXPECT_EQ(std::string(""), this->tcp_client_->getReceivedData());
     EXPECT_TRUE(this->serverStopSucceed());
@@ -698,9 +695,9 @@ TYPED_TEST(DNSServerTest, stopTCPServerDuringPrepareAnswer) {
 TYPED_TEST(DNSServerTest, stopTCPServeMoreThanOnce) {
     ASSERT_NO_THROW({
         boost::function<void()> stop_server_3_times
-            = boost::bind(stopServerManyTimes, this->tcp_server_, 3);
+            = boost::bind(stopServerManyTimes, this->tcp_server_.get(), 3);
         this->tcp_client_->setGetFeedbackCallback(stop_server_3_times);
-        this->testStopServerByStopper(this->tcp_server_, this->tcp_client_,
+        this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                       this->tcp_client_);
         EXPECT_EQ(query_message, this->tcp_client_->getReceivedData());
     });
@@ -759,14 +756,36 @@ TEST_F(SyncServerTest, unsupportedOps) {
 // Check it rejects forgotten resume (eg. insists that it is synchronous)
 TEST_F(SyncServerTest, mustResume) {
     lookup_->allow_resume_ = false;
-    ASSERT_THROW(testStopServerByStopper(udp_server_, udp_client_, lookup_),
+    ASSERT_THROW(testStopServerByStopper(*udp_server_, udp_client_, lookup_),
                  isc::Unexpected);
 }
 
 // SyncUDPServer doesn't allow NULL lookup callback.
 TEST_F(SyncServerTest, nullLookupCallback) {
-    EXPECT_THROW(SyncUDPServer(service, 0, AF_INET, NULL),
+    EXPECT_THROW(SyncUDPServer::create(service, 0, AF_INET, NULL),
                  isc::InvalidParameter);
 }
 
+TEST_F(SyncServerTest, resetUDPServerBeforeEvent) {
+    // Reset the UDP server object after starting and before it would get
+    // an event from io_service (in this case abort event).  The following
+    // sequence confirms it's shut down immediately, and without any
+    // disruption.
+
+    // Since we'll stop the server run() should immediately return, and
+    // it's very unlikely to cause hangup.  But we'll make very sure it
+    // doesn't happen.
+    const unsigned int IO_SERVICE_TIME_OUT = 5;
+    (*udp_server_)();
+    udp_server_->stop();
+    udp_server_.reset();
+    void (*prev_handler)(int) = std::signal(SIGALRM, stopIOService);
+    current_service = &service;
+    alarm(IO_SERVICE_TIME_OUT);
+    service.run();
+    alarm(0);
+    std::signal(SIGALRM, prev_handler);
+    EXPECT_FALSE(io_service_is_time_out);
+}
+
 }