From 0446513c06203ab42af200388ff9dfbced9c12b6 Mon Sep 17 00:00:00 2001
From: Marcus Holland-Moritz <github@mhxnet.de>
Date: Sun, 14 Jan 2024 18:00:09 +0100
Subject: [PATCH] fix: make sure exceptions in worker threads produce an error
 message

---
 src/dwarfs/worker_group.cpp | 10 +++++++++-
 test/test_helpers.cpp       | 19 +++++++++++++++----
 test/test_helpers.h         | 12 +++++++++---
 test/tool_main_test.cpp     | 13 +++++++++++++
 4 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/src/dwarfs/worker_group.cpp b/src/dwarfs/worker_group.cpp
index 589af68a..f640b7f7 100644
--- a/src/dwarfs/worker_group.cpp
+++ b/src/dwarfs/worker_group.cpp
@@ -24,6 +24,8 @@
 #include <cstdint>
 #include <cstring>
 #include <ctime>
+#include <exception>
+#include <iostream>
 #include <mutex>
 #include <queue>
 #include <string>
@@ -326,7 +328,13 @@ class basic_worker_group final : public worker_group::impl, private Policy {
           ::SetThreadPriority(hthr, THREAD_MODE_BACKGROUND_BEGIN);
         }
 #endif
-        job();
+        try {
+          job();
+        } catch (...) {
+          std::cerr << "FATAL ERROR: exception thrown in worker thread: "
+                    << folly::exceptionStr(std::current_exception()) << '\n';
+          std::abort();
+        }
 #ifdef _WIN32
         if (is_background) {
           ::SetThreadPriority(hthr, THREAD_MODE_BACKGROUND_END);
diff --git a/test/test_helpers.cpp b/test/test_helpers.cpp
index 478d114b..bc8e22fb 100644
--- a/test/test_helpers.cpp
+++ b/test/test_helpers.cpp
@@ -329,8 +329,11 @@ void os_access_mock::set_access_fail(fs::path const& path) {
 }
 
 void os_access_mock::set_map_file_error(std::filesystem::path const& path,
-                                        std::exception_ptr ep) {
-  map_file_err_[path] = std::move(ep);
+                                        std::exception_ptr ep,
+                                        size_t after_n_attempts) {
+  auto& e = map_file_errors_[path];
+  e.ep = std::move(ep);
+  e.remaining_successful_attempts = after_n_attempts;
 }
 
 size_t os_access_mock::size() const { return root_ ? root_->size() : 0; }
@@ -431,8 +434,16 @@ std::unique_ptr<mmif>
 os_access_mock::map_file(fs::path const& path, size_t size) const {
   if (auto de = find(path);
       de && de->status.type() == posix_file_type::regular) {
-    if (auto it = map_file_err_.find(path); it != map_file_err_.end()) {
-      std::rethrow_exception(it->second);
+    if (auto it = map_file_errors_.find(path); it != map_file_errors_.end()) {
+      size_t remaining = 0;
+      do {
+        remaining = it->second.remaining_successful_attempts.load();
+      } while (remaining > 0 &&
+               !it->second.remaining_successful_attempts.compare_exchange_weak(
+                   remaining, remaining - 1));
+      if (remaining == 0) {
+        std::rethrow_exception(it->second.ep);
+      }
     }
 
     return std::make_unique<mmap_mock>(
diff --git a/test/test_helpers.h b/test/test_helpers.h
index a3907748..93da45c1 100644
--- a/test/test_helpers.h
+++ b/test/test_helpers.h
@@ -21,6 +21,7 @@
 
 #pragma once
 
+#include <atomic>
 #include <exception>
 #include <filesystem>
 #include <functional>
@@ -92,8 +93,8 @@ class os_access_mock : public os_access {
   void add_local_files(std::filesystem::path const& path);
 
   void set_access_fail(std::filesystem::path const& path);
-  void
-  set_map_file_error(std::filesystem::path const& path, std::exception_ptr ep);
+  void set_map_file_error(std::filesystem::path const& path,
+                          std::exception_ptr ep, size_t after_n_attempts = 0);
 
   void setenv(std::string name, std::string value);
 
@@ -119,6 +120,11 @@ class os_access_mock : public os_access {
   std::optional<std::string> getenv(std::string_view name) const override;
 
  private:
+  struct error_info {
+    std::exception_ptr ep{};
+    std::atomic<size_t> mutable remaining_successful_attempts{0};
+  };
+
   static std::vector<std::string> splitpath(std::filesystem::path const& path);
   struct mock_dirent* find(std::filesystem::path const& path) const;
   struct mock_dirent* find(std::vector<std::string> parts) const;
@@ -128,7 +134,7 @@ class os_access_mock : public os_access {
   std::unique_ptr<mock_dirent> root_;
   size_t ino_{1000000};
   std::set<std::filesystem::path> access_fail_set_;
-  std::map<std::filesystem::path, std::exception_ptr> map_file_err_;
+  std::map<std::filesystem::path, error_info> map_file_errors_;
   std::map<std::string, std::string> env_;
 };
 
diff --git a/test/tool_main_test.cpp b/test/tool_main_test.cpp
index 5de7ca71..4e312978 100644
--- a/test/tool_main_test.cpp
+++ b/test/tool_main_test.cpp
@@ -2114,3 +2114,16 @@ TEST(mkdwarfs_test, map_file_error) {
               ::testing::HasSubstr("map_file_error, creating empty inode"));
   EXPECT_THAT(t.err(), ::testing::HasSubstr("filesystem created with 1 error"));
 }
+
+// TODO: enable this once we know how to make it work reliably
+#if 0
+TEST(mkdwarfs_test, map_file_error_delayed) {
+  mkdwarfs_tester t;
+  t.os->set_map_file_error(
+      "/somedir/ipsum.py",
+      std::make_exception_ptr(std::runtime_error("map_file_error")), 1);
+
+  EXPECT_DEATH(t.run("-i / -o - --categorize --no-progress --log-level=error"),
+               "");
+}
+#endif