Skip to content

Commit 42c7bf6

Browse files
deanberrisdeanberris
committed
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.
1 parent 938733b commit 42c7bf6

File tree

5 files changed

+64
-82
lines changed

5 files changed

+64
-82
lines changed

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,15 +62,13 @@ namespace boost { namespace network { namespace http { namespace impl {
6262
bool is_open() { return socket_.is_open(); }
6363

6464
void close_socket() {
65+
if (!is_open()) return;
6566
boost::system::error_code ignored;
6667
socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored);
68+
if (ignored) return;
6769
socket_.close(ignored);
6870
}
6971

70-
~http_sync_connection() {
71-
close_socket();
72-
}
73-
7472
private:
7573

7674
resolver_type & resolver_;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ namespace boost { namespace network { namespace http { namespace impl {
7676
void close_socket() {
7777
boost::system::error_code ignored;
7878
socket_.lowest_layer().shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored);
79+
if (ignored) return;
7980
socket_.lowest_layer().close(ignored);
8081
}
8182

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

Lines changed: 16 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
#include <boost/network/protocol/http/traits/connection_policy.hpp>
1717
#include <boost/network/protocol/http/client/async_impl.hpp>
18+
#include <boost/network/protocol/http/client/sync_impl.hpp>
1819

1920
namespace boost { namespace network { namespace http {
2021

@@ -27,63 +28,23 @@ namespace boost { namespace network { namespace http {
2728
struct async_client;
2829

2930
template <class Tag, unsigned version_major, unsigned version_minor>
30-
struct sync_client :
31-
connection_policy<Tag, version_major, version_minor>::type
32-
{
33-
typedef typename string<Tag>::type string_type;
34-
typedef typename connection_policy<Tag,version_major,version_minor>::type connection_base;
35-
typedef typename resolver<Tag>::type resolver_type;
36-
friend struct basic_client_impl<Tag,version_major,version_minor>;
37-
38-
boost::asio::io_service * service_ptr;
39-
boost::asio::io_service & service_;
40-
resolver_type resolver_;
41-
optional<string_type> certificate_file, verify_path;
42-
43-
sync_client(bool cache_resolved, bool follow_redirect
44-
, optional<string_type> const & certificate_file = optional<string_type>()
45-
, optional<string_type> const & verify_path = optional<string_type>()
46-
)
47-
: connection_base(cache_resolved, follow_redirect),
48-
service_ptr(new boost::asio::io_service),
49-
service_(*service_ptr),
50-
resolver_(service_)
51-
, certificate_file(certificate_file)
52-
, verify_path(verify_path)
53-
{}
54-
55-
sync_client(bool cache_resolved, bool follow_redirect, boost::asio::io_service & service
56-
, optional<string_type> const & certificate_file = optional<string_type>()
57-
, optional<string_type> const & verify_path = optional<string_type>()
58-
)
59-
: connection_base(cache_resolved, follow_redirect),
60-
service_ptr(0),
61-
service_(service),
62-
resolver_(service_)
63-
, certificate_file(certificate_file)
64-
, verify_path(verify_path)
65-
{}
66-
67-
~sync_client() {
68-
delete service_ptr;
69-
}
70-
71-
basic_response<Tag> const request_skeleton(basic_request<Tag> const & request_, string_type method, bool get_body) {
72-
typename connection_base::connection_ptr connection_;
73-
connection_ = connection_base::get_connection(resolver_, request_, certificate_file, verify_path);
74-
return connection_->send_request(method, request_, get_body);
75-
}
76-
31+
struct sync_client;
32+
33+
34+
template <class Tag, unsigned version_major, unsigned version_minor, class Enable = void>
35+
struct client_base {
36+
typedef unsupported_tag<Tag> type;
7737
};
78-
38+
7939
template <class Tag, unsigned version_major, unsigned version_minor>
80-
struct client_base
81-
: mpl::if_<
82-
typename is_async<Tag>::type,
83-
async_client<Tag,version_major,version_minor>,
84-
sync_client<Tag,version_major,version_minor>
85-
>
86-
{};
40+
struct client_base<Tag, version_major, version_minor, typename enable_if<is_async<Tag> >::type> {
41+
typedef async_client<Tag,version_major,version_minor> type;
42+
};
43+
44+
template <class Tag, unsigned version_major, unsigned version_minor>
45+
struct client_base<Tag, version_major, version_minor, typename enable_if<is_sync<Tag> >::type> {
46+
typedef sync_client<Tag,version_major,version_minor> type;
47+
};
8748

8849
} // namespace impl
8950

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

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,29 +13,56 @@ namespace boost { namespace network { namespace http {
1313

1414
namespace impl {
1515
template <class Tag, unsigned version_major, unsigned version_minor>
16-
struct sync_client :
17-
connection_policy<Tag,version_major,version_minor>::type
16+
struct sync_client :
17+
connection_policy<Tag, version_major, version_minor>::type
1818
{
19-
typedef
20-
typename connection_policy<Tag,version_major,version_minor>::type
21-
connection_base;
19+
typedef typename string<Tag>::type string_type;
20+
typedef typename connection_policy<Tag,version_major,version_minor>::type connection_base;
21+
typedef typename resolver<Tag>::type resolver_type;
22+
friend struct basic_client_impl<Tag,version_major,version_minor>;
2223

23-
sync_client(bool cache_resolved, bool follow_redirect)
24-
: connection_base(cache_resolved, follow_redirect)
24+
boost::asio::io_service * service_ptr;
25+
boost::asio::io_service & service_;
26+
resolver_type resolver_;
27+
optional<string_type> certificate_file, verify_path;
28+
29+
sync_client(bool cache_resolved, bool follow_redirect
30+
, optional<string_type> const & certificate_file = optional<string_type>()
31+
, optional<string_type> const & verify_path = optional<string_type>()
32+
)
33+
: connection_base(cache_resolved, follow_redirect),
34+
service_ptr(new boost::asio::io_service),
35+
service_(*service_ptr),
36+
resolver_(service_)
37+
, certificate_file(certificate_file)
38+
, verify_path(verify_path)
39+
{}
40+
41+
sync_client(bool cache_resolved, bool follow_redirect, boost::asio::io_service & service
42+
, optional<string_type> const & certificate_file = optional<string_type>()
43+
, optional<string_type> const & verify_path = optional<string_type>()
44+
)
45+
: connection_base(cache_resolved, follow_redirect),
46+
service_ptr(0),
47+
service_(service),
48+
resolver_(service_)
49+
, certificate_file(certificate_file)
50+
, verify_path(verify_path)
2551
{}
2652

27-
~sync_client()
28-
{
53+
~sync_client() {
2954
connection_base::cleanup();
55+
delete service_ptr;
3056
}
31-
57+
3258
basic_response<Tag> const request_skeleton(basic_request<Tag> const & request_, string_type method, bool get_body) {
3359
typename connection_base::connection_ptr connection_;
34-
connection_ = connection_base::get_connection(resolver_, request_);
60+
connection_ = connection_base::get_connection(resolver_, request_, certificate_file, verify_path);
3561
return connection_->send_request(method, request_, get_body);
3662
}
3763

3864
};
65+
3966
} // namespace impl
4067

4168
} // namespace http

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

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ namespace boost { namespace network { namespace http {
2929
typedef typename resolver_policy<Tag>::type resolver_base;
3030
typedef typename resolver_base::resolver_type resolver_type;
3131
typedef function<typename resolver_base::resolver_iterator_pair(resolver_type &, string_type const &, string_type const &)> resolver_function_type;
32+
33+
void cleanup() {
34+
host_connection_map().swap(host_connections);
35+
}
3236

3337
struct connection_impl {
3438
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 {
4650
return send_request_impl(method, request_, get_body);
4751
}
4852

49-
~connection_impl () {
50-
pimpl->close_socket();
51-
pimpl.reset();
52-
}
53-
5453
private:
5554

5655
basic_response<Tag> send_request_impl(string_type const & method, basic_request<Tag> request_, bool get_body) {
5756
boost::uint8_t count = 0;
5857
bool retry = false;
5958
do {
6059
if (count >= BOOST_NETWORK_HTTP_MAXIMUM_REDIRECT_COUNT)
61-
throw std::runtime_error("Redirection exceeds maximum redirect count.");
60+
boost::throw_exception(std::runtime_error("Redirection exceeds maximum redirect count."));
6261

6362
basic_response<Tag> response_;
6463
// check if the socket is open first
@@ -110,7 +109,7 @@ namespace boost { namespace network { namespace http {
110109
connection_ = get_connection_(resolver_, request_, certificate_filename_, verify_path_);
111110
++count;
112111
continue;
113-
} else throw std::runtime_error("Location header not defined in redirect response.");
112+
} else boost::throw_exception(std::runtime_error("Location header not defined in redirect response."));
114113
}
115114
}
116115
return response_;
@@ -162,13 +161,9 @@ namespace boost { namespace network { namespace http {
162161
return it->second;
163162
}
164163

165-
void cleanup() {
166-
host_connection_map().swap(host_connections);
167-
}
168-
169164
pooled_connection_policy(bool cache_resolved, bool follow_redirect)
170165
: resolver_base(cache_resolved), host_connections(), follow_redirect_(follow_redirect) {}
171-
166+
172167
};
173168

174169
} // namespace http

0 commit comments

Comments
 (0)