Fix some issues with the (deprecated) synchronous client implementation running under tsan; also require C++11 in GNU compilers moving forward by default

Author: deanberris

CMakeLists.txt

@@ -71,16 +71,12 @@ if (OPENSSL_FOUND)
 endif()
 
 if (${CMAKE_CXX_COMPILER_ID} MATCHES GNU)
-  set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall")
+  # Use C++11 when using GNU compilers.
+  set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -std=c++11")
 elseif (${CMAKE_CXX_COMPILER_ID} MATCHES Clang)
-  if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
-    # We want to link in C++11 mode if we're using Clang and on OS X.
-    set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -ftemplate-depth=256 -std=c++11 -stdlib=libc++")
-  else()
-    # We just add the -Wall and a high enough template depth
-    # flag for Clang in other systems.
-    set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -ftemplate-depth=256")
-  endif()
+  # We want to link in C++11 mode in Clang too, but also set a high enough
+  # template depth for the template metaprogramming.
+  set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -ftemplate-depth=256 -std=c++11 -stdlib=libc++")
 endif()
 
 

boost/network/protocol/http/client/connection/sync_normal.hpp

@@ -9,6 +9,7 @@
 // http://www.boost.org/LICENSE_1_0.txt)
 
 #include <boost/asio/deadline_timer.hpp>
+#include <boost/asio/streambuf.hpp>
 #include <boost/network/protocol/http/algorithms/linearize.hpp>
 #include <boost/network/protocol/http/response.hpp>
 #include <boost/network/protocol/http/traits/resolver_policy.hpp>
@@ -97,7 +98,8 @@ struct http_sync_connection
     connection_base::read_body(socket_, response_, response_buffer);
     typename headers_range<basic_response<Tag> >::type connection_range =
         headers(response_)["Connection"];
-    if (version_major == 1 && version_minor == 1 && !boost::empty(connection_range) &&
+    if (version_major == 1 && version_minor == 1 &&
+        !boost::empty(connection_range) &&
         boost::iequals(boost::begin(connection_range)->second, "close")) {
       close_socket();
     } else if (version_major == 1 && version_minor == 0) {
@@ -109,19 +111,22 @@ struct http_sync_connection
 
   void close_socket() {
     timer_.cancel();
-    if (!is_open()) { return;
-}
+    if (!is_open()) {
+      return;
+    }
     boost::system::error_code ignored;
     socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored);
-    if (ignored != nullptr) { return;
-}
+    if (ignored != nullptr) {
+      return;
+    }
     socket_.close(ignored);
   }
 
  private:
   void handle_timeout(boost::system::error_code const& ec) {
-    if (!ec) { close_socket();
-}
+    if (!ec) {
+      close_socket();
+    }
   }
 
   int timeout_;

boost/network/protocol/http/policies/pooled_connection.hpp

@@ -7,12 +7,12 @@
 // (See accompanying file LICENSE_1_0.txt or copy at
 // http://www.boost.org/LICENSE_1_0.txt)
 
-#include <boost/network/protocol/http/traits/resolver_policy.hpp>
-
 #include <boost/algorithm/string/predicate.hpp>
 #include <boost/network/protocol/http/client/connection/sync_base.hpp>
 #include <boost/network/protocol/http/response.hpp>
+#include <boost/network/protocol/http/traits/resolver_policy.hpp>
 #include <boost/shared_ptr.hpp>
+#include <boost/thread/mutex.hpp>
 #include <boost/unordered_map.hpp>
 #include <utility>
 
@@ -37,7 +37,7 @@ struct pooled_connection_policy : resolver_policy<Tag>::type {
                         system::error_code const&)> body_callback_function_type;
   typedef function<bool(string_type&)> body_generator_function_type;
 
-  void cleanup() { host_connection_map().swap(host_connections); }
+  void cleanup() { host_connection_map().swap(host_connections_); }
 
   struct connection_impl {
     typedef function<shared_ptr<connection_impl>(
@@ -47,10 +47,10 @@ struct pooled_connection_policy : resolver_policy<Tag>::type {
         get_connection_function;
 
     connection_impl(
-        resolver_type& resolver, bool follow_redirect, string_type  /*unused*/const& host,
-        string_type const& port, resolver_function_type resolve,
-        get_connection_function get_connection, bool https,
-        bool always_verify_peer, int timeout,
+        resolver_type& resolver, bool follow_redirect,
+        string_type /*unused*/ const& host, string_type const& port,
+        resolver_function_type resolve, get_connection_function get_connection,
+        bool https, bool always_verify_peer, int timeout,
         optional<string_type> const& certificate_filename =
             optional<string_type>(),
         optional<string_type> const& verify_path = optional<string_type>(),
@@ -77,7 +77,7 @@ struct pooled_connection_policy : resolver_policy<Tag>::type {
       (void)port;
     }
 
-    basic_response<Tag> send_request(string_type  /*unused*/const& method,
+    basic_response<Tag> send_request(string_type const& method,
                                      basic_request<Tag> request_, bool get_body,
                                      body_callback_function_type callback,
                                      body_generator_function_type generator) {
@@ -86,7 +86,7 @@ struct pooled_connection_policy : resolver_policy<Tag>::type {
 
    private:
     basic_response<Tag> send_request_impl(
-        string_type  /*unused*/const& method, basic_request<Tag> request_, bool get_body,
+        string_type const& method, basic_request<Tag> request_, bool get_body,
         body_callback_function_type callback,
         body_generator_function_type generator) {
       // TODO(dberris): review parameter necessity.
@@ -113,8 +113,7 @@ struct pooled_connection_policy : resolver_policy<Tag>::type {
 
         try {
           pimpl->read_status(response_, response_buffer);
-        }
-        catch (boost::system::system_error& e) {
+        } catch (boost::system::system_error& e) {
           if (!retry && e.code() == boost::asio::error::eof) {
             retry = true;
             pimpl->init_socket(request_.host(),
@@ -182,49 +181,57 @@ struct pooled_connection_policy : resolver_policy<Tag>::type {
 
   typedef shared_ptr<connection_impl> connection_ptr;
 
-  typedef unordered_map<string_type, connection_ptr> host_connection_map;
-  host_connection_map host_connections;
+  typedef unordered_map<string_type, weak_ptr<connection_impl>> host_connection_map;
+  boost::mutex host_mutex_;
+  host_connection_map host_connections_;
   bool follow_redirect_;
   int timeout_;
 
   connection_ptr get_connection(
       resolver_type& resolver, basic_request<Tag> const& request_,
       bool always_verify_peer,
-      optional<string_type>  /*unused*/const& certificate_filename =
+      optional<string_type> const& certificate_filename =
           optional<string_type>(),
       optional<string_type> const& verify_path = optional<string_type>(),
       optional<string_type> const& certificate_file = optional<string_type>(),
       optional<string_type> const& private_key_file = optional<string_type>(),
       optional<string_type> const& ciphers = optional<string_type>()) {
     string_type index =
         (request_.host() + ':') + lexical_cast<string_type>(request_.port());
-    connection_ptr connection_;
-    typename host_connection_map::iterator it = host_connections.find(index);
-    if (it == host_connections.end()) {
-      connection_.reset(new connection_impl(
-          resolver, follow_redirect_, request_.host(),
-          lexical_cast<string_type>(request_.port()),
-          boost::bind(&pooled_connection_policy<Tag, version_major,
-                                                version_minor>::resolve,
-                      this, boost::arg<1>(), boost::arg<2>(), boost::arg<3>()),
-          boost::bind(&pooled_connection_policy<Tag, version_major,
-                                                version_minor>::get_connection,
-                      this, boost::arg<1>(), boost::arg<2>(),
-                      always_verify_peer, boost::arg<3>(), boost::arg<4>(),
-                      boost::arg<5>(), boost::arg<6>(), boost::arg<7>()),
-          boost::iequals(request_.protocol(), string_type("https")),
-          always_verify_peer, timeout_, certificate_filename, verify_path,
-          certificate_file, private_key_file, ciphers, 0));
-      host_connections.insert(std::make_pair(index, connection_));
-      return connection_;
+    boost::mutex::scoped_lock lock(host_mutex_);
+    auto it = host_connections_.find(index);
+    if (it != host_connections_.end()) {
+      // We've found an existing connection; but we should check if that
+      // connection hasn't been deleted yet.
+      auto result = it->second.lock();
+      if (!result) return result;
     }
-    return it->second;
+
+    connection_ptr connection(new connection_impl(
+        resolver, follow_redirect_, request_.host(),
+        lexical_cast<string_type>(request_.port()),
+        // resolver function
+        boost::bind(&pooled_connection_policy<Tag, version_major,
+                                              version_minor>::resolve,
+                    this, boost::arg<1>(), boost::arg<2>(), boost::arg<3>()),
+        // connection factory
+        boost::bind(&pooled_connection_policy<Tag, version_major,
+                                              version_minor>::get_connection,
+                    this, boost::arg<1>(), boost::arg<2>(), always_verify_peer,
+                    boost::arg<3>(), boost::arg<4>(), boost::arg<5>(),
+                    boost::arg<6>(), boost::arg<7>()),
+        boost::iequals(request_.protocol(), string_type("https")),
+        always_verify_peer, timeout_, certificate_filename, verify_path,
+        certificate_file, private_key_file, ciphers, 0));
+    host_connections_.insert(std::make_pair(index, connection));
+    return connection;
   }
 
   pooled_connection_policy(bool cache_resolved, bool follow_redirect,
                            int timeout)
       : resolver_base(cache_resolved),
-        host_connections(),
+        host_mutex_(),
+        host_connections_(),
         follow_redirect_(follow_redirect),
         timeout_(timeout) {}
 };