diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index 9c9069238e..804e597b0c 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -222,10 +222,8 @@ void Thread::consider_unblock(time_t now_sec, long now_usec) return; case Thread::Blocked: ASSERT(m_blocker); - if (m_blocker->should_unblock(*this, now_sec, now_usec)) { + if (m_blocker->should_unblock(*this, now_sec, now_usec)) unblock(); - m_blocker = nullptr; - } return; case Thread::Skip1SchedulerPass: set_state(Thread::Skip0SchedulerPasses); diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 68709dd48d..48f70f2797 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -97,7 +97,6 @@ Thread::~Thread() void Thread::unblock() { - m_blocker = nullptr; if (current == this) { set_state(Thread::Running); return; @@ -108,9 +107,11 @@ void Thread::unblock() void Thread::block_helper() { + // This function mostly exists to avoid circular header dependencies. If + // anything needs adding, think carefully about whether it belongs in + // block() instead. Remember that we're unlocking here, so be very careful + // about altering any state once we're unlocked! bool did_unlock = process().big_lock().unlock_if_locked(); - ASSERT(state() == Thread::Running); - set_state(Thread::Blocked); Scheduler::yield(); if (did_unlock) process().big_lock().lock(); @@ -160,8 +161,6 @@ void Thread::finalize() dbgprintf("Finalizing Thread %u in %s(%u)\n", tid(), m_process.name().characters(), pid()); set_state(Thread::State::Dead); - m_blocker = nullptr; - if (this == &m_process.main_thread()) m_process.finalize(); } @@ -539,6 +538,11 @@ bool Thread::is_thread(void* ptr) void Thread::set_state(State new_state) { InterruptDisabler disabler; + if (new_state == Blocked) { + // we should always have an m_blocker while blocked + ASSERT(m_blocker != nullptr); + } + m_state = new_state; if (m_process.pid() != 0) { SchedulerThreadList* list = nullptr; diff --git a/Kernel/Thread.h b/Kernel/Thread.h index de619ba4e4..4af873cd87 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -217,10 +217,19 @@ public: template [[nodiscard]] BlockResult block(Args&& ... args) { - ASSERT(!m_blocker); + // If this is triggered, state has gotten messed: we should never have a + // blocker already set. That means we're re-entering somehow, which is + // bad. + ASSERT(m_blocker == nullptr); + + // We should never be blocking a blocked (or otherwise non-active) thread. + ASSERT(state() == Thread::Running); T t(AK::forward(args)...); m_blocker = &t; - block_helper(); + set_state(Thread::Blocked); + block_helper(); // this will unlock, yield, and eventually unblock us to return here. + ASSERT(state() != Thread::Blocked); + m_blocker = nullptr; if (t.was_interrupted_by_signal()) return BlockResult::InterruptedBySignal; return BlockResult::WokeNormally;