From 771b9f85938dbc1ac449ed4a730fa20ec298609e Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 6 Aug 2026 22:53:02 +0200 Subject: [PATCH] fix: ThreadPool::start must take the lifecycle lock too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit abbb2d4 guarded submit() against stop() with a shared/exclusive lock, because stop() ends with queues_.clear() and submit() indexes queues_. It missed the other writer: start() rebuilds the same vector and took no lock at all. A submission can genuinely land while a pool is inside start(). A network starts its nodes one at a time, and a node already started fires into the next one's channel, whose push callback submits. ThreadSanitizer reports it as the read at scheduler.hpp:114 against the write at :59, and the consequence is worse than a torn read: push_back can reallocate the vector under a reader that has already indexed it. Surfaced by 6802328. Firing push_callback_ unconditionally is correct — the argument in that commit holds — and it makes callbacks frequent enough during startup to hit this window. It went from unobserved to 4 races in one run of the unit suite. Holding the lock across the thread spawn is safe: all queues are constructed before any worker starts, and worker_loop never takes the lifecycle lock, so there is nothing for it to deadlock against. Verified with -DKPN_SANITIZER=thread: 4 races in one run of five before, 0 across five unit runs and two stress runs after. 148/148. --- include/kpn/scheduler.hpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/include/kpn/scheduler.hpp b/include/kpn/scheduler.hpp index 54bbb5b..d11d775 100644 --- a/include/kpn/scheduler.hpp +++ b/include/kpn/scheduler.hpp @@ -53,6 +53,19 @@ public: } void start() override { + // Under the lifecycle lock for the same reason stop() is: submit() + // reads queues_ and this rebuilds it. A network starts its nodes one at + // a time, and a node already started fires into the next one's channel, + // whose push callback submits — so a submission can genuinely land + // while another pool is still inside start(). ThreadSanitizer reports + // it as a read at submit() against this write, and the consequence is + // worse than a torn read: push_back can reallocate the vector under a + // reader that has already indexed it. + // + // Queues are all constructed before any worker is spawned, which is + // what keeps worker_loop's own queues_[id] out of this — it never takes + // the lock, so holding it across the spawn cannot deadlock. + std::unique_lock lk(lifecycle_mx_); stopped_.store(false, std::memory_order_relaxed); queues_.clear(); for (std::size_t i = 0; i < thread_count_; ++i)