From 72c66173c4ac334a12af27be22bb897b39e0aeb3 Mon Sep 17 00:00:00 2001 From: Sridhar Madhugiri Date: Fri, 4 May 2018 14:59:19 -0700 Subject: [PATCH 1/4] Improve error handling in on_accept --- .../src/http/listener/http_server_asio.cpp | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/Release/src/http/listener/http_server_asio.cpp b/Release/src/http/listener/http_server_asio.cpp index 78193fa3e2..4297e3cbdd 100644 --- a/Release/src/http/listener/http_server_asio.cpp +++ b/Release/src/http/listener/http_server_asio.cpp @@ -540,27 +540,43 @@ will_deref_and_erase_t asio_server_connection::start_request_response() void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system::error_code& ec) { - std::unique_ptr usocket(std::move(socket)); + // Listener closed + if (ec == boost::asio::error::operation_aborted) + { + return; + } + // Handle successfull accept if (!ec) { + std::unique_ptr usocket(std::move(socket)); auto conn = new asio_server_connection(std::move(usocket), m_p_server, this); std::lock_guard lock(m_connections_lock); m_connections.insert(conn); - conn->start(m_is_https, m_ssl_context_callback); - if (m_connections.size() == 1) - m_all_connections_complete.reset(); - - if (m_acceptor) + try { - // spin off another async accept - auto newSocket = new ip::tcp::socket(crossplat::threadpool::shared_instance().service()); - m_acceptor->async_accept(*newSocket, [this, newSocket](const boost::system::error_code& ec) - { - this->on_accept(newSocket, ec); - }); + conn->start(m_is_https, m_ssl_context_callback); + if (m_connections.size() == 1) + m_all_connections_complete.reset(); } + catch (boost::system::system_error&) + { + // boost ssl apis throw boost::system::system_error. + // Exception indicates something went wrong setting ssl context. + // Drop connection and continue handling other connections. + internal_erase_connection(conn); + } + } + + if (m_acceptor) + { + // spin off another async accept + auto newSocket = new ip::tcp::socket(crossplat::threadpool::shared_instance().service()); + m_acceptor->async_accept(*newSocket, [this, newSocket](const boost::system::error_code& ec) + { + this->on_accept(newSocket, ec); + }); } } From 48e2cc55156a2d2d1a7b9d52e1b75d8499e15880 Mon Sep 17 00:00:00 2001 From: Sridhar Madhugiri Date: Tue, 8 May 2018 23:29:53 -0700 Subject: [PATCH 2/4] Move lock to the top of the function --- Release/src/http/listener/http_server_asio.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Release/src/http/listener/http_server_asio.cpp b/Release/src/http/listener/http_server_asio.cpp index 4297e3cbdd..59bf483f55 100644 --- a/Release/src/http/listener/http_server_asio.cpp +++ b/Release/src/http/listener/http_server_asio.cpp @@ -540,6 +540,8 @@ will_deref_and_erase_t asio_server_connection::start_request_response() void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system::error_code& ec) { + std::lock_guard lock(m_connections_lock); + // Listener closed if (ec == boost::asio::error::operation_aborted) { @@ -552,7 +554,6 @@ void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system:: std::unique_ptr usocket(std::move(socket)); auto conn = new asio_server_connection(std::move(usocket), m_p_server, this); - std::lock_guard lock(m_connections_lock); m_connections.insert(conn); try { From 89f9e40d44ffd92f91ad159a0637e344f6188bae Mon Sep 17 00:00:00 2001 From: Sridhar Madhugiri Date: Thu, 10 May 2018 16:59:05 -0700 Subject: [PATCH 3/4] Lock shared data at the right locations. --- Release/src/http/listener/http_server_asio.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Release/src/http/listener/http_server_asio.cpp b/Release/src/http/listener/http_server_asio.cpp index 59bf483f55..31047f8f51 100644 --- a/Release/src/http/listener/http_server_asio.cpp +++ b/Release/src/http/listener/http_server_asio.cpp @@ -540,23 +540,24 @@ will_deref_and_erase_t asio_server_connection::start_request_response() void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system::error_code& ec) { - std::lock_guard lock(m_connections_lock); - // Listener closed if (ec == boost::asio::error::operation_aborted) { return; } - // Handle successfull accept + // Handle successful accept if (!ec) { std::unique_ptr usocket(std::move(socket)); auto conn = new asio_server_connection(std::move(usocket), m_p_server, this); - m_connections.insert(conn); try { + // Take a lock here so it released before call to internal_erase_connection + std::lock_guard lock(m_connections_lock); + + m_connections.insert(conn); conn->start(m_is_https, m_ssl_context_callback); if (m_connections.size() == 1) m_all_connections_complete.reset(); @@ -570,6 +571,7 @@ void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system:: } } + std::lock_guard lock(m_connections_lock); if (m_acceptor) { // spin off another async accept From fcc2015b17c62f400f4dd41ac550d5e9b93d3e76 Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Thu, 2 Aug 2018 22:22:58 -0700 Subject: [PATCH 4/4] [http_listener] improve refcount and lifetime management by using RAII. --- .../src/http/listener/http_server_asio.cpp | 69 ++++++++++++++----- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/Release/src/http/listener/http_server_asio.cpp b/Release/src/http/listener/http_server_asio.cpp index 31047f8f51..bef47126dc 100644 --- a/Release/src/http/listener/http_server_asio.cpp +++ b/Release/src/http/listener/http_server_asio.cpp @@ -200,7 +200,7 @@ namespace } private: - void on_accept(boost::asio::ip::tcp::socket* socket, const boost::system::error_code& ec); + void on_accept(std::unique_ptr socket, const boost::system::error_code& ec); }; @@ -333,7 +333,6 @@ class asio_server_connection std::unique_ptr m_ssl_context; std::unique_ptr m_ssl_stream; -public: asio_server_connection(std::unique_ptr socket, http_linux_server* server, hostport_listener* parent) : m_socket(std::move(socket)) , m_request_buf() @@ -341,12 +340,34 @@ class asio_server_connection , m_p_server(server) , m_p_parent(parent) , m_close(false) + , m_chunked(false) , m_refs(1) { } - will_deref_and_erase_t start(bool is_https, const std::function& ssl_context_callback) + struct Dereferencer + { + void operator()(asio_server_connection* conn) const { conn->deref(); } + }; + +public: + using refcount_ptr = std::unique_ptr; + + static refcount_ptr create(std::unique_ptr socket, http_linux_server* server, hostport_listener* parent) { + return refcount_ptr(new asio_server_connection(std::move(socket), server, parent)); + } + + refcount_ptr get_reference() + { + ++m_refs; + return refcount_ptr(this); + } + + will_erase_from_parent_t start_connection(bool is_https, const std::function& ssl_context_callback) + { + auto unique_reference = this->get_reference(); + if (is_https) { m_ssl_context = make_unique(boost::asio::ssl::context::sslv23); @@ -360,11 +381,14 @@ class asio_server_connection { (will_deref_and_erase_t)this->start_request_response(); }); - return will_deref_and_erase_t{}; + unique_reference.release(); + return will_erase_from_parent_t{}; } else { - return start_request_response(); + (will_deref_and_erase_t)start_request_response(); + unique_reference.release(); + return will_erase_from_parent_t{}; } } @@ -385,7 +409,7 @@ class asio_server_connection will_deref_and_erase_t dispatch_request_to_listener(); will_erase_from_parent_t do_response() { - ++m_refs; + auto unique_reference = this->get_reference(); m_request.get_response().then([=](pplx::task r_task) { http_response response; @@ -406,11 +430,12 @@ class asio_server_connection (will_deref_and_erase_t)this->async_write(&asio_server_connection::handle_headers_written, response); }); }); + unique_reference.release(); return will_erase_from_parent_t{}; } will_erase_from_parent_t do_bad_response() { - ++m_refs; + auto unique_reference = this->get_reference(); m_request.get_response().then([=](pplx::task r_task) { http_response response; @@ -428,6 +453,7 @@ class asio_server_connection (will_deref_and_erase_t)async_write(&asio_server_connection::handle_headers_written, response); }); + unique_reference.release(); return will_erase_from_parent_t{}; } @@ -495,10 +521,13 @@ void hostport_listener::start() m_acceptor->listen(0 != m_backlog ? m_backlog : socket_base::max_connections); auto socket = new ip::tcp::socket(service); + std::unique_ptr usocket(socket); m_acceptor->async_accept(*socket, [this, socket](const boost::system::error_code& ec) { - this->on_accept(socket, ec); + std::unique_ptr usocket(socket); + this->on_accept(std::move(usocket), ec); }); + usocket.release(); } void asio_server_connection::close() @@ -538,7 +567,7 @@ will_deref_and_erase_t asio_server_connection::start_request_response() return will_deref_and_erase_t{}; } -void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system::error_code& ec) +void hostport_listener::on_accept(std::unique_ptr socket, const boost::system::error_code& ec) { // Listener closed if (ec == boost::asio::error::operation_aborted) @@ -546,19 +575,21 @@ void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system:: return; } + std::lock_guard lock(m_connections_lock); + // Handle successful accept if (!ec) { - std::unique_ptr usocket(std::move(socket)); - auto conn = new asio_server_connection(std::move(usocket), m_p_server, this); + auto conn = asio_server_connection::create(std::move(socket), m_p_server, this); + m_connections.insert(conn.get()); try { - // Take a lock here so it released before call to internal_erase_connection - std::lock_guard lock(m_connections_lock); + (will_erase_from_parent_t)conn->start_connection(m_is_https, m_ssl_context_callback); + // at this point an asynchronous task has been launched which will call + // m_connections.erase(conn.get()) eventually - m_connections.insert(conn); - conn->start(m_is_https, m_ssl_context_callback); + // the following cannot throw if (m_connections.size() == 1) m_all_connections_complete.reset(); } @@ -567,19 +598,21 @@ void hostport_listener::on_accept(ip::tcp::socket* socket, const boost::system:: // boost ssl apis throw boost::system::system_error. // Exception indicates something went wrong setting ssl context. // Drop connection and continue handling other connections. - internal_erase_connection(conn); + m_connections.erase(conn.get()); } } - std::lock_guard lock(m_connections_lock); if (m_acceptor) { // spin off another async accept auto newSocket = new ip::tcp::socket(crossplat::threadpool::shared_instance().service()); + std::unique_ptr usocket(newSocket); m_acceptor->async_accept(*newSocket, [this, newSocket](const boost::system::error_code& ec) { - this->on_accept(newSocket, ec); + std::unique_ptr usocket(newSocket); + this->on_accept(std::move(usocket), ec); }); + usocket.release(); } }