fix: ThreadPool::start must take the lifecycle lock too
abbb2d4guarded 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 by6802328. 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.
This commit is contained in:
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user