Revert "Connection and websockets fixes"

This reverts commit c00500bcb9c5145f5cacb78bbe3dd694fb85ba0a.

Reason: Makes image upload fail

Tested: Image upload works again

requests.post(
         'https://{}/redfish/v1/UpdateService'.format(args.address),
         data=file.read(), verify=False,
         auth=(args.username, args.password))

Change-Id: Iaf780d052d98accdead32e87f468002f5141b19a
Signed-off-by: James Feist <james.feist@linux.intel.com>

4 files changed, 83 insertions, 70 deletions

diff --git a/http/http_connection.h b/http/http_connection.h
index 4ef3bc6..7d24fe7 100644
--- a/http/http_connection.h
+++ b/http/http_connection.h
@@ -248,8 +248,7 @@ constexpr unsigned int httpReqBodyLimit =
     1024 * 1024 * BMCWEB_HTTP_REQ_BODY_LIMIT_MB;
 
 template <typename Adaptor, typename Handler, typename... Middlewares>
-class Connection : public std::enable_shared_from_this<
-                       Connection<Adaptor, Handler, Middlewares...>>
+class Connection
 {
   public:
     Connection(boost::asio::io_context& ioService, Handler* handlerIn,
@@ -475,15 +474,16 @@ class Connection : public std::enable_shared_from_this<
                                      boost::beast::ssl_stream<
                                          boost::asio::ip::tcp::socket>>)
         {
-            adaptor.async_handshake(boost::asio::ssl::stream_base::server,
-                                    [this, self(shared_from_this())](
-                                        const boost::system::error_code& ec) {
-                                        if (ec)
-                                        {
-                                            return;
-                                        }
-                                        doReadHeaders();
-                                    });
+            adaptor.async_handshake(
+                boost::asio::ssl::stream_base::server,
+                [this](const boost::system::error_code& ec) {
+                    if (ec)
+                    {
+                        checkDestroy();
+                        return;
+                    }
+                    doReadHeaders();
+                });
         }
         else
         {
@@ -561,21 +561,18 @@ class Connection : public std::enable_shared_from_this<
 
             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
@@ -641,16 +638,15 @@ class Connection : public std::enable_shared_from_this<
                 *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())
@@ -687,23 +683,21 @@ class Connection : public std::enable_shared_from_this<
         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,
-             self(shared_from_this())](const boost::system::error_code& ec,
-                                       std::size_t bytes_transferred) {
+            [this](const boost::system::error_code& ec,
+                   std::size_t bytes_transferred) {
+                isReading = false;
                 BMCWEB_LOG_ERROR << this << " async_read_header "
                                  << bytes_transferred << " Bytes";
                 bool errorWhileReading = false;
@@ -728,6 +722,7 @@ class Connection : public std::enable_shared_from_this<
                     cancelDeadlineTimer();
                     close();
                     BMCWEB_LOG_DEBUG << this << " from read(1)";
+                    checkDestroy();
                     return;
                 }
 
@@ -745,15 +740,17 @@ class Connection : public std::enable_shared_from_this<
 
     void doRead()
     {
+        // auto self = this->shared_from_this();
+        isReading = true;
         BMCWEB_LOG_DEBUG << this << " doRead";
 
         boost::beast::http::async_read(
             adaptor, buffer, *parser,
-            [this,
-             self(shared_from_this())](const boost::system::error_code& ec,
-                                       std::size_t bytes_transferred) {
+            [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)
@@ -774,6 +771,7 @@ class Connection : public std::enable_shared_from_this<
                     cancelDeadlineTimer();
                     close();
                     BMCWEB_LOG_DEBUG << this << " from read(1)";
+                    checkDestroy();
                     return;
                 }
                 handle();
@@ -782,26 +780,30 @@ class Connection : public std::enable_shared_from_this<
 
     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,
-            [this,
-             self(shared_from_this())](const boost::system::error_code& ec,
-                                       std::size_t bytes_transferred) {
+            [&](const boost::system::error_code& ec,
+                std::size_t bytes_transferred) {
+                isWriting = false;
                 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;
                 }
 
@@ -818,25 +820,29 @@ class Connection : public std::enable_shared_from_this<
             });
     }
 
-    void cancelDeadlineTimer()
+    void checkDestroy()
     {
-        if (timerCancelKey)
+        BMCWEB_LOG_DEBUG << this << " isReading " << isReading << " isWriting "
+                         << isWriting;
+        if (!isReading && !isWriting)
         {
-            BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue
-                             << ' ' << *timerCancelKey;
-            timerQueue.cancel(*timerCancelKey);
-            timerCancelKey.reset();
+            BMCWEB_LOG_DEBUG << this << " delete (idle) ";
+            delete this;
         }
     }
 
+    void cancelDeadlineTimer()
+    {
+        BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue << ' '
+                         << timerCancelKey;
+        timerQueue.cancel(timerCancelKey);
+    }
+
     void startDeadline()
     {
         cancelDeadlineTimer();
 
-        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();
+        timerCancelKey = timerQueue.add([this] {
             if (!isAlive())
             {
                 return;
@@ -844,7 +850,7 @@ class Connection : public std::enable_shared_from_this<
             close();
         });
         BMCWEB_LOG_DEBUG << this << " timer added: " << &timerQueue << ' '
-                         << *timerCancelKey;
+                         << timerCancelKey;
     }
 
   private:
@@ -871,8 +877,10 @@ class Connection : public std::enable_shared_from_this<
 
     const std::string& serverName;
 
-    std::optional<size_t> timerCancelKey;
+    size_t timerCancelKey = 0;
 
+    bool isReading{};
+    bool isWriting{};
     bool needToCallAfterHandlers{};
     bool needToStartReadAfterComplete{};
 
@@ -881,8 +889,5 @@ class Connection : public std::enable_shared_from_this<
 
     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 6e63cbd..df42214 100644
--- a/http/http_server.h
+++ b/http/http_server.h
@@ -44,8 +44,7 @@ class Server
         ioService(std::move(io)),
         acceptor(std::move(acceptor)),
         signals(*ioService, SIGINT, SIGTERM, SIGHUP), tickTimer(*ioService),
-        timer(*ioService), handler(handler), middlewares(middlewares),
-        adaptorCtx(adaptor_ctx)
+        handler(handler), middlewares(middlewares), adaptorCtx(adaptor_ctx)
     {
     }
 
@@ -124,9 +123,11 @@ class Server
             return this->dateStr;
         };
 
+        boost::asio::steady_timer timer(*ioService);
         timer.expires_after(std::chrono::seconds(1));
 
-        timerHandler = [this](const boost::system::error_code& ec) {
+        std::function<void(const boost::system::error_code& ec)> timerHandler;
+        timerHandler = [&](const boost::system::error_code& ec) {
             if (ec)
             {
                 return;
@@ -230,8 +231,8 @@ class Server
                                        boost::asio::ip::tcp::socket>>::value)
         {
             adaptorTemp = Adaptor(*ioService, *adaptorCtx);
-            auto p =
-                std::make_shared<Connection<Adaptor, Handler, Middlewares...>>(
+            Connection<Adaptor, Handler, Middlewares...>* p =
+                new Connection<Adaptor, Handler, Middlewares...>(
                     *ioService, handler, serverName, middlewares,
                     getCachedDateStr, timerQueue,
                     std::move(adaptorTemp.value()));
@@ -244,14 +245,18 @@ class Server
                                                *this->ioService,
                                                [p] { p->start(); });
                                        }
+                                       else
+                                       {
+                                           delete p;
+                                       }
                                        doAccept();
                                    });
         }
         else
         {
             adaptorTemp = Adaptor(*ioService);
-            auto p =
-                std::make_shared<Connection<Adaptor, Handler, Middlewares...>>(
+            Connection<Adaptor, Handler, Middlewares...>* p =
+                new Connection<Adaptor, Handler, Middlewares...>(
                     *ioService, handler, serverName, middlewares,
                     getCachedDateStr, timerQueue,
                     std::move(adaptorTemp.value()));
@@ -263,6 +268,10 @@ class Server
                         boost::asio::post(*this->ioService,
                                           [p] { p->start(); });
                     }
+                    else
+                    {
+                        delete p;
+                    }
                     doAccept();
                 });
         }
@@ -275,7 +284,6 @@ class Server
     std::unique_ptr<tcp::acceptor> acceptor;
     boost::asio::signal_set signals;
     boost::asio::steady_timer tickTimer;
-    boost::asio::steady_timer timer;
 
     std::string dateStr;
 
@@ -284,7 +292,6 @@ class Server
 
     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 c2a7503..f194ad1 100644
--- a/http/routing.h
+++ b/http/routing.h
@@ -324,19 +324,19 @@ class WebSocketRule : public BaseRule
         res.end();
     }
 
-    void handleUpgrade(const Request& req, Response&,
+    void handleUpgrade(const Request& req, Response& res,
                        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, std::move(adaptor), openHandler, messageHandler,
+                req, res, std::move(adaptor), openHandler, messageHandler,
                 closeHandler, errorHandler);
         myConnection->start();
     }
 #ifdef BMCWEB_ENABLE_SSL
-    void handleUpgrade(const Request& req, Response&,
+    void handleUpgrade(const Request& req, Response& res,
                        boost::beast::ssl_stream<boost::asio::ip::tcp::socket>&&
                            adaptor) override
     {
@@ -344,7 +344,7 @@ class WebSocketRule : public BaseRule
             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, std::move(adaptor), openHandler, messageHandler,
+                req, res, std::move(adaptor), openHandler, messageHandler,
                 closeHandler, errorHandler);
         myConnection->start();
     }
diff --git a/http/websocket.h b/http/websocket.h
index f7c818e..80d536a 100644
--- a/http/websocket.h
+++ b/http/websocket.h
@@ -20,8 +20,8 @@ namespace websocket
 struct Connection : std::enable_shared_from_this<Connection>
 {
   public:
-    explicit Connection(const crow::Request& reqIn) :
-        req(reqIn.req), userdataPtr(nullptr){};
+    explicit Connection(const crow::Request& reqIn, crow::Response& res) :
+        req(reqIn), userdataPtr(nullptr){};
 
     virtual void sendBinary(const std::string_view msg) = 0;
     virtual void sendBinary(std::string&& msg) = 0;
@@ -40,7 +40,7 @@ struct Connection : std::enable_shared_from_this<Connection>
         return userdataPtr;
     }
 
-    boost::beast::http::request<boost::beast::http::string_body> req;
+    crow::Request req;
     crow::Response res;
 
   private:
@@ -51,14 +51,14 @@ template <typename Adaptor> class ConnectionImpl : public Connection
 {
   public:
     ConnectionImpl(
-        const crow::Request& reqIn, Adaptor adaptorIn,
+        const crow::Request& reqIn, crow::Response& res, 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),
+        Connection(reqIn, res),
         ws(std::move(adaptorIn)), inString(), inBuffer(inString, 131088),
         openHandler(std::move(open_handler)),
         messageHandler(std::move(message_handler)),
@@ -80,11 +80,12 @@ template <typename Adaptor> class ConnectionImpl : public Connection
 
         using bf = boost::beast::http::field;
 
-        std::string_view protocol = req[bf::sec_websocket_protocol];
+        std::string_view protocol =
+            req.getHeaderValue(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())