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>

4 files changed, 70 insertions, 83 deletions

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 @@ constexpr unsigned int httpReqBodyLimit =
     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 @@ class Connection
                                      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 @@ class Connection
 
             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 @@ class Connection
                 *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 @@ class Connection
         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 @@ class Connection
                     cancelDeadlineTimer();
                     close();
                     BMCWEB_LOG_DEBUG << this << " from read(1)";
-                    checkDestroy();
                     return;
                 }
 
@@ -740,17 +745,15 @@ class Connection
 
     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 @@ class Connection
                     cancelDeadlineTimer();
                     close();
                     BMCWEB_LOG_DEBUG << this << " from read(1)";
-                    checkDestroy();
                     return;
                 }
                 handle();
@@ -780,30 +782,26 @@ class Connection
 
     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 @@ class Connection
             });
     }
 
-    void checkDestroy()
+    void cancelDeadlineTimer()
     {
-        BMCWEB_LOG_DEBUG << this << " isReading " << isReading << " isWriting "
-                         << isWriting;
-        if (!isReading && !isWriting)
+        if (timerCancelKey)
         {
-            BMCWEB_LOG_DEBUG << this << " delete (idle) ";
-            delete this;
+            BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue
+                             << ' ' << *timerCancelKey;
+            timerQueue.cancel(*timerCancelKey);
+            timerCancelKey.reset();
         }
     }
 
-    void cancelDeadlineTimer()
-    {
-        BMCWEB_LOG_DEBUG << this << " timer cancelled: " << &timerQueue << ' '
-                         << timerCancelKey;
-        timerQueue.cancel(timerCancelKey);
-    }
-
     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 @@ class Connection
             close();
         });
         BMCWEB_LOG_DEBUG << this << " timer added: " << &timerQueue << ' '
-                         << timerCancelKey;
+                         << *timerCancelKey;
     }
 
   private:
@@ -877,10 +871,8 @@ class Connection
 
     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 @@ class Connection
 
     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 @@ class Server
         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 @@ class Server
             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 @@ class Server
                                        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 @@ class Server
                                                *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 @@ class Server
                         boost::asio::post(*this->ioService,
                                           [p] { p->start(); });
                     }
-                    else
-                    {
-                        delete p;
-                    }
                     doAccept();
                 });
         }
@@ -284,6 +275,7 @@ 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;
 
@@ -292,6 +284,7 @@ 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 f194ad1..c2a7503 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& 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 @@ 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, 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 @@ namespace websocket
 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 @@ struct Connection : std::enable_shared_from_this<Connection>
         return userdataPtr;
     }
 
-    crow::Request req;
+    boost::beast::http::request<boost::beast::http::string_body> req;
     crow::Response res;
 
   private:
@@ -51,14 +51,14 @@ template <typename Adaptor> class ConnectionImpl : public Connection
 {
   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 @@ template <typename Adaptor> class ConnectionImpl : public Connection
 
         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())