[#1730] Thread-safety issues per review comments

src/lib/config/tests/cmd_http_listener_unittests.cc
    - replaced used of io_service::restart() with reset()

src/lib/http/connection_pool.cc
src/lib/http/connection_pool.h
    - made HttpConnectionPool thread-safe

src/lib/http/tests/connection_pool_unittests.cc
    - added MT tests

src/lib/http/tests/test_http_client.h
    - corrected spelling

diff --git a/src/lib/config/tests/cmd_http_listener_unittests.cc b/src/lib/config/tests/cmd_http_listener_unittests.cc
index e3feacac5d9e29fd9f3a1ea723a030d88c3a2b14..3d0f1e57f474585c7e4364c66e432da6cf5fef31 100644 (file)
--- a/src/lib/config/tests/cmd_http_listener_unittests.cc
+++ b/src/lib/config/tests/cmd_http_listener_unittests.cc
@@ -53,7 +53,8 @@ public:
     /// from CommandMgr, and enables multi-threading mode.
     CmdHttpListenerTest()
         : io_service_(), test_timer_(io_service_), run_io_service_timer_(io_service_),
-        clients_(), num_threads_(), num_clients_(), num_in_progress_(0), num_finished_(0) {
+        clients_(), num_threads_(), num_clients_(), num_in_progress_(0), num_finished_(0),
+        chunk_size_(0) {
         test_timer_.setup(std::bind(&CmdHttpListenerTest::timeoutHandler, this, true),
                           TEST_TIMEOUT, IntervalTimer::ONE_SHOT);
 
@@ -180,8 +181,8 @@ public:
         // Loop until the clients are done, an error occurs, or the time runs out.
         bool keep_going = true;
         while (keep_going) {
-            // Always call restart() before we call run();
-            io_service_.get_io_service().restart();
+            // Always call reset() before we call run();
+            io_service_.get_io_service().reset();
 
             // Run until a client stops the service.
             io_service_.run();
@@ -252,20 +253,20 @@ public:
         // notify everyone and finish.  The idea is to force each thread
         // to handle the same number of requests over the course of the
         // test, making verification reliable.
-        if (num_clients_ >= num_threads_) {
+        {
             std::unique_lock<std::mutex> lck(mutex_);
             ++num_in_progress_;
-            if (num_in_progress_ == num_threads_) {
+            if (num_in_progress_ == chunk_size_) {
                 num_finished_ = 0;
                 cv_.notify_all();
             } else {
                 bool ret = cv_.wait_for(lck, std::chrono::seconds(10),
-                                        [&]() { return (num_in_progress_ == num_threads_); });
+                                        [&]() { return (num_in_progress_ == chunk_size_); });
                 if (!ret) {
                     ADD_FAILURE() << "clients failed to start work";
                 }
             }
-         }
+        }
 
         // Create the map of response arguments.
         ElementPtr arguments = Element::createMap();
@@ -282,19 +283,17 @@ public:
         ss << std::this_thread::get_id();
         arguments->set("thread-id", Element::create(ss.str()));
 
-        // If we have more clients than threads, we need to wait
-        // for each block of in-progress clients to finish.
-        if (num_clients_ >= num_threads_) {
+        {
             std::unique_lock<std::mutex> lck(mutex_);
             num_finished_++;
-            if (num_finished_ == num_threads_) {
+            if (num_finished_ == chunk_size_) {
                 // We're all done, notify the others and finish.
                 num_in_progress_ = 0;
                 cv_.notify_all();
             } else {
                 // I'm done but others aren't wait here.
                 bool ret = cv_.wait_for(lck, std::chrono::seconds(10),
-                                        [&]() { return (num_finished_ == num_threads_); });
+                                        [&]() { return (num_finished_ == chunk_size_); });
                 if (!ret) {
                     ADD_FAILURE() << "clients failed to finish work";
                 }
@@ -332,6 +331,10 @@ public:
 
         num_threads_ = num_threads;
         num_clients_ = num_clients;
+        chunk_size_ = num_threads_;
+        if (num_clients_ < chunk_size_) {
+            chunk_size_ = num_clients_;
+        }
 
         // Register the thread command handler.
         CommandMgr::instance().registerCommand("thread",
@@ -484,6 +487,13 @@ public:
     /// @brief Number of requests that have finished.
     size_t num_finished_;
 
+    /// @brief Chunk size of requests that need to be processed in parallel.
+    ///
+    /// This can either be the number of threads (if the number of requests is
+    /// greater than the number of threads) or the number of requests (if the
+    /// number of threads is greater than the number of requests).
+    size_t chunk_size_;
+
     /// @brief Mutex used to lock during thread coordination.
     std::mutex mutex_;
 

diff --git a/src/lib/http/connection_pool.cc b/src/lib/http/connection_pool.cc
index db4fbe48ee53ab4de7430f4d14f035ba6a77eee1..5bf3284351757e2cb9d7e4bfd9a618bdb7bac62b 100644 (file)
--- a/src/lib/http/connection_pool.cc
+++ b/src/lib/http/connection_pool.cc
@@ -8,29 +8,53 @@
 
 #include <asiolink/asio_wrapper.h>
 #include <http/connection_pool.h>
+#include <util/multi_threading_mgr.h>
 
 namespace isc {
 namespace http {
 
 void
 HttpConnectionPool::start(const HttpConnectionPtr& connection) {
-    connections_.insert(connections_.end(), connection);
+    if (util::MultiThreadingMgr::instance().getMode()) {
+        std::lock_guard<std::mutex> lk(mutex_);
+        connections_.insert(connections_.end(), connection);
+    } else {
+        connections_.insert(connections_.end(), connection);
+    }
+
     connection->asyncAccept();
 }
 
 void
 HttpConnectionPool::stop(const HttpConnectionPtr& connection) {
-    connections_.remove(connection);
+    if (util::MultiThreadingMgr::instance().getMode()) {
+        std::lock_guard<std::mutex> lk(mutex_);
+        connections_.remove(connection);
+    } else {
+        connections_.remove(connection);
+    }
+
     connection->close();
 }
 
 void
 HttpConnectionPool::stopAll() {
+    if (util::MultiThreadingMgr::instance().getMode()) {
+        std::lock_guard<std::mutex> lk(mutex_);
+        stopAllInternal();
+    } else {
+        stopAllInternal();
+    }
+}
+
+void
+HttpConnectionPool::stopAllInternal() {
     for (auto connection = connections_.begin();
          connection != connections_.end();
          ++connection) {
         (*connection)->close();
     }
+
     connections_.clear();
 }
 

diff --git a/src/lib/http/connection_pool.h b/src/lib/http/connection_pool.h
index b5c19b0f561231b082fe44332d13ef2faca8941c..450b1451880a342ac31f9e178b9d4a1a049b7cf0 100644 (file)
--- a/src/lib/http/connection_pool.h
+++ b/src/lib/http/connection_pool.h
@@ -8,7 +8,9 @@
 #define HTTP_CONNECTION_POOL_H
 
 #include <http/connection.h>
+
 #include <list>
+#include <mutex>
 
 namespace isc {
 namespace http {
@@ -48,9 +50,16 @@ public:
 
 protected:
 
+    /// @brief Stops all connections and removes them from the pool.
+    ///
+    /// Must be called from with a thread-safe context.
+    void stopAllInternal();
+
     /// @brief Set of connections.
     std::list<HttpConnectionPtr> connections_;
 
+    /// @brief Mutex to protect the internal state.
+    std::mutex mutex_;
 };
 
 }

diff --git a/src/lib/http/tests/connection_pool_unittests.cc b/src/lib/http/tests/connection_pool_unittests.cc
index cd3093047b9ef4a10997d216df1522e00f47c347..fdbd18f4e5c0e48565538317615b4d82701f36e1 100644 (file)
--- a/src/lib/http/tests/connection_pool_unittests.cc
+++ b/src/lib/http/tests/connection_pool_unittests.cc
@@ -14,6 +14,8 @@
 #include <http/response_creator.h>
 #include <http/response_json.h>
 #include <http/tests/response_test.h>
+#include <util/multi_threading_mgr.h>
+
 #include <boost/shared_ptr.hpp>
 #include <gtest/gtest.h>
 #include <algorithm>
@@ -21,6 +23,7 @@
 using namespace isc::asiolink;
 using namespace isc::http;
 using namespace isc::http::test;
+using namespace isc::util;
 
 namespace {
 
@@ -104,6 +107,104 @@ public:
     HttpConnectionPoolTest()
         : io_service_(), acceptor_(io_service_), connection_pool_(),
           response_creator_(new TestHttpResponseCreator()) {
+        MultiThreadingMgr::instance().setMode(false);
+    }
+
+    /// @brief Destructor.
+    ~HttpConnectionPoolTest() {
+        MultiThreadingMgr::instance().setMode(false);
+    }
+
+    /// @brief Verifies that connections can be added to the pool and removed.
+    void startStopTest() {
+        // Create two distinct connections.
+        HttpConnectionPtr conn1(new HttpConnection(io_service_, acceptor_,
+                                                   connection_pool_,
+                                                   response_creator_,
+                                                   HttpAcceptorCallback(),
+                                                   CONN_REQUEST_TIMEOUT,
+                                                   CONN_IDLE_TIMEOUT));
+
+        HttpConnectionPtr conn2(new HttpConnection(io_service_, acceptor_,
+                                                   connection_pool_,
+                                                   response_creator_,
+                                                   HttpAcceptorCallback(),
+                                                   CONN_REQUEST_TIMEOUT,
+                                                   CONN_IDLE_TIMEOUT));
+        // The pool should be initially empty.
+        TestHttpConnectionPool pool;
+        ASSERT_TRUE(pool.connections_.empty());
+
+        // Start first connection and check that it has been added to the pool.
+        ASSERT_NO_THROW(pool.start(conn1));
+        ASSERT_EQ(1, pool.connections_.size());
+        ASSERT_EQ(1, pool.hasConnection(conn1));
+
+        // Start second connection and check that it also has been added.
+        ASSERT_NO_THROW(pool.start(conn2));
+        ASSERT_EQ(2, pool.connections_.size());
+        ASSERT_EQ(1, pool.hasConnection(conn2));
+
+        // Stop first connection.
+        ASSERT_NO_THROW(pool.stop(conn1));
+        ASSERT_EQ(1, pool.connections_.size());
+        // Check that it has been removed but the second connection is still
+        // there.
+        ASSERT_EQ(0, pool.hasConnection(conn1));
+        ASSERT_EQ(1, pool.hasConnection(conn2));
+
+        // Remove second connection and verify.
+        ASSERT_NO_THROW(pool.stop(conn2));
+        EXPECT_TRUE(pool.connections_.empty());
+    }
+
+    /// @brief Verifies that all connections can be remove with a single call.
+    void stopAllTest() {
+        // Create two distinct connections.
+        HttpConnectionPtr conn1(new HttpConnection(io_service_, acceptor_,
+                                                   connection_pool_,
+                                                   response_creator_,
+                                                   HttpAcceptorCallback(),
+                                                   CONN_REQUEST_TIMEOUT,
+                                                   CONN_IDLE_TIMEOUT));
+
+        HttpConnectionPtr conn2(new HttpConnection(io_service_, acceptor_,
+                                                   connection_pool_,
+                                                   response_creator_,
+                                                   HttpAcceptorCallback(),
+                                                   CONN_REQUEST_TIMEOUT,
+                                                   CONN_IDLE_TIMEOUT));
+        TestHttpConnectionPool pool;
+        ASSERT_NO_THROW(pool.start(conn1));
+        ASSERT_NO_THROW(pool.start(conn2));
+
+        // There are two distinct connections in the pool.
+        ASSERT_EQ(2, pool.connections_.size());
+
+        // This should remove all connections.
+        ASSERT_NO_THROW(pool.stopAll());
+        EXPECT_TRUE(pool.connections_.empty());
+    }
+
+    /// @brief Verifies that stopping a non-existing connection is no-op.
+    void stopInvalidTest() {
+        HttpConnectionPtr conn1(new HttpConnection(io_service_, acceptor_,
+                                                   connection_pool_,
+                                                   response_creator_,
+                                                   HttpAcceptorCallback(),
+                                                   CONN_REQUEST_TIMEOUT,
+                                                   CONN_IDLE_TIMEOUT));
+        HttpConnectionPtr conn2(new HttpConnection(io_service_, acceptor_,
+                                                   connection_pool_,
+                                                   response_creator_,
+                                                   HttpAcceptorCallback(),
+                                                   CONN_REQUEST_TIMEOUT,
+                                                   CONN_IDLE_TIMEOUT));
+        TestHttpConnectionPool pool;
+        ASSERT_NO_THROW(pool.start(conn1));
+        ASSERT_NO_THROW(pool.stop(conn2));
+        ASSERT_EQ(1, pool.connections_.size());
+        ASSERT_EQ(1, pool.hasConnection(conn1));
     }
 
     IOService io_service_;                      ///< IO service.
@@ -113,92 +214,48 @@ public:
 
 };
 
-// This test verifies that connections can be added to the pool and removed.
-TEST_F(HttpConnectionPoolTest, startStop) {
-    // Create two distinct connections.
-    HttpConnectionPtr conn1(new HttpConnection(io_service_, acceptor_,
-                                               connection_pool_,
-                                               response_creator_,
-                                               HttpAcceptorCallback(),
-                                               CONN_REQUEST_TIMEOUT,
-                                               CONN_IDLE_TIMEOUT));
-    HttpConnectionPtr conn2(new HttpConnection(io_service_, acceptor_,
-                                               connection_pool_,
-                                               response_creator_,
-                                               HttpAcceptorCallback(),
-                                               CONN_REQUEST_TIMEOUT,
-                                               CONN_IDLE_TIMEOUT));
-    // The pool should be initially empty.
-    TestHttpConnectionPool pool;
-    ASSERT_TRUE(pool.connections_.empty());
-
-    // Start first connection and check that it has been added to the pool.
-    ASSERT_NO_THROW(pool.start(conn1));
-    ASSERT_EQ(1, pool.connections_.size());
-    ASSERT_EQ(1, pool.hasConnection(conn1));
-
-    // Start second connection and check that it also has been added.
-    ASSERT_NO_THROW(pool.start(conn2));
-    ASSERT_EQ(2, pool.connections_.size());
-    ASSERT_EQ(1, pool.hasConnection(conn2));
-
-    // Stop first connection.
-    ASSERT_NO_THROW(pool.stop(conn1));
-    ASSERT_EQ(1, pool.connections_.size());
-    // Check that it has been removed but the second connection is still there.
-    ASSERT_EQ(0, pool.hasConnection(conn1));
-    ASSERT_EQ(1, pool.hasConnection(conn2));
-
-    // Remove second connection and verify.
-    ASSERT_NO_THROW(pool.stop(conn2));
-    EXPECT_TRUE(pool.connections_.empty());
+// Verifies that connections can be added to the pool and removed.
+// with MultiThreading disabled.
+TEST_F(HttpConnectionPoolTest, startStopTest) {
+    ASSERT_FALSE(MultiThreadingMgr::instance().getMode());
+    startStopTest();
+}
+
+// Verifies that connections can be added to the pool and removed
+// with MultiThreading enabled.
+TEST_F(HttpConnectionPoolTest, startStopTestMultiThreading) {
+    MultiThreadingMgr::instance().setMode(true);
+    startStopTest();
 }
 
 // Check that all connections can be remove with a single call.
+// with MultiThreading disabled.
 TEST_F(HttpConnectionPoolTest, stopAll) {
-    HttpConnectionPtr conn1(new HttpConnection(io_service_, acceptor_,
-                                               connection_pool_,
-                                               response_creator_,
-                                               HttpAcceptorCallback(),
-                                               CONN_REQUEST_TIMEOUT,
-                                               CONN_IDLE_TIMEOUT));
-    HttpConnectionPtr conn2(new HttpConnection(io_service_, acceptor_,
-                                               connection_pool_,
-                                               response_creator_,
-                                               HttpAcceptorCallback(),
-                                               CONN_REQUEST_TIMEOUT,
-                                               CONN_IDLE_TIMEOUT));
-    TestHttpConnectionPool pool;
-    ASSERT_NO_THROW(pool.start(conn1));
-    ASSERT_NO_THROW(pool.start(conn2));
-
-    // There are two distinct connections in the pool.
-    ASSERT_EQ(2, pool.connections_.size());
-
-    // This should remove all connections.
-    ASSERT_NO_THROW(pool.stopAll());
-    EXPECT_TRUE(pool.connections_.empty());
+    ASSERT_FALSE(MultiThreadingMgr::instance().getMode());
+    stopAllTest();
+}
+
+// Check that all connections can be remove with a single call
+// with MultiThreading enabled.
+TEST_F(HttpConnectionPoolTest, stopAllMultiThreading) {
+    MultiThreadingMgr::instance().setMode(true);
+    ASSERT_TRUE(MultiThreadingMgr::instance().getMode());
+    stopAllTest();
 }
 
 // Check that stopping non-existing connection is no-op.
+// with MultiThreading disabled.
 TEST_F(HttpConnectionPoolTest, stopInvalid) {
-    HttpConnectionPtr conn1(new HttpConnection(io_service_, acceptor_,
-                                               connection_pool_,
-                                               response_creator_,
-                                               HttpAcceptorCallback(),
-                                               CONN_REQUEST_TIMEOUT,
-                                               CONN_IDLE_TIMEOUT));
-    HttpConnectionPtr conn2(new HttpConnection(io_service_, acceptor_,
-                                               connection_pool_,
-                                               response_creator_,
-                                               HttpAcceptorCallback(),
-                                               CONN_REQUEST_TIMEOUT,
-                                               CONN_IDLE_TIMEOUT));
-    TestHttpConnectionPool pool;
-    ASSERT_NO_THROW(pool.start(conn1));
-    ASSERT_NO_THROW(pool.stop(conn2));
-    ASSERT_EQ(1, pool.connections_.size());
-    ASSERT_EQ(1, pool.hasConnection(conn1));
+    ASSERT_FALSE(MultiThreadingMgr::instance().getMode());
+    stopInvalidTest();
+}
+
+// Check that stopping non-existing connection is no-op.
+// with MultiThreading enabled.
+TEST_F(HttpConnectionPoolTest, stopInvalidMultiThreading) {
+    MultiThreadingMgr::instance().setMode(true);
+    ASSERT_TRUE(MultiThreadingMgr::instance().getMode());
+    stopInvalidTest();
 }
 
 }

diff --git a/src/lib/http/tests/test_http_client.h b/src/lib/http/tests/test_http_client.h
index f249d9f48df09b7f18ca70f1ad61b748a5ef22b9..f95b11189cf762c1fe82cc6e4413ebdc60439a07 100644 (file)
--- a/src/lib/http/tests/test_http_client.h
+++ b/src/lib/http/tests/test_http_client.h
@@ -230,14 +230,14 @@ public:
 
     /// @brief Returns the HTTP response string.
     ///
-    /// @retrurn string containg the response.
+    /// @return string containing the response.
     std::string getResponse() const {
         return (response_);
     }
 
     /// @brief Returns true if the receive completed without error.
     ///
-    /// @return True if the receive completed succesfully, false
+    /// @return True if the receive completed successfully, false
     /// otherwise.
     bool receiveDone() {
         return (receive_done_);