From 2fcb713037fc30d410312b190ca20b99e1668ce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= Date: Sat, 12 Nov 2022 14:13:40 +0100 Subject: [PATCH] LibThreading: Overhaul thread behavior with ThreadState This replaces all state-related variables with a single ThreadState. These are simplified over what the Kernel has, but capture all userspace-available thread state. Locking the state behind an atomic and using proper atomic operations also gets rid of quite some deadlocks and race conditions that have existed around m_tid and others beforehand. In terms of behavior, this introduces the following changes: - All thread state mishandling (e.g. joining a detached thread) crashes the program. Mishandling thread state is a severe kind of concurrency bug that might also be indeterministic, so letting it silently disappear with the return value of pthread_ APIs is a bad idea. The thread state can always be checked beforehand to ensure that no crash happens. - Destructing a still-running thread will crash in AK/Function, so the Thread destructor issues its own warning for debugging purposes. - Thread issues warnings before crashes in many places to aid concurrency debugging (the most difficult kind of debugging). - Joining dead but not detached threads is legal, as per POSIX APIs. - The thread ID is never reset to 0 after the thread has been started and subsequently been assigned a valid thread ID. The thread's exit state is still obtainable. - Detaching threads that are about to exit is considered a programming bug and will often (not always, as we can't catch all execution sequences involved in such a situation) crash the program on purpose. If you want to detach a thread that will definitely exit on its own, you have to prevent it from exiting before detach() was called (e.g. with an "exit requested" flag). --- Userland/Libraries/LibThreading/Thread.cpp | 61 ++++++++++++++++++---- Userland/Libraries/LibThreading/Thread.h | 20 +++++-- 2 files changed, 68 insertions(+), 13 deletions(-) diff --git a/Userland/Libraries/LibThreading/Thread.cpp b/Userland/Libraries/LibThreading/Thread.cpp index 632c653ed85..15dcd9ec399 100644 --- a/Userland/Libraries/LibThreading/Thread.cpp +++ b/Userland/Libraries/LibThreading/Thread.cpp @@ -25,10 +25,12 @@ Thread::Thread(Function action, StringView thread_name) Thread::~Thread() { - if (m_tid && !m_detached) { - dbgln("Destroying thread \"{}\"({}) while it is still running!", m_thread_name, m_tid); + if (needs_to_be_joined()) { + dbgln("Destroying {} while it is still running undetached!", *this); [[maybe_unused]] auto res = join(); } + if (m_state == ThreadState::Detached) + dbgln("Bug! {} in state {} is being destroyed; AK/Function will crash shortly!", *this, m_state.load()); } ErrorOr Thread::set_priority(int priority) @@ -56,17 +58,52 @@ DeprecatedString Thread::thread_name() const { return m_thread_name; } pthread_t Thread::tid() const { return m_tid; } -bool Thread::is_started() const { return m_started; } +ThreadState Thread::state() const { return m_state; } + +bool Thread::is_started() const { return m_state != ThreadState::Startable; } + +bool Threading::Thread::needs_to_be_joined() const +{ + auto state = m_state.load(); + return state == ThreadState::Running || state == ThreadState::Exited; +} + +bool Threading::Thread::has_exited() const +{ + auto state = m_state.load(); + return state == ThreadState::Joined || state == ThreadState::Exited || state == ThreadState::DetachedExited; +} void Thread::start() { + VERIFY(!is_started()); + + // Set this first so that the other thread starts out seeing m_state == Running. + m_state = Threading::ThreadState::Running; + int rc = pthread_create( &m_tid, + // FIXME: Use pthread_attr_t to start a thread detached if that was requested by the user before the call to start(). nullptr, [](void* arg) -> void* { Thread* self = static_cast(arg); auto exit_code = self->m_action(); - self->m_tid = 0; + + auto expected = Threading::ThreadState::Running; + // This code might race with a call to detach(). + if (!self->m_state.compare_exchange_strong(expected, Threading::ThreadState::Exited)) { + // If the original state was Detached, we need to set to DetachedExited instead. + if (expected == Threading::ThreadState::Detached) { + if (!self->m_state.compare_exchange_strong(expected, Threading::ThreadState::DetachedExited)) { + dbgln("Thread logic bug: Found thread state {} while trying to set ExitedDetached state!", expected); + VERIFY_NOT_REACHED(); + } + } else { + dbgln("Thread logic bug: Found thread state {} while trying to set Exited state!", expected); + VERIFY_NOT_REACHED(); + } + } + return reinterpret_cast(exit_code); }, static_cast(this)); @@ -78,18 +115,24 @@ void Thread::start() VERIFY(rc == 0); } #endif - dbgln("Started thread \"{}\", tid = {}", m_thread_name, m_tid); - m_started = true; + dbgln("Started {}", *this); } void Thread::detach() { - VERIFY(!m_detached); + auto expected = Threading::ThreadState::Running; + // This code might race with the other thread exiting. + if (!m_state.compare_exchange_strong(expected, Threading::ThreadState::Detached)) { + // Always report a precise error before crashing. These kinds of bugs are hard to reproduce. + if (expected == Threading::ThreadState::Exited) + dbgln("Thread logic bug: {} is being detached after having exited!", this); + else + dbgln("Thread logic bug: trying to detach {} which is not in the Started state, but state {}!", this, expected); + VERIFY_NOT_REACHED(); + } int rc = pthread_detach(m_tid); VERIFY(rc == 0); - - m_detached = true; } } diff --git a/Userland/Libraries/LibThreading/Thread.h b/Userland/Libraries/LibThreading/Thread.h index bb06473a6e1..7dba0ded820 100644 --- a/Userland/Libraries/LibThreading/Thread.h +++ b/Userland/Libraries/LibThreading/Thread.h @@ -7,6 +7,7 @@ #pragma once +#include #include #include #include @@ -49,35 +50,46 @@ public: ErrorOr set_priority(int priority); ErrorOr get_priority() const; + // Only callable in the Startable state. void start(); + // Only callable in the Running state. void detach(); + // Only callable in the Running or Exited states. template Result join(); DeprecatedString thread_name() const; pthread_t tid() const; + ThreadState state() const; bool is_started() const; + bool needs_to_be_joined() const; + bool has_exited() const; private: explicit Thread(Function action, StringView thread_name = {}); Function m_action; pthread_t m_tid { 0 }; DeprecatedString m_thread_name; - bool m_detached { false }; - bool m_started { false }; + Atomic m_state { ThreadState::Startable }; }; template Result Thread::join() { + VERIFY(needs_to_be_joined()); + void* thread_return = nullptr; int rc = pthread_join(m_tid, &thread_return); if (rc != 0) { return ThreadError { rc }; } - m_tid = 0; + // The other thread has now stopped running, so a TOCTOU bug is not possible. + // (If you call join from two different threads, you're doing something *very* wrong anyways.) + VERIFY(m_state == ThreadState::Exited); + m_state = ThreadState::Joined; + if constexpr (IsVoid) return {}; else @@ -98,7 +110,7 @@ template<> struct AK::Formatter : AK::Formatter { ErrorOr format(FormatBuilder& builder, Threading::ThreadState state) { - String name = ""; + DeprecatedString name = ""; switch (state) { case Threading::ThreadState::Detached: name = "Detached";