Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpConnection Leakage #552

Closed
blippyblips opened this issue Oct 30, 2022 · 2 comments · Fixed by #633
Closed

HttpConnection Leakage #552

blippyblips opened this issue Oct 30, 2022 · 2 comments · Fixed by #633

Comments

@blippyblips
Copy link

There's a leaking HttpConnection in http_server.h:224:
auto p = new Connection<Adaptor, Handler, Middlewares...>( is, handler_, server_name_, middlewares_, get_cached_date_str_pool_[service_idx], *task_timer_pool_[service_idx], adaptor_ctx_, task_queue_length_pool_[service_idx]);

It only gets deleted in case of an error, if the connection is still open or successfully completed it doesn't get deleted on shutdown

@z16166
Copy link
Contributor

z16166 commented Dec 8, 2022

I just get the same leak report with Visual Leak Detector(for Windows).

also refer to: #546

If I press CTRL + C to close my program before any successful client connection, it leaks. "io_service_.stop()" will not lead to "delete p;" to free p.

If I add "acceptor_.cancel();" as the following, and add a bool flag "stop_" to prevent it from calling "do_accept()" again on program shutdown, it crashes when executing "delete p", because "tasks_.erase(id);" will crash if tasks_ is empty.

        void stop()
        {
            stop_ = true;

            acceptor_.cancel();
            io_service_.stop();
            for (auto& io_service : io_service_pool_)
                io_service->stop();
        }

        void do_accept()
        {
          if (stop_)
            return;

@z16166
Copy link
Contributor

z16166 commented Dec 8, 2022

well, I have added a connection queue in crow::Server to manage all connections.

       volatile bool stop_;
        std::mutex lock_;
        typename std::set<crow::Connection<Adaptor, Handler, Middlewares...> *> connections_;

In crow::Server::stop(), just call the Stop() method of each connection.

        void crow::Server::CloseConnections() {
              std::lock_guard<std::mutex> guard{lock_};
              std::for_each(connections_.begin(), connections_.end(), [](auto &conn) { conn->Stop(); });
        }

        void crow::Server::stop()
        {
            stop_ = true;

            acceptor_.cancel();
            CloseConnections();

            io_service_.stop();
            for (auto& io_service : io_service_pool_)
                io_service->stop();
        }

The Stop() method of crow::Connection is newly added:

       typename crow::Server<Handler, Adaptor, Middlewares...> *server_;   //add this member for crow::Connection

        void  crow::Connection::Stop() {
            cancel_deadline_timer();

            adaptor_.shutdown_readwrite();
            adaptor_.close();
        }

also, when a crow::Connection object is created or deleted, just update the above queue/set.

        void crow::Server::OnConnectionNew(crow::Connection<Adaptor, Handler, Middlewares...> *conn) {
            std::lock_guard<std::mutex> guard{lock_};

            connections_.insert(conn);
        }

        void crow::Server::OnConnectionDestroy(crow::Connection<Adaptor, Handler, Middlewares...> *conn) {
            std::lock_guard<std::mutex> guard{lock_};

            if (!connections_.empty())
                connections_.erase(conn);
        }
        void crow::Server::do_accept()
        {
          if (stop_)
            return;

            uint16_t service_idx = pick_io_service_idx();
            asio::io_service& is = *io_service_pool_[service_idx];
            task_queue_length_pool_[service_idx]++;
            CROW_LOG_DEBUG << &is << " {" << service_idx << "} queue length: " << task_queue_length_pool_[service_idx];

            auto p = new Connection<Adaptor, Handler, Middlewares...>(
              is, handler_, server_name_, middlewares_,
              get_cached_date_str_pool_[service_idx], *task_timer_pool_[service_idx], adaptor_ctx_, task_queue_length_pool_[service_idx], this);
            
            OnConnectionNew(p);

            acceptor_.async_accept(
              p->socket(),
              [this, p, &is, service_idx](boost::system::error_code ec) {
                  if (!ec)
                  {
                      is.post(
                        [p] {
                            p->start();
                        });
                  }
                  else
                  {
                      task_queue_length_pool_[service_idx]--;
                      CROW_LOG_DEBUG << &is << " {" << service_idx << "} queue length: " << task_queue_length_pool_[service_idx];
                      OnConnectionDestroy(p);
                      delete p;
                  }
                  do_accept();
              });
        void crow::Connection::check_destroy()
        {
            CROW_LOG_DEBUG << this << " is_reading " << is_reading << " is_writing " << is_writing;
            if (!is_reading && !is_writing)
            {
                queue_length_--;
                CROW_LOG_DEBUG << this << " delete (idle) (queue length: " << queue_length_ << ')';
                server_->OnConnectionDestroy(this);
                delete this;
            }
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants