Make timer system use boost
The original crow timeout system had a timer queue setup for handling
many thousands of connections at a time efficiently. The most common
use cases for the bmc involve a handful of connections, so this code
doesn't help us much.
These days, boost asio also implements a very similar timer queue
https://www.boost.org/doc/libs/1_72_0/boost/asio/detail/timer_queue.hpp
internally, so the only thing we're loosing here is the "fuzzy"
coalescing of timeout actions, for which it's tough to say if anyone
will even notice.
This commit implements a timer system that's self contained within each
connection, using steady_timer. This is much more "normal" and how most
of the beast examples implement timers.
Tested:
Minimal touch testing to ensure that things work, but more testing is
required, probably using sloworis to ensure that our timeouts are no
longer issues.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I19156411ce46adff6c88ad97ee8f6af8c858fe3c
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index ccc2f28..bfd6411 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -5,7 +5,6 @@
#include "http_response.hpp"
#include "http_utility.hpp"
#include "logging.hpp"
-#include "timer_queue.hpp"
#include "utility.hpp"
#include <boost/algorithm/string.hpp>
@@ -13,6 +12,7 @@
#include <boost/asio/io_context.hpp>
#include <boost/asio/ip/tcp.hpp>
#include <boost/asio/ssl/stream.hpp>
+#include <boost/asio/steady_timer.hpp>
#include <boost/beast/core/flat_static_buffer.hpp>
#include <boost/beast/ssl/ssl_stream.hpp>
#include <boost/beast/websocket.hpp>
@@ -47,25 +47,17 @@
constexpr uint32_t httpHeaderLimit = 8192;
-// drop all connections after 1 minute, this time limit was chosen
-// arbitrarily and can be adjusted later if needed
-static constexpr const size_t loggedInAttempts =
- (60 / timerQueueTimeoutSeconds);
-
-static constexpr const size_t loggedOutAttempts =
- (15 / timerQueueTimeoutSeconds);
-
template <typename Adaptor, typename Handler>
class Connection :
public std::enable_shared_from_this<Connection<Adaptor, Handler>>
{
public:
- Connection(Handler* handlerIn,
+ Connection(Handler* handlerIn, boost::asio::steady_timer&& timerIn,
std::function<std::string()>& getCachedDateStrF,
- detail::TimerQueue& timerQueueIn, Adaptor adaptorIn) :
+ Adaptor adaptorIn) :
adaptor(std::move(adaptorIn)),
- handler(handlerIn), getCachedDateStr(getCachedDateStrF),
- timerQueue(timerQueueIn)
+ handler(handlerIn), timer(std::move(timerIn)),
+ getCachedDateStr(getCachedDateStrF)
{
parser.emplace(std::piecewise_construct, std::make_tuple());
parser->body_limit(httpReqBodyLimit);
@@ -285,8 +277,7 @@
void start()
{
-
- startDeadline(0);
+ startDeadline();
// TODO(ed) Abstract this to a more clever class with the idea of an
// asynchronous "start"
@@ -312,7 +303,6 @@
void handle()
{
- cancelDeadlineTimer();
std::error_code reqEc;
crow::Request& thisReq = req.emplace(parser->release(), reqEc);
if (reqEc)
@@ -578,13 +568,9 @@
userSession = crow::authorization::authenticate(
ip, res, method, parser->get().base(), userSession);
#endif // BMCWEB_INSECURE_DISABLE_AUTHENTICATION
+
bool loggedIn = userSession != nullptr;
- if (loggedIn)
- {
- startDeadline(loggedInAttempts);
- BMCWEB_LOG_DEBUG << "Starting slow deadline";
- }
- else
+ if (!loggedIn)
{
const boost::optional<uint64_t> contentLength =
parser->content_length();
@@ -597,9 +583,9 @@
return;
}
- startDeadline(loggedOutAttempts);
BMCWEB_LOG_DEBUG << "Starting quick deadline";
}
+
doRead();
});
}
@@ -607,7 +593,7 @@
void doRead()
{
BMCWEB_LOG_DEBUG << this << " doRead";
-
+ startDeadline();
boost::beast::http::async_read(
adaptor, buffer, *parser,
[this,
@@ -615,36 +601,11 @@
std::size_t bytesTransferred) {
BMCWEB_LOG_DEBUG << this << " async_read " << bytesTransferred
<< " Bytes";
-
- bool errorWhileReading = false;
+ cancelDeadlineTimer();
if (ec)
{
BMCWEB_LOG_ERROR
<< this << " Error while reading: " << ec.message();
- errorWhileReading = true;
- }
- else
- {
- if (isAlive())
- {
- cancelDeadlineTimer();
- if (userSession != nullptr)
- {
- startDeadline(loggedInAttempts);
- }
- else
- {
- startDeadline(loggedOutAttempts);
- }
- }
- else
- {
- errorWhileReading = true;
- }
- }
- if (errorWhileReading)
- {
- cancelDeadlineTimer();
close();
BMCWEB_LOG_DEBUG << this << " from read(1)";
return;
@@ -655,18 +616,10 @@
void doWrite()
{
- bool loggedIn = req && req->session;
- if (loggedIn)
- {
- startDeadline(loggedInAttempts);
- }
- else
- {
- startDeadline(loggedOutAttempts);
- }
BMCWEB_LOG_DEBUG << this << " doWrite";
res.preparePayload();
serializer.emplace(*res.stringResponse);
+ startDeadline();
boost::beast::http::async_write(
adaptor, *serializer,
[this,
@@ -712,64 +665,51 @@
void cancelDeadlineTimer()
{
- if (timerCancelKey)
- {
- BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue
- << ' ' << *timerCancelKey;
- timerQueue.cancel(*timerCancelKey);
- timerCancelKey.reset();
- }
+ timer.cancel();
}
- void startDeadline(size_t timerIterations)
+ void startDeadline()
{
cancelDeadlineTimer();
- if (timerIterations)
+ std::chrono::seconds timeout(15);
+ // allow slow uploads for logged in users
+ bool loggedIn = req && req->session;
+ if (loggedIn)
{
- timerIterations--;
- }
-
- timerCancelKey =
- timerQueue.add([self(shared_from_this()), timerIterations,
- readCount{parser->get().body().size()}] {
- // Mark timer as not active to avoid canceling it during
- // Connection destructor which leads to double free issue
- self->timerCancelKey.reset();
- if (!self->isAlive())
- {
- return;
- }
-
- bool loggedIn = self->req && self->req->session;
- // allow slow uploads for logged in users
- if (loggedIn && self->parser->get().body().size() > readCount)
- {
- BMCWEB_LOG_DEBUG << self.get()
- << " restart timer - read in progress";
- self->startDeadline(timerIterations);
- return;
- }
-
- // Threshold can be used to drop slow connections
- // to protect against slow-rate DoS attack
- if (timerIterations)
- {
- BMCWEB_LOG_DEBUG << self.get() << " restart timer";
- self->startDeadline(timerIterations);
- return;
- }
-
- self->close();
- });
-
- if (!timerCancelKey)
- {
- close();
+ timeout = std::chrono::seconds(60);
return;
}
- BMCWEB_LOG_DEBUG << this << " timer added: " << &timerQueue << ' '
- << *timerCancelKey;
+
+ std::weak_ptr<Connection<Adaptor, Handler>> weakSelf = weak_from_this();
+ timer.expires_after(timeout);
+ timer.async_wait([weakSelf](const boost::system::error_code ec) {
+ // Note, we are ignoring other types of errors here; If the timer
+ // failed for any reason, we should still close the connection
+
+ std::shared_ptr<Connection<Adaptor, Handler>> self =
+ weakSelf.lock();
+ if (!self)
+ {
+ BMCWEB_LOG_CRITICAL << self << " Failed to capture connection";
+ return;
+ }
+ if (ec == boost::asio::error::operation_aborted)
+ {
+ // Canceled wait means the path succeeeded.
+ return;
+ }
+ if (ec)
+ {
+ BMCWEB_LOG_CRITICAL << self << " timer failed " << ec;
+ }
+
+ BMCWEB_LOG_WARNING << self << "Connection timed out, closing";
+
+ self->close();
+ });
+
+ BMCWEB_LOG_DEBUG << this << " timer started";
}
private:
@@ -793,12 +733,14 @@
bool sessionIsFromTransport = false;
std::shared_ptr<persistent_data::UserSession> userSession;
- std::optional<size_t> timerCancelKey;
+ boost::asio::steady_timer timer;
std::function<std::string()>& getCachedDateStr;
- detail::TimerQueue& timerQueue;
using std::enable_shared_from_this<
Connection<Adaptor, Handler>>::shared_from_this;
+
+ using std::enable_shared_from_this<
+ Connection<Adaptor, Handler>>::weak_from_this;
};
} // namespace crow