From 56a427f83909940060c154dc7b550de61991ca41 Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Tue, 24 Aug 2021 20:11:08 +0100 Subject: [PATCH] Upgrade Boost to 1.70, fix inefficient connection handling A request to osrm-routed can be assigned to a thread which is currently busy processing another request, even when there are other threads/cores available. This unnecessarily delays the response, and can make requests appear to hang when awaiting CPU intensive requests to finish. The issue looks like a bug in Boost.Asio multithreaded networking stack. osrm-routed server implementation is heavily influenced by the HTTP server 3 example in the Boost.Asio docs. By upgrading to Boost 1.70 and updating the server connections to match the example provided in the 1.70 release, the problem is resolved. The diff of the changes to the Boost.Asio stack are vast, so it's difficult to identify the exact cause. However the implementation change is to push the strand of execution into the socket (and timer) objects, which suggests it could fix the type of threading issue we are observing. --- CHANGELOG.md | 3 +++ CMakeLists.txt | 4 ++-- docker/Dockerfile | 13 ++++++------ include/server/connection.hpp | 4 ++-- include/server/server.hpp | 14 ++++++------- src/server/connection.cpp | 39 +++++++++++++++++------------------ 6 files changed, 40 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cca1ffdefb8..70c690b94f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,10 @@ # Unreleased - Changes from 5.26.0 + - API: + - FIXED: Fix inefficient osrm-routed connection handling [#6113](https://github.com/Project-OSRM/osrm-backend/pull/6113) - Build: - CHANGED: Use Github Actions for building container images [#6138](https://github.com/Project-OSRM/osrm-backend/pull/6138) + - CHANGED: Upgrade Boost dependency to 1.70 [#6113](https://github.com/Project-OSRM/osrm-backend/pull/6113) # 5.26.0 - Changes from 5.25.0 diff --git a/CMakeLists.txt b/CMakeLists.txt index e49fac29aad..aef612daa6f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -36,7 +36,7 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") if(ENABLE_MASON) # versions in use - set(MASON_BOOST_VERSION "1.65.1") + set(MASON_BOOST_VERSION "1.73.0") set(MASON_EXPAT_VERSION "2.2.0") set(MASON_LUA_VERSION "5.2.4") set(MASON_BZIP2_VERSION "1.0.6") @@ -521,7 +521,7 @@ if(ENABLE_MASON) include_directories(SYSTEM ${CMAKE_CURRENT_SOURCE_DIR}/third_party/libosmium/include) else() - find_package(Boost 1.54 REQUIRED COMPONENTS ${BOOST_COMPONENTS}) + find_package(Boost 1.70 REQUIRED COMPONENTS ${BOOST_COMPONENTS}) add_dependency_includes(${Boost_INCLUDE_DIRS}) find_package(TBB REQUIRED) diff --git a/docker/Dockerfile b/docker/Dockerfile index 056f8c7b6ef..e3d69e3b373 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -1,4 +1,4 @@ -FROM debian:stretch-slim as builder +FROM debian:bullseye-slim as builder ARG DOCKER_TAG ARG BUILD_CONCURRENCY RUN mkdir -p /src && mkdir -p /opt @@ -8,7 +8,7 @@ WORKDIR /src RUN NPROC=${BUILD_CONCURRENCY:-$(grep -c ^processor /proc/cpuinfo 2>/dev/null || 1)} && \ apt-get update && \ apt-get -y --no-install-recommends install cmake make git gcc g++ libbz2-dev libxml2-dev \ - libzip-dev libboost1.62-all-dev lua5.2 liblua5.2-dev libtbb-dev -o APT::Install-Suggests=0 -o APT::Install-Recommends=0 && \ + libzip-dev libboost1.74-all-dev lua5.2 liblua5.2-dev libtbb-dev -o APT::Install-Suggests=0 -o APT::Install-Recommends=0 && \ echo "Building OSRM ${DOCKER_TAG}" && \ git show --format="%H" | head -n1 > /opt/OSRM_GITSHA && \ echo "Building OSRM gitsha $(cat /opt/OSRM_GITSHA)" && \ @@ -30,12 +30,13 @@ RUN NPROC=${BUILD_CONCURRENCY:-$(grep -c ^processor /proc/cpuinfo 2>/dev/null || # Multistage build to reduce image size - https://docs.docker.com/engine/userguide/eng-image/multistage-build/#use-multi-stage-builds # Only the content below ends up in the image, this helps remove /src from the image (which is large) -FROM debian:stretch-slim as runstage +FROM debian:bullseye-slim as runstage RUN mkdir -p /src && mkdir -p /opt RUN apt-get update && \ - apt-get install -y --no-install-recommends libboost-program-options1.62.0 libboost-regex1.62.0 \ - libboost-date-time1.62.0 libboost-chrono1.62.0 libboost-filesystem1.62.0 \ - libboost-iostreams1.62.0 libboost-thread1.62.0 expat liblua5.2-0 libtbb2 &&\ + apt-get install -y --no-install-recommends libboost-program-options1.74.0 libboost-regex1.74.0 \ + libboost-date-time1.74.0 libboost-chrono1.74.0 libboost-filesystem1.74.0 \ + libboost-iostreams1.74.0 libboost-system1.74.0 libboost-thread1.74.0 \ + expat liblua5.2-0 libtbb2 &&\ rm -rf /var/lib/apt/lists/* COPY --from=builder /usr/local /usr/local COPY --from=builder /opt /opt diff --git a/include/server/connection.hpp b/include/server/connection.hpp index b40fa2912f3..df5b47d2681 100644 --- a/include/server/connection.hpp +++ b/include/server/connection.hpp @@ -37,7 +37,7 @@ class RequestHandler; class Connection : public std::enable_shared_from_this { public: - explicit Connection(boost::asio::io_service &io_service, RequestHandler &handler); + explicit Connection(boost::asio::io_context &io_context, RequestHandler &handler); Connection(const Connection &) = delete; Connection &operator=(const Connection &) = delete; @@ -60,7 +60,7 @@ class Connection : public std::enable_shared_from_this std::vector compress_buffers(const std::vector &uncompressed_data, const http::compression_type compression_type); - boost::asio::io_service::strand strand; + boost::asio::strand strand; boost::asio::ip::tcp::socket TCP_socket; boost::asio::deadline_timer timer; RequestHandler &request_handler; diff --git a/include/server/server.hpp b/include/server/server.hpp index 06d2ec8e92b..1cc3f11d24f 100644 --- a/include/server/server.hpp +++ b/include/server/server.hpp @@ -43,12 +43,12 @@ class Server } explicit Server(const std::string &address, const int port, const unsigned thread_pool_size) - : thread_pool_size(thread_pool_size), acceptor(io_service), - new_connection(std::make_shared(io_service, request_handler)) + : thread_pool_size(thread_pool_size), acceptor(io_context), + new_connection(std::make_shared(io_context, request_handler)) { const auto port_string = std::to_string(port); - boost::asio::ip::tcp::resolver resolver(io_service); + boost::asio::ip::tcp::resolver resolver(io_context); boost::asio::ip::tcp::resolver::query query(address, port_string); boost::asio::ip::tcp::endpoint endpoint = *resolver.resolve(query); @@ -74,7 +74,7 @@ class Server for (unsigned i = 0; i < thread_pool_size; ++i) { std::shared_ptr thread = std::make_shared( - boost::bind(&boost::asio::io_service::run, &io_service)); + boost::bind(&boost::asio::io_context::run, &io_context)); threads.push_back(thread); } for (auto thread : threads) @@ -83,7 +83,7 @@ class Server } } - void Stop() { io_service.stop(); } + void Stop() { io_context.stop(); } void RegisterServiceHandler(std::unique_ptr service_handler_) { @@ -96,7 +96,7 @@ class Server if (!e) { new_connection->start(); - new_connection = std::make_shared(io_service, request_handler); + new_connection = std::make_shared(io_context, request_handler); acceptor.async_accept( new_connection->socket(), boost::bind(&Server::HandleAccept, this, boost::asio::placeholders::error)); @@ -108,7 +108,7 @@ class Server } unsigned thread_pool_size; - boost::asio::io_service io_service; + boost::asio::io_context io_context; boost::asio::ip::tcp::acceptor acceptor; std::shared_ptr new_connection; RequestHandler request_handler; diff --git a/src/server/connection.cpp b/src/server/connection.cpp index 31fed90e8e2..ace70b75429 100644 --- a/src/server/connection.cpp +++ b/src/server/connection.cpp @@ -14,8 +14,9 @@ namespace osrm namespace server { -Connection::Connection(boost::asio::io_service &io_service, RequestHandler &handler) - : strand(io_service), TCP_socket(io_service), timer(io_service), request_handler(handler) +Connection::Connection(boost::asio::io_context &io_context, RequestHandler &handler) + : strand(boost::asio::make_strand(io_context)), TCP_socket(strand), timer(strand), + request_handler(handler) { } @@ -24,12 +25,11 @@ boost::asio::ip::tcp::socket &Connection::socket() { return TCP_socket; } /// Start the first asynchronous operation for the connection. void Connection::start() { - TCP_socket.async_read_some( - boost::asio::buffer(incoming_data_buffer), - strand.wrap(boost::bind(&Connection::handle_read, - this->shared_from_this(), - boost::asio::placeholders::error, - boost::asio::placeholders::bytes_transferred))); + TCP_socket.async_read_some(boost::asio::buffer(incoming_data_buffer), + boost::bind(&Connection::handle_read, + this->shared_from_this(), + boost::asio::placeholders::error, + boost::asio::placeholders::bytes_transferred)); if (keep_alive) { @@ -123,9 +123,9 @@ void Connection::handle_read(const boost::system::error_code &error, std::size_t // write result to stream boost::asio::async_write(TCP_socket, output_buffer, - strand.wrap(boost::bind(&Connection::handle_write, - this->shared_from_this(), - boost::asio::placeholders::error))); + boost::bind(&Connection::handle_write, + this->shared_from_this(), + boost::asio::placeholders::error)); } else if (result == RequestParser::RequestStatus::invalid) { // request is not parseable @@ -133,19 +133,18 @@ void Connection::handle_read(const boost::system::error_code &error, std::size_t boost::asio::async_write(TCP_socket, current_reply.to_buffers(), - strand.wrap(boost::bind(&Connection::handle_write, - this->shared_from_this(), - boost::asio::placeholders::error))); + boost::bind(&Connection::handle_write, + this->shared_from_this(), + boost::asio::placeholders::error)); } else { // we don't have a result yet, so continue reading - TCP_socket.async_read_some( - boost::asio::buffer(incoming_data_buffer), - strand.wrap(boost::bind(&Connection::handle_read, - this->shared_from_this(), - boost::asio::placeholders::error, - boost::asio::placeholders::bytes_transferred))); + TCP_socket.async_read_some(boost::asio::buffer(incoming_data_buffer), + boost::bind(&Connection::handle_read, + this->shared_from_this(), + boost::asio::placeholders::error, + boost::asio::placeholders::bytes_transferred)); } }