Skip to content

Commit

Permalink
Fix memory leaks caused from the http_connection
Browse files Browse the repository at this point in the history 
This memory leak was caused from the fact that we were creating a raw
pointers in the do_accept method of the http_server. The initial
connection was never released, but all subsequent connection
were released without problem. So this commit introduces shared_ptr
to manage all the connection and release the memory when the
connection is not needed any more.
  • Loading branch information
@anagno
anagno authored and Vasileios Anagnostopoulos committed May 8, 2023
1 parent 3b81e16 commit 1111a9e
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 77 deletions.
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR)
endif()

# Set required C++ standard
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED TRUE)

# Default to build type "Release" unless tests are being built
Expand Down
114 changes: 41 additions & 73 deletions include/crow/http_connection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <atomic>
#include <chrono>
#include <vector>
#include <memory>

#include "crow/http_parser_merged.h"
#include "crow/common.h"
Expand All @@ -31,7 +32,7 @@ namespace crow

/// An HTTP connection.
template<typename Adaptor, typename Handler, typename... Middlewares>
class Connection
class Connection: public std::enable_shared_from_this<Connection<Adaptor, Handler, Middlewares...>>
{
friend struct crow::response;

Expand Down Expand Up @@ -64,8 +65,6 @@ namespace crow

~Connection()
{
res.complete_request_handler_ = nullptr;
cancel_deadline_timer();
#ifdef CROW_ENABLE_DEBUG
connectionCount--;
CROW_LOG_DEBUG << "Connection (" << this << ") freed, total: " << connectionCount;
Expand All @@ -80,18 +79,17 @@ namespace crow

void start()
{
adaptor_.start([this](const asio::error_code& ec) {
adaptor_.start([self = this->shared_from_this()](const asio::error_code& ec) {
if (!ec)
{
start_deadline();
parser_.clear();
self->start_deadline();
self->parser_.clear();

do_read();
self->do_read();
}
else
{
CROW_LOG_ERROR << "Could not start adaptor: " << ec.message();
check_destroy();
}
});
}
Expand Down Expand Up @@ -161,9 +159,9 @@ namespace crow
need_to_call_after_handlers_ = false;
if (!is_invalid_request)
{
res.complete_request_handler_ = [] {};
res.is_alive_helper_ = [this]() -> bool {
return adaptor_.is_open();
res.complete_request_handler_ = nullptr;
res.is_alive_helper_ = [self = this->shared_from_this()]() -> bool {
return self->adaptor_.is_open();
};

ctx_ = detail::context<Middlewares...>();
Expand All @@ -176,8 +174,8 @@ namespace crow

if (!res.completed_)
{
res.complete_request_handler_ = [this] {
this->complete_request();
res.complete_request_handler_ = [self = this->shared_from_this()] {
self->complete_request();
};
need_to_call_after_handlers_ = true;
handler_->handle(req_, res, routing_handle_result_);
Expand All @@ -199,6 +197,7 @@ namespace crow
void complete_request()
{
CROW_LOG_INFO << "Response: " << this << ' ' << req_.raw_url << ' ' << res.code << ' ' << close_connection_;
res.is_alive_helper_ = nullptr;

if (need_to_call_after_handlers_)
{
Expand Down Expand Up @@ -267,8 +266,8 @@ namespace crow
private:
void prepare_buffers()
{
//auto self = this->shared_from_this();
res.complete_request_handler_ = nullptr;
res.is_alive_helper_ = nullptr;

if (!adaptor_.is_open())
{
Expand Down Expand Up @@ -384,7 +383,6 @@ namespace crow

void do_write_static()
{
is_writing = true;
asio::write(adaptor_.socket(), buffers_);

if (res.file_info.statResult == 0)
Expand All @@ -400,13 +398,11 @@ namespace crow
is.read(buf, sizeof(buf));
}
}
is_writing = false;
if (close_connection_)
{
adaptor_.shutdown_readwrite();
adaptor_.close();
CROW_LOG_DEBUG << this << " from write (static)";
check_destroy();
}

res.end();
Expand All @@ -433,7 +429,6 @@ namespace crow
}
else
{
is_writing = true;
asio::write(adaptor_.socket(), buffers_); // Write the response start / headers
cancel_deadline_timer();
if (res.body.length() > 0)
Expand All @@ -459,13 +454,11 @@ namespace crow
buffers.push_back(asio::buffer(buf));
do_write_sync(buffers);
}
is_writing = false;
if (close_connection_)
{
adaptor_.shutdown_readwrite();
adaptor_.close();
CROW_LOG_DEBUG << this << " from write (res_stream)";
check_destroy();
}

res.end();
Expand All @@ -477,77 +470,66 @@ namespace crow

void do_read()
{
//auto self = this->shared_from_this();
is_reading = true;
adaptor_.socket().async_read_some(
asio::buffer(buffer_),
[this](const asio::error_code& ec, std::size_t bytes_transferred) {
[self = this->shared_from_this()](const asio::error_code& ec, std::size_t bytes_transferred) {
bool error_while_reading = true;
if (!ec)
{
bool ret = parser_.feed(buffer_.data(), bytes_transferred);
if (ret && adaptor_.is_open())
bool ret = self->parser_.feed(self->buffer_.data(), bytes_transferred);
if (ret && self->adaptor_.is_open())
{
error_while_reading = false;
}
}

if (error_while_reading)
{
cancel_deadline_timer();
parser_.done();
adaptor_.shutdown_read();
adaptor_.close();
is_reading = false;
CROW_LOG_DEBUG << this << " from read(1) with description: \"" << http_errno_description(static_cast<http_errno>(parser_.http_errno)) << '\"';
check_destroy();
self->cancel_deadline_timer();
self->parser_.done();
self->adaptor_.shutdown_read();
self->adaptor_.close();
CROW_LOG_DEBUG << self << " from read(1) with description: \"" << http_errno_description(static_cast<http_errno>(self->parser_.http_errno)) << '\"';
}
else if (close_connection_)
else if (self->close_connection_)
{
cancel_deadline_timer();
parser_.done();
is_reading = false;
check_destroy();
self->cancel_deadline_timer();
self->parser_.done();
// adaptor will close after write
}
else if (!need_to_call_after_handlers_)
else if (!self->need_to_call_after_handlers_)
{
start_deadline();
do_read();
self->start_deadline();
self->do_read();
}
else
{
// res will be completed later by user
need_to_start_read_after_complete_ = true;
self->need_to_start_read_after_complete_ = true;
}
});
}

void do_write()
{
//auto self = this->shared_from_this();
is_writing = true;
asio::async_write(
adaptor_.socket(), buffers_,
[&](const asio::error_code& ec, std::size_t /*bytes_transferred*/) {
is_writing = false;
res.clear();
res_body_copy_.clear();
parser_.clear();
[self = this->shared_from_this()](const asio::error_code& ec, std::size_t /*bytes_transferred*/) {
self->res.clear();
self->res_body_copy_.clear();
self->parser_.clear();
if (!ec)
{
if (close_connection_)
if (self->close_connection_)
{
adaptor_.shutdown_write();
adaptor_.close();
CROW_LOG_DEBUG << this << " from write(1)";
check_destroy();
self->adaptor_.shutdown_write();
self->adaptor_.close();
CROW_LOG_DEBUG << self << " from write(1)";
}
}
else
{
CROW_LOG_DEBUG << this << " from write(2)";
check_destroy();
CROW_LOG_DEBUG << self << " from write(2)";
}
});
}
Expand All @@ -564,23 +546,11 @@ namespace crow
{
CROW_LOG_ERROR << ec << " - happened while sending buffers";
CROW_LOG_DEBUG << this << " from write (sync)(2)";
check_destroy();
return true;
}
});
}

void 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_ << ')';
delete this;
}
}

void cancel_deadline_timer()
{
CROW_LOG_DEBUG << this << " timer cancelled: " << &task_timer_ << ' ' << task_id_;
Expand All @@ -591,13 +561,13 @@ namespace crow
{
cancel_deadline_timer();

task_id_ = task_timer_.schedule([this] {
if (!adaptor_.is_open())
task_id_ = task_timer_.schedule([self = this->shared_from_this()] {
if (!self->adaptor_.is_open())
{
return;
}
adaptor_.shutdown_readwrite();
adaptor_.close();
self->adaptor_.shutdown_readwrite();
self->adaptor_.close();
});
CROW_LOG_DEBUG << this << " timer added: " << &task_timer_ << ' ' << task_id_;
}
Expand All @@ -624,8 +594,6 @@ namespace crow

detail::task_timer::identifier_type task_id_{};

bool is_reading{};
bool is_writing{};
bool need_to_call_after_handlers_{};
bool need_to_start_read_after_complete_{};
bool add_keep_alive_{};
Expand Down
3 changes: 1 addition & 2 deletions include/crow/http_server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ namespace crow
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...>(
auto p = std::make_shared<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]);

Expand All @@ -239,7 +239,6 @@ namespace crow
{
task_queue_length_pool_[service_idx]--;
CROW_LOG_DEBUG << &is << " {" << service_idx << "} queue length: " << task_queue_length_pool_[service_idx];
delete p;
}
do_accept();
});
Expand Down
2 changes: 1 addition & 1 deletion include/crow/routing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1718,7 +1718,7 @@ namespace crow
return;
}

res.complete_request_handler_ = [&rule, &ctx, &container, &req, &res, &glob_completion_handler] {
res.complete_request_handler_ = [&rule, &ctx, &container, &req, &res, glob_completion_handler = std::move(glob_completion_handler)] {
detail::middleware_call_criteria_dynamic<true> crit_bwd(rule->mw_indices_.indices());

detail::after_handlers_call_helper<
Expand Down

0 comments on commit 1111a9e

Please sign in to comment.