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

@@ -11,10 +11,10 @@
 #include <string>
 #include <vector>
 
-#include "absl/cleanup/cleanup.h"
 #include "engine/ExportQueryExecutionTrees.h"
 #include "engine/QueryPlanner.h"
 #include "util/BoostHelpers/AsyncWaitForFuture.h"
+#include "util/OnDestructionDontThrowDuringStackUnwinding.h"
 
 template <typename T>
 using Awaitable = Server::Awaitable<T>;
@@ -720,12 +720,8 @@ Awaitable<T> 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(

src/parser/sparqlParser/SparqlQleverVisitor.cpp

@@ -10,7 +10,6 @@
 #include <string>
 #include <vector>
 
-#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());

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

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_);
   }

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.

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);
+}