Fixing cleanup order bug in pooled connection policy

Because of the refactorings that happened in the client implementation certain cleanup sequeneces
that used to not be an issue is now causing some errors especially with the pooled connection policy. These cleanup issues have been partly addressed in this commit, addressing a destruction bug that is manifested in the HTTP Client get tests when a synchronous HTTP Client with keepalive
support enabled in the tag used is destroyed.

The actual bug is that the io_service is destroyed first before the actual pooled connection policy
cleanup code is invoked, causing a segmentation fault with sockets bound to the io_service object
is destroyed because the internal state of the io_service referred to is already undefined by the
time the pool of sockets is destroyed.

Author: deanberris

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

@@ -62,15 +62,13 @@ namespace boost { namespace network { namespace http { namespace impl {
         bool is_open() { return socket_.is_open(); }
 
         void close_socket() {
+            if (!is_open()) return;
             boost::system::error_code ignored;
             socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored);
+            if (ignored) return;
             socket_.close(ignored);
         }
 
-        ~http_sync_connection() {
-            close_socket();
-        }
-
         private:
 
         resolver_type & resolver_;

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

@@ -76,6 +76,7 @@ namespace boost { namespace network { namespace http { namespace impl {
         void close_socket() { 
             boost::system::error_code ignored;
             socket_.lowest_layer().shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored);
+            if (ignored) return;
             socket_.lowest_layer().close(ignored);
         }
 

boost/network/protocol/http/client/pimpl.hpp

@@ -15,6 +15,7 @@
 
 #include <boost/network/protocol/http/traits/connection_policy.hpp>
 #include <boost/network/protocol/http/client/async_impl.hpp>
+#include <boost/network/protocol/http/client/sync_impl.hpp>
 
 namespace boost { namespace network { namespace http {
 
@@ -27,63 +28,23 @@ namespace boost { namespace network { namespace http {
         struct async_client;
 
         template <class Tag, unsigned version_major, unsigned version_minor>
-        struct sync_client :
-            connection_policy<Tag, version_major, version_minor>::type
-        {
-            typedef typename string<Tag>::type string_type;
-            typedef typename connection_policy<Tag,version_major,version_minor>::type connection_base;
-            typedef typename resolver<Tag>::type resolver_type;
-            friend struct basic_client_impl<Tag,version_major,version_minor>;
-
-            boost::asio::io_service * service_ptr;
-            boost::asio::io_service & service_;
-            resolver_type resolver_;
-            optional<string_type> certificate_file, verify_path;
-
-            sync_client(bool cache_resolved, bool follow_redirect
-                , optional<string_type> const & certificate_file = optional<string_type>()
-                , optional<string_type> const & verify_path = optional<string_type>()
-            )
-                : connection_base(cache_resolved, follow_redirect),
-                service_ptr(new boost::asio::io_service),
-                service_(*service_ptr),
-                resolver_(service_)
-                , certificate_file(certificate_file)
-                , verify_path(verify_path)
-            {}
-
-            sync_client(bool cache_resolved, bool follow_redirect, boost::asio::io_service & service
-                , optional<string_type> const & certificate_file = optional<string_type>()
-                , optional<string_type> const & verify_path = optional<string_type>()
-            )
-                : connection_base(cache_resolved, follow_redirect),
-                service_ptr(0),
-                service_(service),
-                resolver_(service_)
-                , certificate_file(certificate_file)
-                , verify_path(verify_path)
-            {}
-
-            ~sync_client() {
-                delete service_ptr;
-            }
-
-            basic_response<Tag> const request_skeleton(basic_request<Tag> const & request_, string_type method, bool get_body) {
-                typename connection_base::connection_ptr connection_;
-                connection_ = connection_base::get_connection(resolver_, request_, certificate_file, verify_path);
-                return connection_->send_request(method, request_, get_body);
-            }
-
+        struct sync_client;
+        
+        
+        template <class Tag, unsigned version_major, unsigned version_minor, class Enable = void>
+        struct client_base {
+            typedef unsupported_tag<Tag> type;
         };
-
+        
         template <class Tag, unsigned version_major, unsigned version_minor>
-        struct client_base
-            : mpl::if_<
-                typename is_async<Tag>::type,
-                async_client<Tag,version_major,version_minor>,
-                sync_client<Tag,version_major,version_minor>
-            >
-        {};
+        struct client_base<Tag, version_major, version_minor, typename enable_if<is_async<Tag> >::type> {
+            typedef async_client<Tag,version_major,version_minor> type;
+        };
+        
+        template <class Tag, unsigned version_major, unsigned version_minor>
+        struct client_base<Tag, version_major, version_minor, typename enable_if<is_sync<Tag> >::type> {
+            typedef sync_client<Tag,version_major,version_minor> type;
+        };
 
     } // namespace impl
 

boost/network/protocol/http/client/sync_impl.hpp

@@ -13,29 +13,56 @@ namespace boost { namespace network { namespace http {
 
     namespace impl {
         template <class Tag, unsigned version_major, unsigned version_minor>
-        struct sync_client :
-            connection_policy<Tag,version_major,version_minor>::type
+        struct sync_client  :
+            connection_policy<Tag, version_major, version_minor>::type
         {
-            typedef 
-                typename connection_policy<Tag,version_major,version_minor>::type
-                connection_base;
+            typedef typename string<Tag>::type string_type;
+            typedef typename connection_policy<Tag,version_major,version_minor>::type connection_base;
+            typedef typename resolver<Tag>::type resolver_type;
+            friend struct basic_client_impl<Tag,version_major,version_minor>;
 
-            sync_client(bool cache_resolved, bool follow_redirect)
-                : connection_base(cache_resolved, follow_redirect)
+            boost::asio::io_service * service_ptr;
+            boost::asio::io_service & service_;
+            resolver_type resolver_;
+            optional<string_type> certificate_file, verify_path;
+
+            sync_client(bool cache_resolved, bool follow_redirect
+                , optional<string_type> const & certificate_file = optional<string_type>()
+                , optional<string_type> const & verify_path = optional<string_type>()
+            )
+                : connection_base(cache_resolved, follow_redirect),
+                service_ptr(new boost::asio::io_service),
+                service_(*service_ptr),
+                resolver_(service_)
+                , certificate_file(certificate_file)
+                , verify_path(verify_path)
+            {}
+
+            sync_client(bool cache_resolved, bool follow_redirect, boost::asio::io_service & service
+                , optional<string_type> const & certificate_file = optional<string_type>()
+                , optional<string_type> const & verify_path = optional<string_type>()
+            )
+                : connection_base(cache_resolved, follow_redirect),
+                service_ptr(0),
+                service_(service),
+                resolver_(service_)
+                , certificate_file(certificate_file)
+                , verify_path(verify_path)
             {}
 
-            ~sync_client()
-            {
+            ~sync_client() {
                 connection_base::cleanup();
+                delete service_ptr;
             }
-            
+
             basic_response<Tag> const request_skeleton(basic_request<Tag> const & request_, string_type method, bool get_body) {
                 typename connection_base::connection_ptr connection_;
-                connection_ = connection_base::get_connection(resolver_, request_);
+                connection_ = connection_base::get_connection(resolver_, request_, certificate_file, verify_path);
                 return connection_->send_request(method, request_, get_body);
             }
 
         };
+        
     } // namespace impl
 
 } // namespace http

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

@@ -29,6 +29,10 @@ namespace boost { namespace network { namespace http {
         typedef typename resolver_policy<Tag>::type resolver_base;
         typedef typename resolver_base::resolver_type resolver_type;
         typedef function<typename resolver_base::resolver_iterator_pair(resolver_type &, string_type const &, string_type const &)> resolver_function_type;
+        
+        void cleanup() {
+            host_connection_map().swap(host_connections);
+        }
 
         struct connection_impl {
             typedef function<shared_ptr<connection_impl>(resolver_type &,basic_request<Tag> const &,optional<string_type> const &, optional<string_type> const &)> get_connection_function;
@@ -46,19 +50,14 @@ namespace boost { namespace network { namespace http {
                 return send_request_impl(method, request_, get_body);
             }
 
-            ~connection_impl () {
-                pimpl->close_socket();
-                pimpl.reset();
-            }
-
         private:
 
             basic_response<Tag> send_request_impl(string_type const & method, basic_request<Tag> request_, bool get_body) {
                 boost::uint8_t count = 0;
                 bool retry = false;
                 do {
                     if (count >= BOOST_NETWORK_HTTP_MAXIMUM_REDIRECT_COUNT)
-                        throw std::runtime_error("Redirection exceeds maximum redirect count.");
+                        boost::throw_exception(std::runtime_error("Redirection exceeds maximum redirect count."));
 
                     basic_response<Tag> response_;
                     // check if the socket is open first
@@ -110,7 +109,7 @@ namespace boost { namespace network { namespace http {
                                 connection_ = get_connection_(resolver_, request_, certificate_filename_, verify_path_);
                                 ++count;
                                 continue;
-                            } else throw std::runtime_error("Location header not defined in redirect response.");
+                            } else boost::throw_exception(std::runtime_error("Location header not defined in redirect response."));
                         }
                     }
                     return response_;
@@ -162,13 +161,9 @@ namespace boost { namespace network { namespace http {
             return it->second;
         }
 
-        void cleanup() {
-            host_connection_map().swap(host_connections);
-        }
-
         pooled_connection_policy(bool cache_resolved, bool follow_redirect)
         : resolver_base(cache_resolved), host_connections(), follow_redirect_(follow_redirect) {}
-
+        
     };
 
 } // namespace http