Fix handling of timed out connections kept alive in connection pool under Unix (#762)

* 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.

* 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.

* 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.

* Reduce duplicate code between ssl_proxy and asio_context in handle_read_status_line.
Avoid increasing public surface with rewind function.

Author: vadz

Release/src/http/client/http_client_asio.cpp

@@ -28,6 +28,7 @@
 #endif
 #include <boost/asio.hpp>
 #include <boost/asio/ssl.hpp>
+#include <boost/asio/ssl/error.hpp>
 #include <boost/asio/steady_timer.hpp>
 #include <boost/algorithm/string.hpp>
 #include <boost/bind.hpp>
@@ -170,6 +171,45 @@ 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_reused_and_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;
+        }
+
+        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;
+    }
+
     template <typename Iterator, typename Handler>
     void async_connect(const Iterator &begin, const Handler &handler)
     {
@@ -578,37 +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
             {
-                // 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())
-                {
-                    // 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_completion = m_context->m_request_completion;
-                    new_ctx->m_cancellationRegistration = m_context->m_cancellationRegistration;
-
-                    auto client = std::static_pointer_cast<asio_client>(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");
             }
         }
 
@@ -619,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,
@@ -821,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;
@@ -1173,31 +1193,33 @@ 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())
-            {
-                // 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_completion = m_request_completion;
-                new_ctx->m_cancellationRegistration = m_cancellationRegistration;
-
-                auto client = std::static_pointer_cast<asio_client>(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<asio_client>(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);
         }
     }