Change the completionhandler to accept Res
These modifications are from WIP:Redfish:Query parameters:Only
(https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/47474). It will
be used in future CLs for Query Parameters.
The code changed the completion handle to accept Res to be able to
recall handle with a new Response object.
AsyncResp owns a new res, so there is no need to pass in a res.
Also fixed a self-move assignment bug.
Context:
Originally submitted:
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/480020
Reveted here:
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/48880
Because of failures here:
https://gerrit.openbmc-project.xyz/c/openbmc/openbmc/+/48864
Tested:
1. Romulus QEMU + Robot tests; all passed
2. Use scripts/websocket_test.py to test websockets. It is still work correctly.
3. Tested in real hardware; no new validator errors; tested both
authless, session, and basic auth.
4. Hacked codes to return 500 errors on certain resource; response is
expected;
5. Tested Eventing, the push style one (not SSE which is still under
review), worked as expected.
6. Tested 404 errors; response is expected.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Signed-off-by: John Edward Broadbent <jebr@google.com>
Change-Id: I52adb174476e0f6656335baa6657456752a031be
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index 61d3f7b..6a874a7 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -334,7 +334,7 @@
if (thisReq.getHeaderValue(boost::beast::http::field::host).empty())
{
res.result(boost::beast::http::status::bad_request);
- completeRequest();
+ completeRequest(res);
return;
}
}
@@ -353,25 +353,27 @@
if (res.completed)
{
- completeRequest();
+ completeRequest(res);
return;
}
#ifndef BMCWEB_INSECURE_DISABLE_AUTHENTICATION
if (!crow::authorization::isOnAllowlist(req->url, req->method()) &&
thisReq.session == nullptr)
{
- BMCWEB_LOG_WARNING << "[AuthMiddleware] authorization failed";
+ BMCWEB_LOG_WARNING << "Authentication failed";
forward_unauthorized::sendUnauthorized(
req->url, req->getHeaderValue("User-Agent"),
req->getHeaderValue("Accept"), res);
- completeRequest();
+ completeRequest(res);
return;
}
#endif // BMCWEB_INSECURE_DISABLE_AUTHENTICATION
- res.setCompleteRequestHandler([self(shared_from_this())] {
- boost::asio::post(self->adaptor.get_executor(),
- [self] { self->completeRequest(); });
- });
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ BMCWEB_LOG_DEBUG << "Setting completion handler";
+ asyncResp->res.setCompleteRequestHandler(
+ [self(shared_from_this())](crow::Response& thisRes) {
+ self->completeRequest(thisRes);
+ });
if (thisReq.isUpgrade() &&
boost::iequals(
@@ -381,10 +383,9 @@
handler->handleUpgrade(thisReq, res, std::move(adaptor));
// delete lambda with self shared_ptr
// to enable connection destruction
- res.setCompleteRequestHandler(nullptr);
+ asyncResp->res.setCompleteRequestHandler(nullptr);
return;
}
- auto asyncResp = std::make_shared<bmcweb::AsyncResp>(res);
handler->handle(thisReq, asyncResp);
}
@@ -408,8 +409,7 @@
boost::asio::ip::tcp::socket>>)
{
adaptor.next_layer().close();
-#ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
- if (userSession != nullptr)
+ if (sessionIsFromTransport && userSession != nullptr)
{
BMCWEB_LOG_DEBUG
<< this
@@ -417,7 +417,6 @@
persistent_data::SessionStore::getInstance().removeSession(
userSession);
}
-#endif // BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
}
else
{
@@ -425,12 +424,13 @@
}
}
- void completeRequest()
+ void completeRequest(crow::Response& thisRes)
{
if (!req)
{
return;
}
+ res = std::move(thisRes);
BMCWEB_LOG_INFO << "Response: " << this << ' ' << req->url << ' '
<< res.resultInt() << " keepalive=" << req->keepAlive();
@@ -483,7 +483,7 @@
res.keepAlive(req->keepAlive());
- doWrite();
+ doWrite(res);
// delete lambda with self shared_ptr
// to enable connection destruction
@@ -627,11 +627,11 @@
});
}
- void doWrite()
+ void doWrite(crow::Response& thisRes)
{
BMCWEB_LOG_DEBUG << this << " doWrite";
- res.preparePayload();
- serializer.emplace(*res.stringResponse);
+ thisRes.preparePayload();
+ serializer.emplace(*thisRes.stringResponse);
startDeadline();
boost::beast::http::async_write(
adaptor, *serializer,
diff --git a/http/http_response.hpp b/http/http_response.hpp
index 08b76f2..3c2a3f9 100644
--- a/http/http_response.hpp
+++ b/http/http_response.hpp
@@ -43,15 +43,25 @@
Response(const Response&) = delete;
Response(Response&&) = delete;
+
Response& operator=(const Response& r) = delete;
Response& operator=(Response&& r) noexcept
{
- BMCWEB_LOG_DEBUG << "Moving response containers";
+ BMCWEB_LOG_DEBUG << "Moving response containers; this: " << this
+ << "; other: " << &r;
+ if (this == &r)
+ {
+ return *this;
+ }
stringResponse = std::move(r.stringResponse);
r.stringResponse.emplace(response_type{});
jsonValue = std::move(r.jsonValue);
completed = r.completed;
+ completeRequestHandler = std::move(r.completeRequestHandler);
+ isAliveHelper = std::move(r.isAliveHelper);
+ r.completeRequestHandler = nullptr;
+ r.isAliveHelper = nullptr;
return *this;
}
@@ -117,37 +127,53 @@
{
if (completed)
{
- BMCWEB_LOG_ERROR << "Response was ended twice";
+ BMCWEB_LOG_ERROR << this << " Response was ended twice";
return;
}
completed = true;
- BMCWEB_LOG_DEBUG << "calling completion handler";
+ BMCWEB_LOG_DEBUG << this << " calling completion handler";
if (completeRequestHandler)
{
- BMCWEB_LOG_DEBUG << "completion handler was valid";
- completeRequestHandler();
+ BMCWEB_LOG_DEBUG << this << " completion handler was valid";
+ completeRequestHandler(*this);
}
}
- void end(std::string_view bodyPart)
- {
- write(bodyPart);
- end();
- }
-
bool isAlive()
{
return isAliveHelper && isAliveHelper();
}
- void setCompleteRequestHandler(std::function<void()> newHandler)
+ void setCompleteRequestHandler(std::function<void(Response&)>&& handler)
{
- completeRequestHandler = std::move(newHandler);
+ BMCWEB_LOG_DEBUG << this << " setting completion handler";
+ completeRequestHandler = std::move(handler);
+ }
+
+ std::function<void(Response&)> releaseCompleteRequestHandler()
+ {
+ BMCWEB_LOG_DEBUG << this << " releasing completion handler"
+ << static_cast<bool>(completeRequestHandler);
+ std::function<void(Response&)> ret = completeRequestHandler;
+ completeRequestHandler = nullptr;
+ return ret;
+ }
+
+ void setIsAliveHelper(std::function<bool()>&& handler)
+ {
+ isAliveHelper = std::move(handler);
+ }
+
+ std::function<bool()> releaseIsAliveHelper()
+ {
+ std::function<bool()> ret = std::move(isAliveHelper);
+ isAliveHelper = nullptr;
+ return ret;
}
private:
- bool completed{};
- std::function<void()> completeRequestHandler;
+ bool completed = false;
+ std::function<void(Response&)> completeRequestHandler;
std::function<bool()> isAliveHelper;
// In case of a JSON object, set the Content-Type header
diff --git a/http/websocket.hpp b/http/websocket.hpp
index daa5100..bd7b841 100644
--- a/http/websocket.hpp
+++ b/http/websocket.hpp
@@ -199,10 +199,10 @@
{
BMCWEB_LOG_DEBUG << "Websocket accepted connection";
- auto asyncResp = std::make_shared<bmcweb::AsyncResp>(
- res, [this, self(shared_from_this())]() { doRead(); });
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
asyncResp->res.result(boost::beast::http::status::ok);
+ doRead();
if (openHandler)
{
diff --git a/include/async_resp.hpp b/include/async_resp.hpp
index 607d168..219d9df 100644
--- a/include/async_resp.hpp
+++ b/include/async_resp.hpp
@@ -15,12 +15,7 @@
class AsyncResp
{
public:
- AsyncResp(crow::Response& response) : res(response)
- {}
-
- AsyncResp(crow::Response& response, std::function<void()>&& function) :
- res(response), func(std::move(function))
- {}
+ AsyncResp() = default;
AsyncResp(const AsyncResp&) = delete;
AsyncResp(AsyncResp&&) = delete;
@@ -29,16 +24,10 @@
~AsyncResp()
{
- if (func && res.result() == boost::beast::http::status::ok)
- {
- func();
- }
-
res.end();
}
- crow::Response& res;
- std::function<void()> func;
+ crow::Response res;
};
} // namespace bmcweb
diff --git a/include/openbmc_dbus_rest.hpp b/include/openbmc_dbus_rest.hpp
index cb5d727..9f84309 100644
--- a/include/openbmc_dbus_rest.hpp
+++ b/include/openbmc_dbus_rest.hpp
@@ -2283,9 +2283,7 @@
[](const crow::Request&,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
const std::string& connection) {
- introspectObjects(
- connection, "/",
- std::make_shared<bmcweb::AsyncResp>(asyncResp->res));
+ introspectObjects(connection, "/", asyncResp);
});
BMCWEB_ROUTE(app, "/bus/system/<str>/<path>")
diff --git a/redfish-core/lib/sensors.hpp b/redfish-core/lib/sensors.hpp
index a53c112..dd2d752 100644
--- a/redfish-core/lib/sensors.hpp
+++ b/redfish-core/lib/sensors.hpp
@@ -2910,10 +2910,9 @@
return;
}
- auto res = std::make_shared<crow::Response>();
- auto asyncResp = std::make_shared<bmcweb::AsyncResp>(*res);
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
auto callback =
- [res, asyncResp, mapCompleteCb{std::move(mapComplete)}](
+ [asyncResp, mapCompleteCb{std::move(mapComplete)}](
const boost::beast::http::status status,
const boost::container::flat_map<std::string, std::string>&
uriToDbus) { mapCompleteCb(status, uriToDbus); };