-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add host based connection pool map #156
add host based connection pool map #156
Conversation
Hi @hanzhumsft, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
@@ -325,32 +325,53 @@ class asio_connection_pool | |||
boost::asio::io_service& m_io_service; | |||
const int m_timeout_secs; | |||
const bool m_start_with_ssl; | |||
const std::function<void(boost::asio::ssl::context&)>& m_ssl_context_callback; | |||
std::function<void(boost::asio::ssl::context&)> m_ssl_context_callback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent a dangling reference, because the lifetime of asio_connection_pool
can be longer than the function object m_ssl_context_callback
references. That function object comes from http_client_config
when creating a http_client
object.
Running tests on Linux: **** outside_tests:outside_cnn_dot_com FAILED **** **** outside_tests:outside_google_dot_com FAILED **** **** outside_tests:multiple_https_requests FAILED **** **** outside_tests:reading_google_stream FAILED **** Finished running all 777 tests. All 4 cases failed without my change (1e0b5c5). |
return std::make_shared<asio_connection_pool>(crossplat::threadpool::shared_instance().service(), | ||
base_uri().scheme() == "https" && !_http_client_communicator::client_config().proxy().is_specified(), | ||
std::chrono::seconds(30), // Unused sockets are kept in pool for 30 seconds. | ||
this->client_config().get_ssl_context_callback()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] This branch of the if
already has guaranteed there is no ssl_context_callback, so passing in nullptr
here is probably more clear.
@@ -87,7 +87,10 @@ class connection | |||
if (is_https) | |||
{ | |||
m_ssl_context = utility::details::make_unique<boost::asio::ssl::context>(boost::asio::ssl::context::sslv23); | |||
ssl_context_callback(*m_ssl_context); | |||
if (ssl_context_callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible that the ssl_context_callback
does not have a valid target.
if (!ec) | ||
{ | ||
std::lock_guard<std::mutex> lg(m_connection_pool_map_mutex); | ||
auto &pool = m_connection_pool_map[pool_key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allocate a new pool shared_ptr
if there is not a map entry. It would probably be better to use .find(pool_key)
to avoid allocating a new map entry just to erase it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Instead of having a "pool of pools" and a timer for each |
This change is for the http_client implementation on Linux.
The change can make the following code running efficiently on Linux.
Originally, the following code will create a connection, close it, create another connection, and so on ...
Now the connections can be reused if the authority is same.
On Windows, connection is always reused (based on host) and managed by WinHTTP. This change makes it easier for cross-platform projects to use the same piece of code.