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
diff --git a/http/http_client.hpp b/http/http_client.hpp
index d34fb2e..6bb9159 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 @@
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 @@
// 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 @@
<< 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 @@
<< ", 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
{
// Now let's try to resend the data
state = ConnState::retry;
- this->doResolve();
+ doResolve();
}
else
{