From 3c0d0ecc282194905431e1082b5e5552e71d9a24 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 5 Jun 2026 00:48:32 +0200 Subject: [PATCH] LibThreading: Remove BackgroundAction Remove BackgroundAction after ImageDecoder stopped using it. Drop its dedicated test target and sources from LibThreading. Replace the LibCore stream tests' remaining usage with explicit Thread objects so the tree no longer depends on the removed API. --- Libraries/LibThreading/BackgroundAction.cpp | 86 -------------- Libraries/LibThreading/BackgroundAction.h | 97 ---------------- Libraries/LibThreading/CMakeLists.txt | 1 - Tests/LibCore/TestLibCoreStream.cpp | 22 ++-- Tests/LibThreading/CMakeLists.txt | 3 - Tests/LibThreading/TestBackgroundAction.cpp | 119 -------------------- 6 files changed, 13 insertions(+), 315 deletions(-) delete mode 100644 Libraries/LibThreading/BackgroundAction.cpp delete mode 100644 Libraries/LibThreading/BackgroundAction.h delete mode 100644 Tests/LibThreading/TestBackgroundAction.cpp diff --git a/Libraries/LibThreading/BackgroundAction.cpp b/Libraries/LibThreading/BackgroundAction.cpp deleted file mode 100644 index c1a61e5f00..0000000000 --- a/Libraries/LibThreading/BackgroundAction.cpp +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright (c) 2019-2020, Sergey Bugaev - * Copyright (c) 2021, Andreas Kling - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#include -#include -#include -#include - -static pthread_mutex_t s_mutex = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t s_condition = PTHREAD_COND_INITIALIZER; -static Queue>* s_all_actions; -static Threading::Thread* s_background_thread; -static Atomic s_background_thread_should_run = true; - -static intptr_t background_thread_func() -{ - Vector> actions; - while (s_background_thread_should_run.load(AK::MemoryOrder::memory_order_acquire)) { - pthread_mutex_lock(&s_mutex); - - while (s_all_actions->is_empty() && s_background_thread_should_run.load(AK::MemoryOrder::memory_order_acquire)) - pthread_cond_wait(&s_condition, &s_mutex); - - while (!s_all_actions->is_empty()) - actions.append(s_all_actions->dequeue()); - - pthread_mutex_unlock(&s_mutex); - - for (auto& action : actions) { - if (s_background_thread_should_run.load(AK::MemoryOrder::memory_order_acquire)) - action(); - } - actions.clear(); - } - return 0; -} - -static void init() -{ - s_all_actions = new Queue>; - s_background_thread = &Threading::Thread::construct("Background"sv, background_thread_func).leak_ref(); - s_background_thread->start(); -} - -void Threading::quit_background_thread() -{ - if (!s_background_thread) - return; - - s_background_thread_should_run.store(false, AK::MemoryOrder::memory_order_release); - - pthread_mutex_lock(&s_mutex); - pthread_cond_broadcast(&s_condition); - pthread_mutex_unlock(&s_mutex); - - MUST(s_background_thread->join()); - - delete s_all_actions; - s_background_thread->unref(); - s_all_actions = nullptr; - s_background_thread = nullptr; - - s_background_thread_should_run.store(true, AK::MemoryOrder::memory_order_release); -} - -Threading::Thread& Threading::BackgroundActionBase::background_thread() -{ - if (s_background_thread == nullptr) - init(); - return *s_background_thread; -} - -void Threading::BackgroundActionBase::enqueue_work(Function work) -{ - if (s_all_actions == nullptr) - init(); - - pthread_mutex_lock(&s_mutex); - s_all_actions->enqueue(move(work)); - pthread_cond_broadcast(&s_condition); - pthread_mutex_unlock(&s_mutex); -} diff --git a/Libraries/LibThreading/BackgroundAction.h b/Libraries/LibThreading/BackgroundAction.h deleted file mode 100644 index 4efab3cb63..0000000000 --- a/Libraries/LibThreading/BackgroundAction.h +++ /dev/null @@ -1,97 +0,0 @@ -/* - * Copyright (c) 2019-2020, Sergey Bugaev - * Copyright (c) 2021, Andreas Kling - * Copyright (c) 2022-2023, the SerenityOS developers. - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#pragma once - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -namespace Threading { - -template -class BackgroundAction; - -class BackgroundActionBase { - template - friend class BackgroundAction; - -private: - BackgroundActionBase() = default; - - static void enqueue_work(ESCAPING Function); - static Thread& background_thread(); -}; - -template -class BackgroundAction final - : public Core::EventReceiver - , private BackgroundActionBase { - C_OBJECT(BackgroundAction); - -public: - virtual ~BackgroundAction() = default; - - Optional const& result() const { return m_result; } - Optional& result() { return m_result; } - - // Cancellation is a best-effort cross-thread signal. No other state is protected by this flag. - // It is not used to synchronize access to any other state (m_result), so relaxed atomics are fine. - void cancel() { m_canceled.store(true, AK::MemoryOrder::memory_order_relaxed); } - // If your action is long-running, you should periodically check the cancel state and possibly return early. - bool is_canceled() const { return m_canceled.load(AK::MemoryOrder::memory_order_relaxed); } - -private: - BackgroundAction(ESCAPING Function(BackgroundAction&)> action, ESCAPING Function on_complete, ESCAPING Function on_error = {}) - : m_action(move(action)) - , m_on_complete(move(on_complete)) - , m_on_error(move(on_error)) - { - enqueue_work([self = NonnullRefPtr(*this), origin_event_loop = Core::EventLoop::current_weak()]() mutable { - auto result = self->m_action(*self); - - auto event_loop = origin_event_loop->take(); - if (!event_loop) { - dbgln("BackgroundAction {:p} was dropped, the origin loop is gone.", self.ptr()); - return; - } - event_loop->deferred_invoke([self = move(self), result = move(result)]() mutable { - auto const canceled = self->m_canceled.load(AK::MemoryOrder::memory_order_relaxed); - - if (canceled) - return; - - if (result.is_error()) { - if (self->m_on_error) - self->m_on_error(result.release_error()); - return; - } - - if (self->m_on_complete) - self->m_on_complete(result.release_value()); - }); - }); - } - - Function(BackgroundAction&)> m_action; - Function m_on_complete; - Function m_on_error; - Optional m_result; - Atomic m_canceled { false }; -}; - -void quit_background_thread(); - -} diff --git a/Libraries/LibThreading/CMakeLists.txt b/Libraries/LibThreading/CMakeLists.txt index a6ae6ccfe8..928dfd49d4 100644 --- a/Libraries/LibThreading/CMakeLists.txt +++ b/Libraries/LibThreading/CMakeLists.txt @@ -1,5 +1,4 @@ set(SOURCES - BackgroundAction.cpp Thread.cpp ThreadPool.cpp ) diff --git a/Tests/LibCore/TestLibCoreStream.cpp b/Tests/LibCore/TestLibCoreStream.cpp index 8a82cf52f5..bdb4366015 100644 --- a/Tests/LibCore/TestLibCoreStream.cpp +++ b/Tests/LibCore/TestLibCoreStream.cpp @@ -17,7 +17,7 @@ #include #include #include -#include +#include #include #include @@ -360,8 +360,9 @@ TEST_CASE(local_socket_read) // impasse. LocalSocket::connect blocks because there's nobody to // accept, and LocalServer::accept blocks because there's nobody // connected. - auto background_action = Threading::BackgroundAction::construct( - [&socket_path](auto&) { + auto client_thread = Threading::Thread::construct( + "LocalSocketRead"sv, + [socket_path] { Core::EventLoop event_loop; auto client_socket = MUST(Core::LocalSocket::connect(socket_path)); @@ -381,10 +382,11 @@ TEST_CASE(local_socket_read) EXPECT_EQ(sent_data, received_data); return 0; - }, - nullptr); + }); + client_thread->start(); event_loop.exec(); + MUST(client_thread->join()); ::unlink(socket_path.characters()); } @@ -421,18 +423,20 @@ TEST_CASE(local_socket_write) }; // NOTE: Same reason as in the local_socket_read test. - auto background_action = Threading::BackgroundAction::construct( - [&socket_path](auto&) { + auto client_thread = Threading::Thread::construct( + "LocalSocketWrite"sv, + [socket_path] { auto client_socket = MUST(Core::LocalSocket::connect(socket_path)); MUST(client_socket->write_until_depleted({ sent_data.characters_without_null_termination(), sent_data.length() })); client_socket->close(); return 0; - }, - nullptr); + }); + client_thread->start(); event_loop.exec(); + MUST(client_thread->join()); ::unlink(socket_path.characters()); } diff --git a/Tests/LibThreading/CMakeLists.txt b/Tests/LibThreading/CMakeLists.txt index afb74b4154..efd7b60f19 100644 --- a/Tests/LibThreading/CMakeLists.txt +++ b/Tests/LibThreading/CMakeLists.txt @@ -1,10 +1,7 @@ set(TEST_SOURCES - TestBackgroundAction.cpp TestThread.cpp ) foreach(source IN LISTS TEST_SOURCES) ladybird_test("${source}" LibThreading LIBS LibThreading) endforeach() - -target_link_libraries(TestBackgroundAction PRIVATE LibSync) diff --git a/Tests/LibThreading/TestBackgroundAction.cpp b/Tests/LibThreading/TestBackgroundAction.cpp deleted file mode 100644 index 916a75ad87..0000000000 --- a/Tests/LibThreading/TestBackgroundAction.cpp +++ /dev/null @@ -1,119 +0,0 @@ -/* - * Copyright (c) 2026, The Ladybird developers - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -using namespace AK::TimeLiterals; - -static void spin_until(Core::EventLoop& loop, Function condition, AK::Duration timeout = 2000_ms) -{ - i64 const timeout_ms = timeout.to_milliseconds(); - for (i64 elapsed_ms = 0; elapsed_ms < timeout_ms; elapsed_ms += 5) { - (void)loop.pump(Core::EventLoop::WaitMode::PollForEvents); - if (condition()) - return; - MUST(Core::System::sleep_ms(5)); - } - - FAIL("Timed out waiting for condition"); -} - -TEST_CASE(background_action_on_error_called_on_action_failure_and_on_origin_thread) -{ - Core::EventLoop loop; - - pthread_t const origin_thread_id = pthread_self(); - - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic action_ran = false; - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic on_error_called = false; - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic on_complete_called = false; - - Optional action_thread_id; - - auto background_action = Threading::BackgroundAction::construct( - [&](auto&) -> ErrorOr { - action_thread_id = pthread_self(); - action_ran.store(true, AK::MemoryOrder::memory_order_relaxed); - return Error::from_string_literal("action failed"); - }, - [&](int) { - on_complete_called.store(true, AK::MemoryOrder::memory_order_relaxed); - loop.quit(1); - }, - [&](Error error) { - EXPECT(pthread_equal(origin_thread_id, pthread_self())); - EXPECT_EQ(error.string_literal(), "action failed"sv); - on_error_called.store(true, AK::MemoryOrder::memory_order_relaxed); - loop.quit(0); - }); - - loop.exec(); - - EXPECT(action_ran.load(AK::MemoryOrder::memory_order_relaxed)); - EXPECT(action_thread_id.has_value()); - EXPECT(!pthread_equal(action_thread_id.value(), origin_thread_id)); - EXPECT(on_error_called.load(AK::MemoryOrder::memory_order_relaxed)); - EXPECT(!on_complete_called.load(AK::MemoryOrder::memory_order_relaxed)); - - (void)background_action; -} - -TEST_CASE(background_action_cancel_suppresses_on_error_and_on_complete) -{ - Core::EventLoop loop; - - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic started = false; - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic finished = false; - - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic on_error_count = 0; - IGNORE_USE_IN_ESCAPING_LAMBDA Atomic on_complete_count = 0; - - auto background_action = Threading::BackgroundAction::construct( - [&](auto& action) -> ErrorOr { - started.store(true, AK::MemoryOrder::memory_order_relaxed); - - while (!action.is_canceled()) - MUST(Core::System::sleep_ms(1)); - - finished.store(true, AK::MemoryOrder::memory_order_relaxed); - return Error::from_string_literal("error after cancel"); - }, - [&](int) { - on_complete_count.fetch_add(1, AK::MemoryOrder::memory_order_relaxed); - }, - [&](Error) { - on_error_count.fetch_add(1, AK::MemoryOrder::memory_order_relaxed); - }); - - spin_until(loop, [&] { - return started.load(AK::MemoryOrder::memory_order_relaxed); - }); - - background_action->cancel(); - - spin_until(loop, [&] { - return finished.load(AK::MemoryOrder::memory_order_relaxed); - }); - - // Run the loop a bit more to ensure any incorrectly-posted callbacks would execute. - for (size_t i = 0; i < 50; ++i) { - (void)loop.pump(Core::EventLoop::WaitMode::PollForEvents); - MUST(Core::System::sleep_ms(1)); - } - - EXPECT(background_action->is_canceled()); - EXPECT_EQ(on_complete_count.load(AK::MemoryOrder::memory_order_relaxed), 0); - EXPECT_EQ(on_error_count.load(AK::MemoryOrder::memory_order_relaxed), 0); -}