From d87fa2b3a54a8cd681932c6efb85f6e55611a469 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 8 May 2018 17:05:18 +0200 Subject: [PATCH 1/4] Add asio_connection::was_closed_by_server() to reduce duplication Reduce code duplication between ssl_proxy_tunnel::handle_status_line() and the method with the same name in asio_context itself by moving the common handling of connections closed by server into a new function. No real changes, this is a pure refactoring. --- Release/src/http/client/http_client_asio.cpp | 36 ++++++++++++++------ 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index 4ba3e0851b..25ce9d8909 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -170,6 +170,30 @@ class asio_connection bool keep_alive() const { return m_keep_alive; } bool is_ssl() const { return m_ssl_stream ? true : false; } + // Check if the error code indicates that the connection was closed by the + // server: this is used to detect if a connection in the pool was closed during + // its period of inactivity and we should reopen it. + bool was_closed_by_server(const boost::system::error_code& ec) const + { + if (!is_reused()) + { + // Don't bother reopening the connection if it's a new one: in this + // case, even if the connection was really lost, it's still a real + // error and we shouldn't try to reopen it. + return false; + } + + // These errors tell if connection was closed. + if ((boost::asio::error::eof == ec) + || (boost::asio::error::connection_reset == ec) + || (boost::asio::error::connection_aborted == ec)) + { + return true; + } + + return false; + } + template void async_connect(const Iterator &begin, const Handler &handler) { @@ -586,11 +610,7 @@ class asio_context : public request_context, public std::enable_shared_from_this } else { - // These errors tell if connection was closed. - const bool socket_was_closed((boost::asio::error::eof == ec) - || (boost::asio::error::connection_reset == ec) - || (boost::asio::error::connection_aborted == ec)); - if (socket_was_closed && m_context->m_connection->is_reused()) + if (m_context->m_connection->was_closed_by_server(ec)) { // Failed to write to socket because connection was already closed while it was in the pool. // close() here ensures socket is closed in a robust way and prevents the connection from being put to the pool again. @@ -1179,11 +1199,7 @@ class asio_context : public request_context, public std::enable_shared_from_this } else { - // These errors tell if connection was closed. - const bool socket_was_closed((boost::asio::error::eof == ec) - || (boost::asio::error::connection_reset == ec) - || (boost::asio::error::connection_aborted == ec)); - if (socket_was_closed && m_connection->is_reused()) + if (m_connection->was_closed_by_server(ec)) { // Failed to write to socket because connection was already closed while it was in the pool. // close() here ensures socket is closed in a robust way and prevents the connection from being put to the pool again. From fb08ce5c1e330191ab47bd20264de907283574d7 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 8 May 2018 17:07:27 +0200 Subject: [PATCH 2/4] Fix checking for server-side closure of HTTPS connections When an SSL connection times out due to being closed by server, a different error code is returned, so we need to check for it too in was_closed_by_server(). Without this, losing connection was not detected at all when using HTTPS, resulting in "Failed to read HTTP status line" errors whenever the same http_client was reused after more than the server keep alive timeout of inactivity. See #592. --- Release/src/http/client/http_client_asio.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index 25ce9d8909..d7bd0698d8 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -28,6 +28,7 @@ #endif #include #include +#include #include #include #include @@ -191,6 +192,21 @@ class asio_connection return true; } + if (is_ssl()) + { + // For SSL connections, we can also get a different error due to + // incorrect secure connection shutdown if it was closed by the + // server due to inactivity. Unfortunately, the exact error we get + // in this case depends on the Boost.Asio version used. +#if BOOST_ASIO_VERSION >= 101008 + if (boost::asio::ssl::error::stream_truncated == ec) + return true; +#else // Asio < 1.10.8 didn't have ssl::error::stream_truncated + if (boost::system::error_code(ERR_PACK(ERR_LIB_SSL, 0, SSL_R_SHORT_READ), boost::asio::error::get_ssl_category()) == ec) + return true; +#endif + } + return false; } From f4f234840ea6b8bbae63dd0bf803019d0530a1db Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 8 May 2018 17:52:07 +0200 Subject: [PATCH 3/4] Fix bug with using re-opened connections with ASIO Creating a new request when the existing connection being used was closed by the server didn't work correctly because the associated input stream was already exhausted, as its contents had been already "sent" to the server using the now discarded connection, so starting a new request using the same body stream never worked. Fix this by explicitly rewinding the stream to the beginning before using it again. Note that even with this fix using a custom body stream which is not positioned at the beginning initially (or which doesn't support rewinding) still wouldn't work, but at least it fixes the most common use case. See #592. --- Release/include/cpprest/http_msg.h | 10 ++++++++++ Release/src/http/client/http_client_asio.cpp | 2 ++ 2 files changed, 12 insertions(+) diff --git a/Release/include/cpprest/http_msg.h b/Release/include/cpprest/http_msg.h index b85e98ff42..72ae473112 100644 --- a/Release/include/cpprest/http_msg.h +++ b/Release/include/cpprest/http_msg.h @@ -328,6 +328,11 @@ class http_msg_base /// const concurrency::streams::istream & instream() const { return m_inStream; } + /// + /// Rewind the message body stream to the beginning allowing to reuse it. + /// + void rewind_instream() { m_inStream.seek(0); } + /// /// Set the stream through which the message body could be written /// @@ -1363,6 +1368,11 @@ class http_request _m_impl->_set_base_uri(base_uri); } + void _rewind_instream() + { + _m_impl->rewind_instream(); + } + private: friend class http::details::_http_request; friend class http::client::http_client; diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index d7bd0698d8..2591dbb0bf 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -636,6 +636,7 @@ class asio_context : public request_context, public std::enable_shared_from_this // cancellation registration to maintain the old state. // This also obtains a new connection from pool. auto new_ctx = m_context->create_request_context(m_context->m_http_client, m_context->m_request); + new_ctx->m_request._rewind_instream(); new_ctx->m_request_completion = m_context->m_request_completion; new_ctx->m_cancellationRegistration = m_context->m_cancellationRegistration; @@ -1225,6 +1226,7 @@ class asio_context : public request_context, public std::enable_shared_from_this // cancellation registration to maintain the old state. // This also obtains a new connection from pool. auto new_ctx = create_request_context(m_http_client, m_request); + new_ctx->m_request._rewind_instream(); new_ctx->m_request_completion = m_request_completion; new_ctx->m_cancellationRegistration = m_cancellationRegistration; From d5b1b7a9297d69ea9144b80f8697acd83b2429f6 Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Thu, 2 Aug 2018 23:15:56 -0700 Subject: [PATCH 4/4] Reduce duplicate code between ssl_proxy and asio_context in handle_read_status_line. Avoid increasing public surface with rewind function. --- Release/include/cpprest/http_msg.h | 10 --- Release/src/http/client/http_client_asio.cpp | 82 +++++++++----------- 2 files changed, 35 insertions(+), 57 deletions(-) diff --git a/Release/include/cpprest/http_msg.h b/Release/include/cpprest/http_msg.h index 72ae473112..b85e98ff42 100644 --- a/Release/include/cpprest/http_msg.h +++ b/Release/include/cpprest/http_msg.h @@ -328,11 +328,6 @@ class http_msg_base /// const concurrency::streams::istream & instream() const { return m_inStream; } - /// - /// Rewind the message body stream to the beginning allowing to reuse it. - /// - void rewind_instream() { m_inStream.seek(0); } - /// /// Set the stream through which the message body could be written /// @@ -1368,11 +1363,6 @@ class http_request _m_impl->_set_base_uri(base_uri); } - void _rewind_instream() - { - _m_impl->rewind_instream(); - } - private: friend class http::details::_http_request; friend class http::client::http_client; diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index e6c019b617..3bf1fcba23 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -174,7 +174,7 @@ class asio_connection // Check if the error code indicates that the connection was closed by the // server: this is used to detect if a connection in the pool was closed during // its period of inactivity and we should reopen it. - bool was_closed_by_server(const boost::system::error_code& ec) const + bool was_reused_and_closed_by_server(const boost::system::error_code& ec) const { if (!is_reused()) { @@ -618,34 +618,13 @@ class asio_context : public request_context, public std::enable_shared_from_this return; } - m_context->m_connection->upgrade_to_ssl(m_context->m_http_client->client_config().get_ssl_context_callback()); + m_context->upgrade_to_ssl(); m_ssl_tunnel_established(m_context); } else { - if (m_context->m_connection->was_closed_by_server(ec)) - { - // Failed to write to socket because connection was already closed while it was in the pool. - // close() here ensures socket is closed in a robust way and prevents the connection from being put to the pool again. - m_context->m_connection->close(); - - // Create a new context and copy the request object, completion event and - // cancellation registration to maintain the old state. - // This also obtains a new connection from pool. - auto new_ctx = m_context->create_request_context(m_context->m_http_client, m_context->m_request); - new_ctx->m_request._rewind_instream(); - new_ctx->m_request_completion = m_context->m_request_completion; - new_ctx->m_cancellationRegistration = m_context->m_cancellationRegistration; - - auto client = std::static_pointer_cast(m_context->m_http_client); - // Resend the request using the new context. - client->send_request(new_ctx); - } - else - { - m_context->report_error("Failed to read HTTP status line from proxy", ec, httpclient_errorcode_context::readheader); - } + m_context->handle_failed_read_status_line(ec, "Failed to read HTTP status line from proxy"); } } @@ -656,7 +635,6 @@ class asio_context : public request_context, public std::enable_shared_from_this boost::asio::streambuf m_response; }; - enum class http_proxy_type { none, @@ -858,6 +836,11 @@ class asio_context : public request_context, public std::enable_shared_from_this } private: + void upgrade_to_ssl() + { + m_connection->upgrade_to_ssl(m_http_client->client_config().get_ssl_context_callback()); + } + std::string generate_basic_auth_header() { std::string header; @@ -1210,28 +1193,33 @@ class asio_context : public request_context, public std::enable_shared_from_this } else { - if (m_connection->was_closed_by_server(ec)) - { - // Failed to write to socket because connection was already closed while it was in the pool. - // close() here ensures socket is closed in a robust way and prevents the connection from being put to the pool again. - m_connection->close(); - - // Create a new context and copy the request object, completion event and - // cancellation registration to maintain the old state. - // This also obtains a new connection from pool. - auto new_ctx = create_request_context(m_http_client, m_request); - new_ctx->m_request._rewind_instream(); - new_ctx->m_request_completion = m_request_completion; - new_ctx->m_cancellationRegistration = m_cancellationRegistration; - - auto client = std::static_pointer_cast(m_http_client); - // Resend the request using the new context. - client->send_request(new_ctx); - } - else - { - report_error("Failed to read HTTP status line", ec, httpclient_errorcode_context::readheader); - } + handle_failed_read_status_line(ec, "Failed to read HTTP status line"); + } + } + + void handle_failed_read_status_line(const boost::system::error_code& ec, const char* generic_error_message) + { + if (m_connection->was_reused_and_closed_by_server(ec)) + { + // Failed to write to socket because connection was already closed while it was in the pool. + // close() here ensures socket is closed in a robust way and prevents the connection from being put to the pool again. + m_connection->close(); + + // Create a new context and copy the request object, completion event and + // cancellation registration to maintain the old state. + // This also obtains a new connection from pool. + auto new_ctx = create_request_context(m_http_client, m_request); + new_ctx->m_request._get_impl()->instream().seek(0); + new_ctx->m_request_completion = m_request_completion; + new_ctx->m_cancellationRegistration = m_cancellationRegistration; + + auto client = std::static_pointer_cast(m_http_client); + // Resend the request using the new context. + client->send_request(new_ctx); + } + else + { + report_error(generic_error_message, ec, httpclient_errorcode_context::readheader); } }