Remove tcp_stream

tcp_stream has several problems in its current implementation.  First,
it takes up a significant amount of binary size, for features that we
don't use.  Next, it has some implied guarantees about timeouts that we
erronously rely on, namely that async_connect will timeout
appropriately given the tcp_stream timeout (it doesn't).

We already have a timer present in the ConnectionInfo class anyway, this
commit just makes use of it for ALL timeout operations, rather than just
the connect timeout operations.  This flow is roughly analogous to what
we do in http_connection.hpp already, so the pattern is well troden.

This saves 2.8% of the binary size of bmcweb, and solves several race
conditions.


Tested:
Relying on Carson: Aggregated a sub-bmc, and ensured that top level
collections returned correctly under GET /redfish/v1/Chassis

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I72e8e97b667826f272bb4921afc2b16907f3b271

1 files changed, 50 insertions, 22 deletions

diff --git a/http/http_client.hpp b/http/http_client.hpp
index d34fb2e582..6bb9159d0e 100644
--- a/http/http_client.hpp
+++ b/http/http_client.hpp
@@ -14,6 +14,7 @@
 // limitations under the License.
 */
 #pragma once
+#include <boost/asio/connect.hpp>
 #include <boost/asio/io_context.hpp>
 #include <boost/asio/ip/address.hpp>
 #include <boost/asio/ip/basic_endpoint.hpp>
@@ -23,7 +24,6 @@
 #include <boost/asio/steady_timer.hpp>
 #include <boost/beast/core/flat_buffer.hpp>
 #include <boost/beast/core/flat_static_buffer.hpp>
-#include <boost/beast/core/tcp_stream.hpp>
 #include <boost/beast/http/message.hpp>
 #include <boost/beast/http/parser.hpp>
 #include <boost/beast/http/read.hpp>
@@ -122,7 +122,6 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
   private:
     ConnState state = ConnState::initialized;
     uint32_t retryCount = 0;
-    bool runningTimer = false;
     std::string subId;
     std::string host;
     uint16_t port;
@@ -143,8 +142,9 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
     // Ascync callables
     std::function<void(bool, uint32_t, Response&)> callback;
     crow::async_resolve::Resolver resolver;
-    boost::beast::tcp_stream conn;
-    std::optional<boost::beast::ssl_stream<boost::beast::tcp_stream&>> sslConn;
+    boost::asio::ip::tcp::socket conn;
+    std::optional<boost::beast::ssl_stream<boost::asio::ip::tcp::socket&>>
+        sslConn;
 
     boost::asio::steady_timer timer;
 
@@ -187,17 +187,20 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
                          << std::to_string(port)
                          << ", id: " << std::to_string(connId);
 
-        conn.expires_after(std::chrono::seconds(30));
-        conn.async_connect(endpointList,
-                           std::bind_front(&ConnectionInfo::afterConnect, this,
-                                           shared_from_this()));
+        timer.expires_after(std::chrono::seconds(30));
+        timer.async_wait(std::bind_front(onTimeout, weak_from_this()));
+
+        boost::asio::async_connect(
+            conn, endpointList,
+            std::bind_front(&ConnectionInfo::afterConnect, this,
+                            shared_from_this()));
     }
 
     void afterConnect(const std::shared_ptr<ConnectionInfo>& /*self*/,
                       boost::beast::error_code ec,
                       const boost::asio::ip::tcp::endpoint& endpoint)
     {
-
+        timer.cancel();
         if (ec)
         {
             BMCWEB_LOG_ERROR << "Connect " << endpoint.address().to_string()
@@ -213,20 +216,22 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
                          << ", id: " << std::to_string(connId);
         if (sslConn)
         {
-            doSSLHandshake();
+            doSslHandshake();
             return;
         }
         state = ConnState::connected;
         sendMessage();
     }
 
-    void doSSLHandshake()
+    void doSslHandshake()
     {
         if (!sslConn)
         {
             return;
         }
         state = ConnState::handshakeInProgress;
+        timer.expires_after(std::chrono::seconds(30));
+        timer.async_wait(std::bind_front(onTimeout, weak_from_this()));
         sslConn->async_handshake(
             boost::asio::ssl::stream_base::client,
             std::bind_front(&ConnectionInfo::afterSslHandshake, this,
@@ -236,6 +241,7 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
     void afterSslHandshake(const std::shared_ptr<ConnectionInfo>& /*self*/,
                            boost::beast::error_code ec)
     {
+        timer.cancel();
         if (ec)
         {
             BMCWEB_LOG_ERROR << "SSL Handshake failed -"
@@ -256,7 +262,8 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
         state = ConnState::sendInProgress;
 
         // Set a timeout on the operation
-        conn.expires_after(std::chrono::seconds(30));
+        timer.expires_after(std::chrono::seconds(30));
+        timer.async_wait(std::bind_front(onTimeout, weak_from_this()));
 
         // Send the HTTP request to the remote host
         if (sslConn)
@@ -278,6 +285,7 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
     void afterWrite(const std::shared_ptr<ConnectionInfo>& /*self*/,
                     const boost::beast::error_code& ec, size_t bytesTransferred)
     {
+        timer.cancel();
         if (ec)
         {
             BMCWEB_LOG_ERROR << "sendMessage() failed: " << ec.message();
@@ -298,6 +306,9 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
         parser.emplace(std::piecewise_construct, std::make_tuple());
         parser->body_limit(httpReadBodyLimit);
 
+        timer.expires_after(std::chrono::seconds(30));
+        timer.async_wait(std::bind_front(onTimeout, weak_from_this()));
+
         // Receive the HTTP response
         if (sslConn)
         {
@@ -319,6 +330,7 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
                    const boost::beast::error_code& ec,
                    const std::size_t& bytesTransferred)
     {
+        timer.cancel();
         if (ec && ec != boost::asio::ssl::error::stream_truncated)
         {
             BMCWEB_LOG_ERROR << "recvMessage() failed: " << ec.message();
@@ -362,6 +374,30 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
         callback(parser->keep_alive(), connId, res);
     }
 
+    static void onTimeout(const std::weak_ptr<ConnectionInfo>& weakSelf,
+                          const boost::system::error_code ec)
+    {
+        if (ec == boost::asio::error::operation_aborted)
+        {
+            BMCWEB_LOG_DEBUG
+                << "async_wait failed since the operation is aborted"
+                << ec.message();
+            return;
+        }
+        if (ec)
+        {
+            BMCWEB_LOG_ERROR << "async_wait failed: " << ec.message();
+            // If the timer fails, we need to close the socket anyway, same as
+            // if it expired.
+        }
+        std::shared_ptr<ConnectionInfo> self = weakSelf.lock();
+        if (self == nullptr)
+        {
+            return;
+        }
+        self->waitAndRetry();
+    }
+
     void waitAndRetry()
     {
         if ((retryCount >= retryPolicy.maxRetryAttempts) ||
@@ -393,13 +429,6 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
             return;
         }
 
-        if (runningTimer)
-        {
-            BMCWEB_LOG_DEBUG << "Retry timer is already running.";
-            return;
-        }
-        runningTimer = true;
-
         retryCount++;
 
         BMCWEB_LOG_DEBUG << "Attempt retry after "
@@ -421,7 +450,6 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
                 // Ignore the error and continue the retry loop to attempt
                 // sending the event as per the retry policy
             }
-            self->runningTimer = false;
 
             // Let's close the connection and restart from resolve.
             self->doClose(true);
@@ -431,7 +459,7 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
     void shutdownConn(bool retry)
     {
         boost::beast::error_code ec;
-        conn.socket().shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
+        conn.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
         conn.close();
 
         // not_connected happens sometimes so don't bother reporting it.
@@ -454,7 +482,7 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
             {
                 // Now let's try to resend the data
                 state = ConnState::retry;
-                this->doResolve();
+                doResolve();
             }
             else
             {