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_client.hpp b/http/http_client.hpp
index 7fd2041..6aad0da 100644
--- a/http/http_client.hpp
+++ b/http/http_client.hpp
@@ -21,6 +21,7 @@
#include <boost/beast/core/tcp_stream.hpp>
#include <boost/beast/http/message.hpp>
#include <boost/beast/version.hpp>
+#include <boost/circular_buffer.hpp>
#include <include/async_resolve.hpp>
#include <cstdlib>
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
diff --git a/http/http_server.hpp b/http/http_server.hpp
index 20b4e50..4037789 100644
--- a/http/http_server.hpp
+++ b/http/http_server.hpp
@@ -2,7 +2,6 @@
#include "http_connection.hpp"
#include "logging.hpp"
-#include "timer_queue.hpp"
#include <boost/asio/ip/address.hpp>
#include <boost/asio/ip/tcp.hpp>
@@ -36,8 +35,8 @@
std::make_shared<boost::asio::io_context>()) :
ioService(std::move(io)),
acceptor(std::move(acceptorIn)),
- signals(*ioService, SIGINT, SIGTERM, SIGHUP), timer(*ioService),
- handler(handlerIn), adaptorCtx(std::move(adaptorCtx))
+ signals(*ioService, SIGINT, SIGTERM, SIGHUP), handler(handlerIn),
+ adaptorCtx(std::move(adaptorCtx))
{}
Server(Handler* handlerIn, const std::string& bindaddr, uint16_t port,
@@ -91,19 +90,6 @@
return this->dateStr;
};
- timer.expires_after(std::chrono::seconds(1));
-
- timerHandler = [this](const boost::system::error_code& ec) {
- if (ec)
- {
- return;
- }
- timerQueue.process();
- timer.expires_after(std::chrono::seconds(1));
- timer.async_wait(timerHandler);
- };
- timer.async_wait(timerHandler);
-
BMCWEB_LOG_INFO << "bmcweb server is running, local endpoint "
<< acceptor->local_endpoint();
startAsyncWaitForSignal();
@@ -179,13 +165,14 @@
void doAccept()
{
std::optional<Adaptor> adaptorTemp;
+ boost::asio::steady_timer timer(*ioService);
if constexpr (std::is_same<Adaptor,
boost::beast::ssl_stream<
boost::asio::ip::tcp::socket>>::value)
{
adaptorTemp = Adaptor(*ioService, *adaptorCtx);
auto p = std::make_shared<Connection<Adaptor, Handler>>(
- handler, getCachedDateStr, timerQueue,
+ handler, std::move(timer), getCachedDateStr,
std::move(adaptorTemp.value()));
acceptor->async_accept(p->socket().next_layer(),
@@ -203,7 +190,7 @@
{
adaptorTemp = Adaptor(*ioService);
auto p = std::make_shared<Connection<Adaptor, Handler>>(
- handler, getCachedDateStr, timerQueue,
+ handler, std::move(timer), getCachedDateStr,
std::move(adaptorTemp.value()));
acceptor->async_accept(
@@ -220,21 +207,17 @@
private:
std::shared_ptr<boost::asio::io_context> ioService;
- detail::TimerQueue timerQueue;
std::function<std::string()> getCachedDateStr;
std::unique_ptr<boost::asio::ip::tcp::acceptor> acceptor;
boost::asio::signal_set signals;
- boost::asio::steady_timer timer;
std::string dateStr;
Handler* handler;
- std::function<void(const boost::system::error_code& ec)> timerHandler;
-
#ifdef BMCWEB_ENABLE_SSL
bool useSsl{false};
#endif
std::shared_ptr<boost::asio::ssl::context> adaptorCtx;
-}; // namespace crow
+};
} // namespace crow
diff --git a/http/timer_queue.hpp b/http/timer_queue.hpp
deleted file mode 100644
index 24a4ab4..0000000
--- a/http/timer_queue.hpp
+++ /dev/null
@@ -1,97 +0,0 @@
-#pragma once
-
-#include "logging.hpp"
-
-#include <boost/circular_buffer.hpp>
-#include <boost/circular_buffer/space_optimized.hpp>
-
-#include <chrono>
-#include <functional>
-
-namespace crow
-{
-
-constexpr const size_t timerQueueTimeoutSeconds = 5;
-namespace detail
-{
-
-constexpr const size_t maxSize = 100;
-// fast timer queue for fixed tick value.
-class TimerQueue
-{
- public:
- TimerQueue()
- {
- dq.set_capacity(maxSize);
- }
-
- void cancel(size_t k)
- {
- size_t index = k - step;
- if (index < dq.size())
- {
- dq[index].second = nullptr;
- }
- while (dq.begin() != dq.end() && dq.front().second == nullptr)
- {
- dq.pop_front();
- step++;
- }
- }
-
- std::optional<size_t> add(std::function<void()> f)
- {
- if (dq.size() == maxSize)
- {
- return std::nullopt;
- }
-
- dq.push_back(
- std::make_pair(std::chrono::steady_clock::now(), std::move(f)));
- size_t ret = step + dq.size() - 1;
-
- BMCWEB_LOG_DEBUG << "timer add inside: " << this << ' ' << ret;
- return ret;
- }
-
- void process()
- {
- auto now = std::chrono::steady_clock::now();
- while (!dq.empty())
- {
- auto& x = dq.front();
- // Check expiration time only for active handlers,
- // remove canceled ones immediately
- if (x.second)
- {
- if (now - x.first <
- std::chrono::seconds(timerQueueTimeoutSeconds))
- {
- break;
- }
-
- BMCWEB_LOG_DEBUG << "timer call: " << this << ' ' << step;
- // we know that timer handlers are very simple currently; call
- // here
- x.second();
- }
- dq.pop_front();
- step++;
- }
- }
-
- private:
- using storage_type =
- std::pair<std::chrono::time_point<std::chrono::steady_clock>,
- std::function<void()>>;
-
- boost::circular_buffer_space_optimized<storage_type,
- std::allocator<storage_type>>
- dq{};
-
- // boost::circular_buffer<storage_type> dq{20};
- // std::deque<storage_type> dq{};
- size_t step{};
-};
-} // namespace detail
-} // namespace crow