From 2cfeccfd71256ce36ba44cf948a06b995c294217 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 20:13:21 -0500 Subject: [PATCH] cleanup locking --- esphome/core/scheduler.cpp | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 2954f6d1e6..75b73e910d 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -294,29 +294,27 @@ void HOT Scheduler::call() { } #endif // ESPHOME_DEBUG_SCHEDULER - auto to_remove_was = this->to_remove_; - auto items_was = this->items_.size(); // If we have too many items to remove if (this->to_remove_ > MAX_LOGICALLY_DELETED_ITEMS) { + // We hold the lock for the entire cleanup operation because: + // 1. We're rebuilding the entire items_ list, so we need exclusive access throughout + // 2. Other threads must see either the old state or the new state, not intermediate states + // 3. The operation is already expensive (O(n)), so lock overhead is negligible + // 4. No operations inside can block or take other locks, so no deadlock risk + LockGuard guard{this->lock_}; + std::vector> valid_items; - while (!this->empty_()) { - LockGuard guard{this->lock_}; - auto item = std::move(this->items_[0]); - this->pop_raw_(); - valid_items.push_back(std::move(item)); + + // Move all non-removed items to valid_items + for (auto &item : this->items_) { + if (!item->remove) { + valid_items.push_back(std::move(item)); + } } - { - LockGuard guard{this->lock_}; - this->items_ = std::move(valid_items); - } - - // The following should not happen unless I'm missing something - if (this->to_remove_ != 0) { - ESP_LOGW(TAG, "to_remove_ was %" PRIu32 " now: %" PRIu32 " items where %zu now %zu. Please report this", - to_remove_was, to_remove_, items_was, items_.size()); - this->to_remove_ = 0; - } + // Replace items_ with the filtered list + this->items_ = std::move(valid_items); + this->to_remove_ = 0; } while (!this->empty_()) {