Fix off-by-one error in connection pooling introduced with asio certificate changes in 2.10.4 (#920)

* Fix off-by-one error introduced in connection pool stack.

This got introduced when the connection pool queue was changed into a stack to reuse hot connections.

* Make connection_pool_stack testable and add tests.

Author: BillyONeal

Release/src/CMakeLists.txt

@@ -16,12 +16,13 @@ set(SOURCES
   ${HEADERS_DETAILS}
   pch/stdafx.h
   http/client/http_client.cpp
-  http/client/http_client_msg.cpp
   http/client/http_client_impl.h
-  http/common/internal_http_helpers.h
+  http/client/http_client_msg.cpp
+  http/common/connection_pool_helpers.h
+  http/common/http_compression.cpp
   http/common/http_helpers.cpp
   http/common/http_msg.cpp
-  http/common/http_compression.cpp
+  http/common/internal_http_helpers.h
   http/listener/http_listener.cpp
   http/listener/http_listener_msg.cpp
   http/listener/http_server_api.cpp

Release/src/http/client/http_client_asio.cpp

@@ -17,6 +17,7 @@
 #include <sstream>
 
 #include "../common/internal_http_helpers.h"
+#include "../common/connection_pool_helpers.h"
 #include "cpprest/asyncrt_utils.h"
 
 #if defined(__clang__)
@@ -345,48 +346,6 @@ class asio_connection
     bool m_closed;
 };
 
-class connection_pool_stack
-{
-public:
-    // attempts to acquire a connection from the deque; returns nullptr if no connection is
-    // available
-    std::shared_ptr<asio_connection> try_acquire() CPPREST_NOEXCEPT
-    {
-        const size_t oldConnectionsSize = m_connections.size();
-        if (m_highWater > oldConnectionsSize)
-        {
-            m_highWater = oldConnectionsSize;
-        }
-
-        if (oldConnectionsSize == 0)
-        {
-            return nullptr;
-        }
-
-        auto result = std::move(m_connections.back());
-        m_connections.pop_back();
-        return result;
-    }
-
-    // releases `released` back to the connection pool
-    void release(std::shared_ptr<asio_connection>&& released)
-    {
-        m_connections.push_back(std::move(released));
-    }
-
-    bool free_stale_connections() CPPREST_NOEXCEPT
-    {
-        m_connections.erase(m_connections.begin(), m_connections.begin() + m_highWater);
-        const size_t connectionsSize = m_connections.size();
-        m_highWater = connectionsSize;
-        return (connectionsSize != 0);
-    }
-
-private:
-    size_t m_highWater = 0;
-    std::vector<std::shared_ptr<asio_connection>> m_connections;
-};
-
 /// <summary>Implements a connection pool with adaptive connection removal</summary>
 /// <remarks>
 /// Every 30 seconds, the lambda in `start_epoch_interval` fires, triggering the
@@ -501,7 +460,7 @@ class asio_connection_pool final : public std::enable_shared_from_this<asio_conn
     }
 
     std::mutex m_lock;
-    std::map<std::string, connection_pool_stack> m_connections;
+    std::map<std::string, connection_pool_stack<asio_connection>> m_connections;
     bool m_is_timer_running;
     boost::asio::deadline_timer m_pool_epoch_timer;
 };

Release/src/http/common/connection_pool_helpers.h

@@ -0,0 +1,66 @@
+#pragma once
+
+#include "cpprest/details/cpprest_compat.h"
+#include <stddef.h>
+#include <memory>
+#include <vector>
+
+namespace web
+{
+namespace http
+{
+namespace client
+{
+namespace details
+{
+
+template<class ConnectionIsh>
+class connection_pool_stack
+{
+public:
+    // attempts to acquire a connection from the deque; returns nullptr if no connection is
+    // available
+    std::shared_ptr<ConnectionIsh> try_acquire() CPPREST_NOEXCEPT
+    {
+        const size_t oldConnectionsSize = m_connections.size();
+        if (oldConnectionsSize == 0)
+        {
+            m_staleBefore = 0;
+            return nullptr;
+        }
+
+        auto result = std::move(m_connections.back());
+        m_connections.pop_back();
+        const size_t newConnectionsSize = m_connections.size();
+        if (m_staleBefore > newConnectionsSize)
+        {
+            m_staleBefore = newConnectionsSize;
+        }
+
+        return result;
+    }
+
+    // releases `released` back to the connection pool
+    void release(std::shared_ptr<ConnectionIsh>&& released)
+    {
+        m_connections.push_back(std::move(released));
+    }
+
+    bool free_stale_connections() CPPREST_NOEXCEPT
+    {
+        assert(m_staleBefore <= m_connections.size());
+        m_connections.erase(m_connections.begin(), m_connections.begin() + m_staleBefore);
+        const size_t connectionsSize = m_connections.size();
+        m_staleBefore = connectionsSize;
+        return (connectionsSize != 0);
+    }
+
+private:
+    std::vector<std::shared_ptr<ConnectionIsh>> m_connections;
+    size_t m_staleBefore = 0;
+};
+
+} // details
+} // client
+} // http
+} // web

Release/tests/functional/http/client/CMakeLists.txt

@@ -2,8 +2,11 @@ set(SOURCES
   authentication_tests.cpp
   building_request_tests.cpp
   client_construction.cpp
+  compression_tests.cpp
+  connection_pool_tests.cpp
   connections_and_errors.cpp
   header_tests.cpp
+  http_client_fuzz_tests.cpp
   http_client_tests.cpp
   http_methods_tests.cpp
   multiple_requests.cpp
@@ -20,8 +23,6 @@ set(SOURCES
   response_stream_tests.cpp
   status_code_reason_phrase_tests.cpp
   to_string_tests.cpp
-  http_client_fuzz_tests.cpp
-  compression_tests.cpp
 )
 
 add_casablanca_test(httpclient_test SOURCES)

Release/tests/functional/http/client/connection_pool_tests.cpp

@@ -0,0 +1,45 @@
+#include "stdafx.h"
+#include <memory>
+#include "../../../src/http/common/connection_pool_helpers.h"
+
+using namespace web::http::client::details;
+
+SUITE(connection_pooling) {
+    TEST(empty_returns_nullptr) {
+        connection_pool_stack<int> connectionStack;
+        VERIFY_ARE_EQUAL(connectionStack.try_acquire(), std::shared_ptr<int>{});
+    }
+
+    static int noisyCount = 0;
+    struct noisy {
+        noisy() = delete;
+        noisy(int) { ++noisyCount; }
+        noisy(const noisy&) = delete;
+        noisy(noisy&&) { ++noisyCount; }
+        noisy& operator=(const noisy&) = delete;
+        noisy& operator=(noisy&&) = delete;
+        ~noisy() { --noisyCount; }
+    };
+
+    TEST(cycled_connections_survive) {
+        connection_pool_stack<noisy> connectionStack;
+        VERIFY_ARE_EQUAL(0, noisyCount);
+        connectionStack.release(std::make_shared<noisy>(42));
+        connectionStack.release(std::make_shared<noisy>(42));
+        connectionStack.release(std::make_shared<noisy>(42));
+        VERIFY_ARE_EQUAL(3, noisyCount);
+        VERIFY_IS_TRUE(connectionStack.free_stale_connections());
+        auto tmp = connectionStack.try_acquire();
+        VERIFY_ARE_NOT_EQUAL(tmp, std::shared_ptr<noisy>{});
+        connectionStack.release(std::move(tmp));
+        VERIFY_ARE_EQUAL(tmp, std::shared_ptr<noisy>{});
+        tmp = connectionStack.try_acquire();
+        VERIFY_ARE_NOT_EQUAL(tmp, std::shared_ptr<noisy>{});
+        connectionStack.release(std::move(tmp));
+        VERIFY_IS_TRUE(connectionStack.free_stale_connections());
+        VERIFY_ARE_EQUAL(1, noisyCount);
+        VERIFY_IS_FALSE(connectionStack.free_stale_connections());
+        VERIFY_ARE_EQUAL(0, noisyCount);
+        VERIFY_IS_FALSE(connectionStack.free_stale_connections());
+    }
+};