From db760224dee1571a727550121133af4162be5dd6 Mon Sep 17 00:00:00 2001 From: Johannes Kalmbach Date: Wed, 7 Jun 2023 19:47:43 +0200 Subject: [PATCH] Don't terminate the server when socket operation fails (#996) When a socket operation was aborted, the closing of the socket threw an exception due to a subtle bug. That exception was then not caught and the server was terminated. The bug is now fixed and exceptions that would lead to a termination of the program are now suppressed by handling the cleanup with our own `ad_utility::makeOnDestructionDontThrowDuringStackUnwinding` instead of with `absl::Cleanup`. --- src/engine/Server.cpp | 10 +++------- src/parser/sparqlParser/SparqlQleverVisitor.cpp | 16 +++++++++------- .../OnDestructionDontThrowDuringStackUnwinding.h | 7 +++++++ src/util/Synchronized.h | 5 +++-- src/util/http/HttpServer.h | 11 +++++------ ...tructionDontThrowDuringStackUnwindingTest.cpp | 16 ++++++++++++++++ 6 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index c7e49fde2c..3ff82e1177 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -11,10 +11,10 @@ #include #include -#include "absl/cleanup/cleanup.h" #include "engine/ExportQueryExecutionTrees.h" #include "engine/QueryPlanner.h" #include "util/BoostHelpers/AsyncWaitForFuture.h" +#include "util/OnDestructionDontThrowDuringStackUnwinding.h" template using Awaitable = Server::Awaitable; @@ -720,12 +720,8 @@ Awaitable Server::computeInNewThread(Function function) const { auto acquireComputeRelease = [this, function = std::move(function)] { LOG(DEBUG) << "Acquiring new thread for query processing\n"; queryProcessingSemaphore_.acquire(); - absl::Cleanup f{[this]() noexcept { - try { - queryProcessingSemaphore_.release(); - } catch (...) { - } - }}; + auto cleanup = ad_utility::makeOnDestructionDontThrowDuringStackUnwinding( + [this]() { queryProcessingSemaphore_.release(); }); return function(); }; co_return co_await ad_utility::asio_helpers::async_on_external_thread( diff --git a/src/parser/sparqlParser/SparqlQleverVisitor.cpp b/src/parser/sparqlParser/SparqlQleverVisitor.cpp index 193a5089df..3dc6a6fe2e 100644 --- a/src/parser/sparqlParser/SparqlQleverVisitor.cpp +++ b/src/parser/sparqlParser/SparqlQleverVisitor.cpp @@ -10,7 +10,6 @@ #include #include -#include "absl/cleanup/cleanup.h" #include "absl/strings/str_join.h" #include "engine/sparqlExpressions/LangExpression.h" #include "engine/sparqlExpressions/RandomExpression.h" @@ -20,6 +19,7 @@ #include "parser/TokenizerCtre.h" #include "parser/TurtleParser.h" #include "parser/data/Variable.h" +#include "util/OnDestructionDontThrowDuringStackUnwinding.h" #include "util/StringUtils.h" using namespace ad_utility::sparql_types; @@ -282,12 +282,14 @@ GraphPattern Visitor::visit(Parser::GroupGraphPatternContext* ctx) { // the graph pattern are visible outside the graph pattern. auto visibleVariablesSoFar = std::move(visibleVariables_); visibleVariables_.clear(); - absl::Cleanup mergeVariables{[this, &visibleVariablesSoFar]() noexcept { - std::swap(visibleVariables_, visibleVariablesSoFar); - visibleVariables_.insert(visibleVariables_.end(), - visibleVariablesSoFar.begin(), - visibleVariablesSoFar.end()); - }}; + auto mergeVariables = + ad_utility::makeOnDestructionDontThrowDuringStackUnwinding( + [this, &visibleVariablesSoFar]() { + std::swap(visibleVariables_, visibleVariablesSoFar); + visibleVariables_.insert(visibleVariables_.end(), + visibleVariablesSoFar.begin(), + visibleVariablesSoFar.end()); + }); pattern._id = numGraphPatterns_++; if (ctx->subSelect()) { auto [subquery, valuesOpt] = visit(ctx->subSelect()); diff --git a/src/util/OnDestructionDontThrowDuringStackUnwinding.h b/src/util/OnDestructionDontThrowDuringStackUnwinding.h index eca9b8e7d5..546a5c1f9a 100644 --- a/src/util/OnDestructionDontThrowDuringStackUnwinding.h +++ b/src/util/OnDestructionDontThrowDuringStackUnwinding.h @@ -20,6 +20,7 @@ class OnDestructionDontThrowDuringStackUnwinding { private: F f_; int numExceptionsDuringConstruction_ = std::uncaught_exceptions(); + bool isCanceled_ = false; public: // It is forbidden to copy or move these objects. @@ -28,8 +29,14 @@ class OnDestructionDontThrowDuringStackUnwinding { OnDestructionDontThrowDuringStackUnwinding( OnDestructionDontThrowDuringStackUnwinding&&) noexcept = delete; + // Cancel the cleanup. When this has been called, the destructor will do + // nothing. + void cancel() { isCanceled_ = true; } // The destructor that calls `f_`. ~OnDestructionDontThrowDuringStackUnwinding() noexcept(false) { + if (isCanceled_) { + return; + } // If the number of uncaught exceptions is the same as when then constructor // was called, then it is safe to throw a possible exception For details see // https://en.cppreference.com/w/cpp/error/uncaught_exception, especially diff --git a/src/util/Synchronized.h b/src/util/Synchronized.h index 6d3874e38b..04c08dcd6a 100644 --- a/src/util/Synchronized.h +++ b/src/util/Synchronized.h @@ -14,6 +14,7 @@ #include "absl/cleanup/cleanup.h" #include "util/Forward.h" +#include "util/OnDestructionDontThrowDuringStackUnwinding.h" namespace ad_utility { @@ -132,11 +133,11 @@ class Synchronized { // It is important to create this AFTER the lock, s.t. the // nextOrderedRequest_ update is still protected. We must give it a name, // s.t. it is not destroyed immediately. - absl::Cleanup od{[&]() mutable noexcept { + auto od = ad_utility::makeOnDestructionDontThrowDuringStackUnwinding([&]() { ++nextOrderedRequest_; l.unlock(); requestCv_.notify_all(); - }}; + }); requestCv_.wait(l, [&]() { return requestNumber == nextOrderedRequest_; }); return f(data_); } diff --git a/src/util/http/HttpServer.h b/src/util/http/HttpServer.h index dfca244c9e..c6374a19c7 100644 --- a/src/util/http/HttpServer.h +++ b/src/util/http/HttpServer.h @@ -178,13 +178,12 @@ class HttpServer { beast::flat_buffer buffer; beast::tcp_stream stream{std::move(socket)}; - auto releaseConnection = absl::Cleanup{ - - [&stream]() noexcept { - beast::error_code ec; + auto releaseConnection = + ad_utility::makeOnDestructionDontThrowDuringStackUnwinding([&stream]() { + [[maybe_unused]] beast::error_code ec; stream.socket().shutdown(tcp::socket::shutdown_send, ec); - stream.socket().close(); - }}; + stream.socket().close(ec); + }); // Keep track of whether we have to close the session after a // request/response pair. diff --git a/test/OnDestructionDontThrowDuringStackUnwindingTest.cpp b/test/OnDestructionDontThrowDuringStackUnwindingTest.cpp index cf7f9573f0..5ceb68f9fa 100644 --- a/test/OnDestructionDontThrowDuringStackUnwindingTest.cpp +++ b/test/OnDestructionDontThrowDuringStackUnwindingTest.cpp @@ -93,3 +93,19 @@ TEST(OnDestruction, OnDestructionDontThrowDuringStackUnwinding) { ASSERT_THROW(runCleanupNested2(), std::out_of_range); ASSERT_EQ(i, 18); } + +// ________________________________________________________________ +TEST(OnDestruction, cancel) { + int i = 12; + { + auto cl = ad_utility::makeOnDestructionDontThrowDuringStackUnwinding( + [&i] { i = 24; }); + } + ASSERT_EQ(i, 24); + { + auto cl = ad_utility::makeOnDestructionDontThrowDuringStackUnwinding( + [&i] { i = 123; }); + cl.cancel(); + } + ASSERT_EQ(i, 24); +}