Revert "Revert "Connection and websockets fixes""
This reverts commit a8086647b103f55116ce4c872e1455ebf1f3e346.
Reason for revert: Restoring commit c00500b as base for upload image issue fix
Change-Id: I1dd5d3fda2d1ee6f4027193a0506d5ca764b01e4
Signed-off-by: Jan Sowinski <jan.sowinski@intel.com>
diff --git a/http/http_connection.h b/http/http_connection.h
index 7d24fe7..4ef3bc6 100644
--- a/http/http_connection.h
+++ b/http/http_connection.h
@@ -248,7 +248,8 @@
1024 * 1024 * BMCWEB_HTTP_REQ_BODY_LIMIT_MB;
template <typename Adaptor, typename Handler, typename... Middlewares>
-class Connection
+class Connection : public std::enable_shared_from_this<
+ Connection<Adaptor, Handler, Middlewares...>>
{
public:
Connection(boost::asio::io_context& ioService, Handler* handlerIn,
@@ -474,16 +475,15 @@
boost::beast::ssl_stream<
boost::asio::ip::tcp::socket>>)
{
- adaptor.async_handshake(
- boost::asio::ssl::stream_base::server,
- [this](const boost::system::error_code& ec) {
- if (ec)
- {
- checkDestroy();
- return;
- }
- doReadHeaders();
- });
+ adaptor.async_handshake(boost::asio::ssl::stream_base::server,
+ [this, self(shared_from_this())](
+ const boost::system::error_code& ec) {
+ if (ec)
+ {
+ return;
+ }
+ doReadHeaders();
+ });
}
else
{
@@ -561,18 +561,21 @@
if (!res.completed)
{
+ needToCallAfterHandlers = true;
+ res.completeRequestHandler = [self(shared_from_this())] {
+ self->completeRequest();
+ };
if (req->isUpgrade() &&
boost::iequals(
req->getHeaderValue(boost::beast::http::field::upgrade),
"websocket"))
{
handler->handleUpgrade(*req, res, std::move(adaptor));
+ // delete lambda with self shared_ptr
+ // to enable connection destruction
+ res.completeRequestHandler = nullptr;
return;
}
- res.completeRequestHandler = [this] {
- this->completeRequest();
- };
- needToCallAfterHandlers = true;
handler->handle(*req, res);
}
else
@@ -638,15 +641,16 @@
*middlewares, ctx, *req, res);
}
- // auto self = this->shared_from_this();
- res.completeRequestHandler = res.completeRequestHandler = [] {};
-
if (!isAlive())
{
// BMCWEB_LOG_DEBUG << this << " delete (socket is closed) " <<
// isReading
// << ' ' << isWriting;
// delete this;
+
+ // delete lambda with self shared_ptr
+ // to enable connection destruction
+ res.completeRequestHandler = nullptr;
return;
}
if (res.body().empty() && !res.jsonValue.empty())
@@ -683,21 +687,23 @@
res.keepAlive(req->keepAlive());
doWrite();
+
+ // delete lambda with self shared_ptr
+ // to enable connection destruction
+ res.completeRequestHandler = nullptr;
}
private:
void doReadHeaders()
{
- // auto self = this->shared_from_this();
- isReading = true;
BMCWEB_LOG_DEBUG << this << " doReadHeaders";
// Clean up any previous Connection.
boost::beast::http::async_read_header(
adaptor, buffer, *parser,
- [this](const boost::system::error_code& ec,
- std::size_t bytes_transferred) {
- isReading = false;
+ [this,
+ self(shared_from_this())](const boost::system::error_code& ec,
+ std::size_t bytes_transferred) {
BMCWEB_LOG_ERROR << this << " async_read_header "
<< bytes_transferred << " Bytes";
bool errorWhileReading = false;
@@ -722,7 +728,6 @@
cancelDeadlineTimer();
close();
BMCWEB_LOG_DEBUG << this << " from read(1)";
- checkDestroy();
return;
}
@@ -740,17 +745,15 @@
void doRead()
{
- // auto self = this->shared_from_this();
- isReading = true;
BMCWEB_LOG_DEBUG << this << " doRead";
boost::beast::http::async_read(
adaptor, buffer, *parser,
- [this](const boost::system::error_code& ec,
- std::size_t bytes_transferred) {
+ [this,
+ self(shared_from_this())](const boost::system::error_code& ec,
+ std::size_t bytes_transferred) {
BMCWEB_LOG_DEBUG << this << " async_read " << bytes_transferred
<< " Bytes";
- isReading = false;
bool errorWhileReading = false;
if (ec)
@@ -771,7 +774,6 @@
cancelDeadlineTimer();
close();
BMCWEB_LOG_DEBUG << this << " from read(1)";
- checkDestroy();
return;
}
handle();
@@ -780,30 +782,26 @@
void doWrite()
{
- // auto self = this->shared_from_this();
- isWriting = true;
BMCWEB_LOG_DEBUG << this << " doWrite";
res.preparePayload();
serializer.emplace(*res.stringResponse);
boost::beast::http::async_write(
adaptor, *serializer,
- [&](const boost::system::error_code& ec,
- std::size_t bytes_transferred) {
- isWriting = false;
+ [this,
+ self(shared_from_this())](const boost::system::error_code& ec,
+ std::size_t bytes_transferred) {
BMCWEB_LOG_DEBUG << this << " async_write " << bytes_transferred
<< " bytes";
if (ec)
{
BMCWEB_LOG_DEBUG << this << " from write(2)";
- checkDestroy();
return;
}
if (!res.keepAlive())
{
close();
BMCWEB_LOG_DEBUG << this << " from write(1)";
- checkDestroy();
return;
}
@@ -820,29 +818,25 @@
});
}
- void checkDestroy()
- {
- BMCWEB_LOG_DEBUG << this << " isReading " << isReading << " isWriting "
- << isWriting;
- if (!isReading && !isWriting)
- {
- BMCWEB_LOG_DEBUG << this << " delete (idle) ";
- delete this;
- }
- }
-
void cancelDeadlineTimer()
{
- BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue << ' '
- << timerCancelKey;
- timerQueue.cancel(timerCancelKey);
+ if (timerCancelKey)
+ {
+ BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue
+ << ' ' << *timerCancelKey;
+ timerQueue.cancel(*timerCancelKey);
+ timerCancelKey.reset();
+ }
}
void startDeadline()
{
cancelDeadlineTimer();
- timerCancelKey = timerQueue.add([this] {
+ timerCancelKey = timerQueue.add([this, self(shared_from_this())] {
+ // Mark timer as not active to avoid canceling it during
+ // Connection destructor which leads to double free issue
+ timerCancelKey.reset();
if (!isAlive())
{
return;
@@ -850,7 +844,7 @@
close();
});
BMCWEB_LOG_DEBUG << this << " timer added: " << &timerQueue << ' '
- << timerCancelKey;
+ << *timerCancelKey;
}
private:
@@ -877,10 +871,8 @@
const std::string& serverName;
- size_t timerCancelKey = 0;
+ std::optional<size_t> timerCancelKey;
- bool isReading{};
- bool isWriting{};
bool needToCallAfterHandlers{};
bool needToStartReadAfterComplete{};
@@ -889,5 +881,8 @@
std::function<std::string()>& getCachedDateStr;
detail::TimerQueue& timerQueue;
+
+ using std::enable_shared_from_this<
+ Connection<Adaptor, Handler, Middlewares...>>::shared_from_this;
};
} // namespace crow
diff --git a/http/http_server.h b/http/http_server.h
index df42214..6e63cbd 100644
--- a/http/http_server.h
+++ b/http/http_server.h
@@ -44,7 +44,8 @@
ioService(std::move(io)),
acceptor(std::move(acceptor)),
signals(*ioService, SIGINT, SIGTERM, SIGHUP), tickTimer(*ioService),
- handler(handler), middlewares(middlewares), adaptorCtx(adaptor_ctx)
+ timer(*ioService), handler(handler), middlewares(middlewares),
+ adaptorCtx(adaptor_ctx)
{
}
@@ -123,11 +124,9 @@
return this->dateStr;
};
- boost::asio::steady_timer timer(*ioService);
timer.expires_after(std::chrono::seconds(1));
- std::function<void(const boost::system::error_code& ec)> timerHandler;
- timerHandler = [&](const boost::system::error_code& ec) {
+ timerHandler = [this](const boost::system::error_code& ec) {
if (ec)
{
return;
@@ -231,8 +230,8 @@
boost::asio::ip::tcp::socket>>::value)
{
adaptorTemp = Adaptor(*ioService, *adaptorCtx);
- Connection<Adaptor, Handler, Middlewares...>* p =
- new Connection<Adaptor, Handler, Middlewares...>(
+ auto p =
+ std::make_shared<Connection<Adaptor, Handler, Middlewares...>>(
*ioService, handler, serverName, middlewares,
getCachedDateStr, timerQueue,
std::move(adaptorTemp.value()));
@@ -245,18 +244,14 @@
*this->ioService,
[p] { p->start(); });
}
- else
- {
- delete p;
- }
doAccept();
});
}
else
{
adaptorTemp = Adaptor(*ioService);
- Connection<Adaptor, Handler, Middlewares...>* p =
- new Connection<Adaptor, Handler, Middlewares...>(
+ auto p =
+ std::make_shared<Connection<Adaptor, Handler, Middlewares...>>(
*ioService, handler, serverName, middlewares,
getCachedDateStr, timerQueue,
std::move(adaptorTemp.value()));
@@ -268,10 +263,6 @@
boost::asio::post(*this->ioService,
[p] { p->start(); });
}
- else
- {
- delete p;
- }
doAccept();
});
}
@@ -284,6 +275,7 @@
std::unique_ptr<tcp::acceptor> acceptor;
boost::asio::signal_set signals;
boost::asio::steady_timer tickTimer;
+ boost::asio::steady_timer timer;
std::string dateStr;
@@ -292,6 +284,7 @@
std::chrono::milliseconds tickInterval{};
std::function<void()> tickFunction;
+ std::function<void(const boost::system::error_code& ec)> timerHandler;
std::tuple<Middlewares...>* middlewares;
diff --git a/http/routing.h b/http/routing.h
index f194ad1..c2a7503 100644
--- a/http/routing.h
+++ b/http/routing.h
@@ -324,19 +324,19 @@
res.end();
}
- void handleUpgrade(const Request& req, Response& res,
+ void handleUpgrade(const Request& req, Response&,
boost::asio::ip::tcp::socket&& adaptor) override
{
std::shared_ptr<
crow::websocket::ConnectionImpl<boost::asio::ip::tcp::socket>>
myConnection = std::make_shared<
crow::websocket::ConnectionImpl<boost::asio::ip::tcp::socket>>(
- req, res, std::move(adaptor), openHandler, messageHandler,
+ req, std::move(adaptor), openHandler, messageHandler,
closeHandler, errorHandler);
myConnection->start();
}
#ifdef BMCWEB_ENABLE_SSL
- void handleUpgrade(const Request& req, Response& res,
+ void handleUpgrade(const Request& req, Response&,
boost::beast::ssl_stream<boost::asio::ip::tcp::socket>&&
adaptor) override
{
@@ -344,7 +344,7 @@
boost::beast::ssl_stream<boost::asio::ip::tcp::socket>>>
myConnection = std::make_shared<crow::websocket::ConnectionImpl<
boost::beast::ssl_stream<boost::asio::ip::tcp::socket>>>(
- req, res, std::move(adaptor), openHandler, messageHandler,
+ req, std::move(adaptor), openHandler, messageHandler,
closeHandler, errorHandler);
myConnection->start();
}
diff --git a/http/websocket.h b/http/websocket.h
index 80d536a..f7c818e 100644
--- a/http/websocket.h
+++ b/http/websocket.h
@@ -20,8 +20,8 @@
struct Connection : std::enable_shared_from_this<Connection>
{
public:
- explicit Connection(const crow::Request& reqIn, crow::Response& res) :
- req(reqIn), userdataPtr(nullptr){};
+ explicit Connection(const crow::Request& reqIn) :
+ req(reqIn.req), userdataPtr(nullptr){};
virtual void sendBinary(const std::string_view msg) = 0;
virtual void sendBinary(std::string&& msg) = 0;
@@ -40,7 +40,7 @@
return userdataPtr;
}
- crow::Request req;
+ boost::beast::http::request<boost::beast::http::string_body> req;
crow::Response res;
private:
@@ -51,14 +51,14 @@
{
public:
ConnectionImpl(
- const crow::Request& reqIn, crow::Response& res, Adaptor adaptorIn,
+ const crow::Request& reqIn, Adaptor adaptorIn,
std::function<void(Connection&, std::shared_ptr<bmcweb::AsyncResp>)>
open_handler,
std::function<void(Connection&, const std::string&, bool)>
message_handler,
std::function<void(Connection&, const std::string&)> close_handler,
std::function<void(Connection&)> error_handler) :
- Connection(reqIn, res),
+ Connection(reqIn),
ws(std::move(adaptorIn)), inString(), inBuffer(inString, 131088),
openHandler(std::move(open_handler)),
messageHandler(std::move(message_handler)),
@@ -80,12 +80,11 @@
using bf = boost::beast::http::field;
- std::string_view protocol =
- req.getHeaderValue(bf::sec_websocket_protocol);
+ std::string_view protocol = req[bf::sec_websocket_protocol];
// Perform the websocket upgrade
ws.async_accept_ex(
- req.req,
+ req,
[protocol{std::string(protocol)}](
boost::beast::websocket::response_type& m) {
if (!protocol.empty())