[master] Merge branch 'trac5189'

# Conflicts:
#	src/bin/agent/ca_command_mgr.h
#	src/bin/agent/tests/ca_command_mgr_unittests.cc
#	src/lib/asiolink/testutils/test_server_unix_socket.cc

src/bin/agent/ca_command_mgr.cc

@@ -14,6 +14,8 @@
 #include <asiolink/unix_domain_socket.h>
 #include <cc/command_interpreter.h>
 #include <cc/data.h>
+#include <cc/json_feed.h>
+#include <config/client_connection.h>
 #include <boost/pointer_cast.hpp>
 #include <iterator>
 #include <string>
@@ -25,6 +27,14 @@ using namespace isc::data;
 using namespace isc::hooks;
 using namespace isc::process;
 
+namespace {
+
+/// @brief Client side connection timeout.
+/// @todo Make it configurable.
+const long CONNECTION_TIMEOUT = 5000;
+
+}
+
 namespace isc {
 namespace agent {
 
@@ -186,34 +196,40 @@ CtrlAgentCommandMgr::forwardCommand(const std::string& service,
     std::string socket_name = socket_info->get("socket-name")->stringValue();
 
     // Forward command and receive reply.
-    IOService io_service;
-    UnixDomainSocket unix_socket(io_service);
-    size_t receive_len;
-    try {
-        unix_socket.connect(socket_name);
-        std::string wire_command = command->toWire();
-        unix_socket.write(&wire_command[0], wire_command.size());
-        receive_len = unix_socket.receive(&receive_buf_[0], receive_buf_.size());
-
-    } catch (const std::exception& ex) {
+    IOServicePtr io_service(new IOService());;
+    ClientConnection conn(*io_service);
+    boost::system::error_code received_ec;
+    ConstJSONFeedPtr received_feed;
+    conn.start(ClientConnection::SocketPath(socket_name),
+               ClientConnection::ControlCommand(command->toWire()),
+               [&io_service, &received_ec, &received_feed]
+               (const boost::system::error_code& ec, ConstJSONFeedPtr feed) {
+                   // Capture error code and parsed data.
+                   received_ec = ec;
+                   received_feed = feed;
+                   // Got the IO service so stop IO service. This causes to
+                   // stop IO service when all handlers have been invoked.
+                   io_service->stopWork();
+               }, ClientConnection::Timeout(CONNECTION_TIMEOUT));
+    io_service->run();
+
+    if (received_ec) {
         isc_throw(CommandForwardingError, "unable to forward command to the "
-                  << service << " service: " << ex.what() << ". The server "
-                  "is likely to be offline");
+                  << service << " service: " << received_ec.message()
+                  << ". The server is likely to be offline");
     }
 
-    // This is really not possible right now, but when we migrate to the
-    // solution using timeouts it is possible that the response is not
-    // received.
-    if (receive_len == 0) {
-        isc_throw(CommandForwardingError, "internal server error: no answer"
-                  " received from the server to the forwarded message");
+    // This shouldn't happen because the fact that there was no time out indicates
+    // that the whole response has been read and it should be stored within the
+    // feed. But, let's check to prevent assertions.
+    if (!received_feed) {
+        isc_throw(CommandForwardingError, "internal server error: empty response"
+                  " received from the unix domain socket");
     }
 
-    std::string reply(&receive_buf_[0], receive_len);
-
     ConstElementPtr answer;
     try {
-        answer = Element::fromJSON(reply);
+        answer = received_feed->toElement();
 
         LOG_INFO(agent_logger, CTRL_AGENT_COMMAND_FORWARDED)
             .arg(cmd_name).arg(service);

src/bin/agent/ca_command_mgr.h

@@ -11,7 +11,6 @@
 #include <exceptions/exceptions.h>
 #include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>
-#include <array>
 
 namespace isc {
 namespace agent {
@@ -109,9 +108,6 @@ private:
     /// thus the constructor is private.
     CtrlAgentCommandMgr();
 
-    /// @brief Buffer into which responses to forwarded commands are stored.
-    std::array<char, 65535> receive_buf_;
-
 };
 
 } // end of namespace isc::agent

src/bin/agent/tests/ca_command_mgr_unittests.cc

@@ -171,12 +171,14 @@ public:
     ///
     /// @param response Stub response to be sent from the server socket to the
     /// client.
-    void bindServerSocket(const std::string& response) {
+    /// @param use_thread Indicates if the IO service will be ran in thread.
+    void bindServerSocket(const std::string& response,
+                          const bool use_thread = false) {
         server_socket_.reset(new test::TestServerUnixSocket(*getIOService(),
                                                             unixSocketFilePath(),
-                                                            TEST_TIMEOUT,
                                                             response));
-        server_socket_->bindServerSocket();
+        server_socket_->startTimer(TEST_TIMEOUT);
+        server_socket_->bindServerSocket(use_thread);
     }
 
     /// @brief Creates command with no arguments.
@@ -224,43 +226,35 @@ public:
         // Configure client side socket.
         configureControlSocket(server_type);
         // Create server side socket.
-        bindServerSocket(server_response);
+        bindServerSocket(server_response, true);
 
         // The client side communication is synchronous. To be able to respond
-        // to this we need to run the server side socket at the same time.
-        // Running IO service in a thread guarantees that the server responds
-        // as soon as it receives the control command.
-        isc::util::thread::Thread th(boost::bind(&CtrlAgentCommandMgrTest::runIO,
-                                                 getIOService(), server_socket_,
-                                                 expected_responses));
+        // to this we need to run the server side socket at the same time as the
+        // client. Running IO service in a thread guarantees that the server
+        //responds as soon as it receives the control command.
+        isc::util::thread::Thread th(boost::bind(&IOService::run,
+                                                 getIOService().get()));
+
+
+        // Wait for the IO service in thread to actually run.
+        server_socket_->waitForRunning();
 
         ConstElementPtr command = createCommand("foo", service);
         ConstElementPtr answer = mgr_.handleCommand("foo", ConstElementPtr(),
                                                     command);
 
+        // Cancel all asynchronous operations and let the handlers to be invoked
+        // with operation_aborted error code.
+        server_socket_->stopServer();
+        getIOService()->stopWork();
+
+        // Wait for the thread to finish.
         th.wait();
 
+        EXPECT_EQ(expected_responses, server_socket_->getResponseNum());
         checkAnswer(answer, expected_result0, expected_result1, expected_result2);
     }
 
-    /// @brief Runs IO service until number of sent responses is lower than
-    /// expected.
-    ///
-    /// @param server_socket Pointer to the server socket.
-    /// @param expected_responses Number of expected responses.
-    static void runIO(IOServicePtr& io_service,
-                      const test::TestServerUnixSocketPtr& server_socket,
-                      const size_t expected_responses) {
-        while (server_socket->getResponseNum() < expected_responses) {
-            io_service->run_one();
-        }
-    }
-
-
-    CtrlAgentCommandMgrTest* getTestSelf() {
-        return (this);
-    }
-
     /// @brief a convenience reference to control agent command manager
     CtrlAgentCommandMgr& mgr_;
 
@@ -327,6 +321,18 @@ TEST_F(CtrlAgentCommandMgrTest, noService) {
 /// Check that error is returned to the client when the server to which the
 /// command was forwarded sent an invalid message.
 TEST_F(CtrlAgentCommandMgrTest, invalidAnswer) {
+    testForward(CtrlAgentCfgContext::TYPE_DHCP6, "dhcp6",
+                isc::config::CONTROL_RESULT_ERROR, -1, -1, 1,
+                "{ \"result\": }");
+}
+
+/// Check that connection is dropped if it takes too long. The test checks
+/// client's behavior when partial JSON is returned. Client will be waiting
+/// for the '}' and will timeout because it is never received.
+/// @todo Currently this test is disabled because we don't have configurable
+/// timeout value. It is hardcoded to 5 sec, which is too long for the
+/// unit test to run.
+TEST_F(CtrlAgentCommandMgrTest, DISABLED_connectionTimeout) {
     testForward(CtrlAgentCfgContext::TYPE_DHCP6, "dhcp6",
                 isc::config::CONTROL_RESULT_ERROR, -1, -1, 1,
                 "{ \"result\": 0");
@@ -360,19 +366,27 @@ TEST_F(CtrlAgentCommandMgrTest, forwardListCommands) {
     // Configure client side socket.
     configureControlSocket(CtrlAgentCfgContext::TYPE_DHCP4);
     // Create server side socket.
-    bindServerSocket("{ \"result\" : 3 }");
+    bindServerSocket("{ \"result\" : 3 }", true);
 
     // The client side communication is synchronous. To be able to respond
     // to this we need to run the server side socket at the same time.
     // Running IO service in a thread guarantees that the server responds
     // as soon as it receives the control command.
-    isc::util::thread::Thread th(boost::bind(&CtrlAgentCommandMgrTest::runIO,
-                                             getIOService(), server_socket_, 1));
+    isc::util::thread::Thread th(boost::bind(&IOService::run, getIOService().get()));
+
+    // Wait for the IO service in thread to actually run.
+    server_socket_->waitForRunning();
 
     ConstElementPtr command = createCommand("list-commands", "dhcp4");
     ConstElementPtr answer = mgr_.handleCommand("list-commands", ConstElementPtr(),
                                                 command);
 
+    // Cancel all asynchronous operations and let the handlers to be invoked
+    // with operation_aborted error code.
+    server_socket_->stopServer();
+    getIOService()->stopWork();
+
+    // Wait for the thread to finish.
     th.wait();
 
     // Answer of 3 is specific to the stub response we send when the

src/lib/asiolink/io_service.cc

-// Copyright (C) 2011-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -10,6 +10,7 @@
 
 #include <unistd.h>             // for some IPC/network system calls
 #include <netinet/in.h>
+#include <boost/shared_ptr.hpp>
 #include <sys/socket.h>
 
 namespace isc {
@@ -40,7 +41,7 @@ public:
     /// \brief The constructor
     IOServiceImpl() :
         io_service_(),
-        work_(io_service_)
+        work_(new boost::asio::io_service::work(io_service_))
     {};
     /// \brief The destructor.
     ~IOServiceImpl() {};
@@ -76,6 +77,12 @@ public:
     /// This will return the control to the caller of the \c run() method.
     void stop() { io_service_.stop();} ;
 
+    /// \brief Removes IO service work object to let it finish running
+    /// when all handlers have been invoked.
+    void stopWork() {
+        work_.reset();
+    }
+
     /// \brief Return the native \c io_service object used in this wrapper.
     ///
     /// This is a short term work around to support other Kea modules
@@ -89,7 +96,7 @@ public:
     }
 private:
     boost::asio::io_service io_service_;
-    boost::asio::io_service::work work_;
+    boost::shared_ptr<boost::asio::io_service::work> work_;
 };
 
 IOService::IOService() {
@@ -120,6 +127,11 @@ IOService::stop() {
     io_impl_->stop();
 }
 
+void
+IOService::stopWork() {
+    io_impl_->stopWork();
+}
+
 boost::asio::io_service&
 IOService::get_io_service() {
     return (io_impl_->get_io_service());

src/lib/asiolink/io_service.h

-// Copyright (C) 2011-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -64,6 +64,10 @@ public:
     /// This will return the control to the caller of the \c run() method.
     void stop();
 
+    /// \brief Removes IO service work object to let it finish running
+    /// when all handlers have been invoked.
+    void stopWork();
+
     /// \brief Return the native \c io_service object used in this wrapper.
     ///
     /// This is a short term work around to support other Kea modules

src/lib/asiolink/tests/unix_domain_socket_unittest.cc

@@ -33,9 +33,13 @@ public:
     /// @brief Constructor.
     ///
     /// Removes unix socket descriptor before the test.
-    UnixDomainSocketTest() : io_service_(),
-                             test_socket_(io_service_, unixSocketFilePath(),
-                                          TEST_TIMEOUT) {
+    UnixDomainSocketTest() :
+        io_service_(),
+        test_socket_(new test::TestServerUnixSocket(io_service_,
+                                                    unixSocketFilePath())),
+        response_(),
+        read_buf_() {
+        test_socket_->startTimer(TEST_TIMEOUT);
         removeUnixSocketFile();
     }
 
@@ -69,18 +73,58 @@ public:
         static_cast<void>(remove(unixSocketFilePath().c_str()));
     }
 
+    /// @brief Performs asynchronous receive on unix domain socket.
+    ///
+    /// This function performs partial read from the unix domain socket.
+    /// It uses @c read_buf_ or small size to ensure that the buffer fills
+    /// in before all that have been read. The partial responses are
+    /// appended to the @c response_ class member.
+    ///
+    /// If the response received so far is shorter than the expected
+    /// response, another partial read is scheduled.
+    ///
+    /// @param socket Reference to the unix domain socket.
+    /// @param expected_response Expected response.
+    void doReceive(UnixDomainSocket& socket,
+                   const std::string& expected_response) {
+        socket.asyncReceive(&read_buf_[0], read_buf_.size(),
+        [this, &socket, expected_response]
+            (const boost::system::error_code& ec, size_t length) {
+            if (!ec) {
+                // Append partial response received and see if the
+                // size of the response received so far is still
+                // smaller than expected. If it is, schedule another
+                // partial read.
+                response_.append(&read_buf_[0], length);
+                if (expected_response.size() > response_.size()) {
+                    doReceive(socket, expected_response);
+                }
+
+            } else if (ec.value() != boost::asio::error::operation_aborted) {
+                ADD_FAILURE() << "error occurred while asynchronously receiving"
+                    " data via unix domain socket: " << ec.message();
+            }
+        });
+    }
+
     /// @brief IO service used by the tests.
     IOService io_service_;
 
     /// @brief Server side unix socket used in these tests.
-    test::TestServerUnixSocket test_socket_;
+    test::TestServerUnixSocketPtr test_socket_;
+
+    /// @brief String containing a response received with @c doReceive.
+    std::string response_;
+
+    /// @brief Read buffer used by @c doReceive.
+    std::array<char, 2> read_buf_;
 };
 
 // This test verifies that the client can send data over the unix
 // domain socket and receive a response.
 TEST_F(UnixDomainSocketTest, sendReceive) {
     // Start the server.
-    test_socket_.bindServerSocket();
+    test_socket_->bindServerSocket();
 
     // Setup client side.
     UnixDomainSocket socket(io_service_);
@@ -95,7 +139,8 @@ TEST_F(UnixDomainSocketTest, sendReceive) {
     ASSERT_EQ(outbound_data.size(), sent_size);
 
     // Run IO service to generate server's response.
-    while (test_socket_.getResponseNum() < 1) {
+    while ((test_socket_->getResponseNum() < 1) &&
+           (!test_socket_->isStopped())) {
         io_service_.run_one();
     }
 
@@ -109,6 +154,77 @@ TEST_F(UnixDomainSocketTest, sendReceive) {
     EXPECT_EQ("received foo", response);
 }
 
+// This test verifies that the client can send the data over the unix
+// domain socket and receive a response asynchronously.
+TEST_F(UnixDomainSocketTest, asyncSendReceive) {
+    // Start the server.
+    test_socket_->bindServerSocket();
+
+    // Setup client side.
+    UnixDomainSocket socket(io_service_);
+
+    // We're going to asynchronously connect to the server. The boolean value
+    // below will be modified by the connect handler function (lambda) invoked
+    // when the connection is established or if an error occurs.
+    bool connect_handler_invoked = false;
+    ASSERT_NO_THROW(socket.asyncConnect(unixSocketFilePath(),
+        [this, &connect_handler_invoked](const boost::system::error_code& ec) {
+            // Indicate that the handler has been called so as the loop below gets
+            // interrupted.
+            connect_handler_invoked = true;
+            // Operation aborted indicates that IO service has been stopped. This
+            // shouldn't happen here.
+            if (ec && (ec.value() != boost::asio::error::operation_aborted)) {
+                ADD_FAILURE() << "error occurred while asynchronously connecting"
+                    " via unix domain socket: " << ec.message();
+            }
+        }
+    ));
+    // Run IO service until connect handler is invoked.
+    while (!connect_handler_invoked && (!test_socket_->isStopped())) {
+        io_service_.run_one();
+    }
+
+    // We are going to asynchronously send the 'foo' over the unix socket.
+    const std::string outbound_data = "foo";
+    size_t sent_size = 0;
+    ASSERT_NO_THROW(socket.asyncSend(outbound_data.c_str(), outbound_data.size(),
+        [this, &sent_size](const boost::system::error_code& ec, size_t length) {
+        // If we have been successful sending the data, record the number of
+        // bytes we have sent.
+        if (!ec) {
+            sent_size = length;
+
+        } else if (ec.value() != boost::asio::error::operation_aborted) {
+            ADD_FAILURE() << "error occurred while asynchronously sending the"
+            " data over unix domain socket: " << ec.message();
+        }
+    }
+    ));
+
+    // Run IO service to generate server's response.
+    while ((test_socket_->getResponseNum() < 1) &&
+           (!test_socket_->isStopped())) {
+        io_service_.run_one();
+    }
+
+    // There is no guarantee that all data have been sent so we only check that
+    // some data have been sent.
+    ASSERT_GT(sent_size, 0);
+
+    std::string expected_response = "received foo";
+    doReceive(socket, expected_response);
+
+    // Run IO service until we get the full response from the server.
+    while ((response_.size() < expected_response.size()) &&
+           !test_socket_->isStopped()) {
+        io_service_.run_one();
+    }
+
+    // Check that the entire response has been received and is correct.
+    EXPECT_EQ(expected_response, response_);
+}
+
 // This test verifies that UnixDomainSocketError exception is thrown
 // on attempt to connect, write or receive when the server socket
 // is not available.
@@ -123,11 +239,64 @@ TEST_F(UnixDomainSocketTest, clientErrors) {
                  UnixDomainSocketError);
 }
 
+// This test verifies that an error is returned on attempt to asynchronously
+// connect, write or receive when the server socket is not available.
+TEST_F(UnixDomainSocketTest, asyncClientErrors) {
+    UnixDomainSocket socket(io_service_);
+
+    // Asynchronous operations signal errors through boost::system::error_code
+    // object passed to the handler function. This object casts to boolean.
+    // In case of success the object casts to false. In case of an error it
+    // casts to true. The actual error codes can be retrieved by comparing the
+    // ec objects to predefined error objects. We don't check for the actual
+    // errors here, because it is not certain that the same error codes would
+    // be returned on various operating systems.
+
+    // In the following tests we use C++11 lambdas as callbacks.
+
+    // Connect
+    bool connect_handler_invoked = false;
+    socket.asyncConnect(unixSocketFilePath(),
+        [this, &connect_handler_invoked](const boost::system::error_code& ec) {
+        connect_handler_invoked = true;
+        EXPECT_TRUE(ec);
+    });
+    while (!connect_handler_invoked && !test_socket_->isStopped()) {
+        io_service_.run_one();
+    }
+
+    // Send
+    const std::string outbound_data = "foo";
+    bool send_handler_invoked = false;
+    socket.asyncSend(outbound_data.c_str(), outbound_data.size(),
+        [this, &send_handler_invoked]
+        (const boost::system::error_code& ec, size_t length) {
+        send_handler_invoked = true;
+        EXPECT_TRUE(ec);
+    });
+    while (!send_handler_invoked && !test_socket_->isStopped()) {
+        io_service_.run_one();
+    }
+
+    // Receive
+    bool receive_handler_invoked = false;
+    std::array<char, 1024> read_buf;
+    socket.asyncReceive(&read_buf[0], read_buf.size(),
+        [this, &receive_handler_invoked]
+        (const boost::system::error_code& ec, size_t length) {
+        receive_handler_invoked = true;
+        EXPECT_TRUE(ec);
+    });
+    while (!receive_handler_invoked && !test_socket_->isStopped()) {
+        io_service_.run_one();
+    }
+}
+
 // Check that native socket descriptor is returned correctly when
 // the socket is connected.
 TEST_F(UnixDomainSocketTest, getNative) {
     // Start the server.
-    test_socket_.bindServerSocket();
+    test_socket_->bindServerSocket();
 
     // Setup client side.
     UnixDomainSocket socket(io_service_);

src/lib/asiolink/testutils/test_server_unix_socket.cc

@@ -7,9 +7,11 @@
 #include <asiolink/asio_wrapper.h>
 #include <asiolink/testutils/test_server_unix_socket.h>
 #include <boost/bind.hpp>
+#include <boost/enable_shared_from_this.hpp>
 #include <boost/shared_ptr.hpp>
 #include <functional>
 #include <set>
+#include <sstream>
 
 using namespace boost::asio::local;
 
@@ -29,7 +31,7 @@ typedef std::function<void()> SentResponseCallback;
 /// @brief Connection to the server over unix domain socket.
 ///
 /// It reads the data over the socket, sends responses and closes a socket.
-class Connection {
+class Connection : public boost::enable_shared_from_this<Connection> {
 public:
 
     /// @brief Constructor.
@@ -43,11 +45,22 @@ public:
     /// server sends a response.
     Connection(const UnixSocketPtr& unix_socket,
                const std::string custom_response,
-               const SentResponseCallback& sent_response_callback)
+               SentResponseCallback sent_response_callback)
         : socket_(unix_socket), custom_response_(custom_response),
           sent_response_callback_(sent_response_callback) {
+    }
+
+    /// @brief Starts asynchronous read from the socket.
+    void start() {
        socket_->async_read_some(boost::asio::buffer(&raw_buf_[0], raw_buf_.size()),
-           boost::bind(&Connection::readHandler, this, _1, _2));
+           boost::bind(&Connection::readHandler, shared_from_this(),
+                       boost::asio::placeholders::error,
+                       boost::asio::placeholders::bytes_transferred));
+    }
+
+    /// @brief Closes the socket.
+    void stop() {
+        socket_->close();
     }
 
     /// @brief Handler invoked when data have been received over the socket.
@@ -79,15 +92,12 @@ public:
                 boost::asio::buffer(response.c_str(), response.size()));
         }
 
+        start();
+
         // Invoke callback function to notify that the response has been sent.
         sent_response_callback_();
     }
 
-    /// @brief Closes the socket.
-    void stop() {
-        socket_->close();
-    }
-
 private:
 
     /// @brief Pointer to the unix domain socket.
@@ -153,6 +163,7 @@ public:
         ConnectionPtr conn(new Connection(next_socket_, custom_response, [this] {
             ++response_num_;
         }));
+        conn->start();
 
         connections_.insert(conn);
         next_socket_.reset();
@@ -200,28 +211,59 @@ private: