From 109eae26a7a62467979c313fbc4268fca78ca8aa Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 20 Jul 2025 12:09:11 -1000 Subject: [PATCH 1/2] [core] Refactor scheduler to eliminate hidden side effects in empty_() method --- esphome/core/scheduler.cpp | 27 +++++++++++++++++++-------- esphome/core/scheduler.h | 19 +++++++------------ 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 7a0c08e1f0..4778ad61a1 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -219,10 +219,16 @@ bool HOT Scheduler::cancel_retry(Component *component, const std::string &name) optional HOT Scheduler::next_schedule_in(uint32_t now) { // IMPORTANT: This method should only be called from the main thread (loop task). - // It calls empty_() and accesses items_[0] without holding a lock, which is only + // It performs cleanup and accesses items_[0] without holding a lock, which is only // safe when called from the main thread. Other threads must not call this method. - if (this->empty_()) + + // Cleanup removed items first + size_t item_count = this->cleanup_(); + + // If no items, return empty optional + if (item_count == 0) return {}; + auto &item = this->items_[0]; // Convert the fresh timestamp from caller (usually Application::loop()) to 64-bit const auto now_64 = this->millis_64_(now); // 'now' from parameter - fresh from caller @@ -281,7 +287,9 @@ void HOT Scheduler::call(uint32_t now) { ESP_LOGD(TAG, "Items: count=%zu, now=%" PRIu64 " (%u, %" PRIu32 ")", this->items_.size(), now_64, this->millis_major_, this->last_millis_); #endif - while (!this->empty_()) { + // Cleanup before debug output + this->cleanup_(); + while (!this->items_.empty()) { std::unique_ptr item; { LockGuard guard{this->lock_}; @@ -332,7 +340,9 @@ void HOT Scheduler::call(uint32_t now) { this->to_remove_ = 0; } - while (!this->empty_()) { + // Cleanup removed items before processing + this->cleanup_(); + while (!this->items_.empty()) { // use scoping to indicate visibility of `item` variable { // Don't copy-by value yet @@ -398,8 +408,8 @@ void HOT Scheduler::process_to_add() { } this->to_add_.clear(); } -void HOT Scheduler::cleanup_() { - // Fast path: if nothing to remove, just return +size_t HOT Scheduler::cleanup_() { + // Fast path: if nothing to remove, just return the current size // Reading to_remove_ without lock is safe because: // 1. We only call this from the main thread during call() // 2. If it's 0, there's definitely nothing to cleanup @@ -407,7 +417,7 @@ void HOT Scheduler::cleanup_() { // 4. Not all platforms support atomics, so we accept this race in favor of performance // 5. The worst case is a one-loop-iteration delay in cleanup, which is harmless if (this->to_remove_ == 0) - return; + return this->items_.size(); // We must hold the lock for the entire cleanup operation because: // 1. We're modifying items_ (via pop_raw_) which requires exclusive access @@ -421,10 +431,11 @@ void HOT Scheduler::cleanup_() { while (!this->items_.empty()) { auto &item = this->items_[0]; if (!item->remove) - return; + break; this->to_remove_--; this->pop_raw_(); } + return this->items_.size(); } void HOT Scheduler::pop_raw_() { std::pop_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 64df2f2bb0..41e9bc9a44 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -57,6 +57,9 @@ class Scheduler { // Calculate when the next scheduled item should run // @param now Fresh timestamp from millis() - must not be stale/cached + // Returns the time in milliseconds until the next scheduled item, or nullopt if no items + // This method performs cleanup of removed items before checking the schedule + // IMPORTANT: This method should only be called from the main thread (loop task). optional next_schedule_in(uint32_t now); // Execute all scheduled items that are ready @@ -146,7 +149,10 @@ class Scheduler { uint32_t delay, std::function func); uint64_t millis_64_(uint32_t now); - void cleanup_(); + // Cleanup logically deleted items from the scheduler + // Returns the number of items remaining after cleanup + // IMPORTANT: This method should only be called from the main thread (loop task). + size_t cleanup_(); void pop_raw_(); private: @@ -190,17 +196,6 @@ class Scheduler { return item->remove || (item->component != nullptr && item->component->is_failed()); } - // Check if the scheduler has no items. - // IMPORTANT: This method should only be called from the main thread (loop task). - // It performs cleanup of removed items and checks if the queue is empty. - // The items_.empty() check at the end is done without a lock for performance, - // which is safe because this is only called from the main thread while other - // threads only add items (never remove them). - bool empty_() { - this->cleanup_(); - return this->items_.empty(); - } - Mutex lock_; std::vector> items_; std::vector> to_add_; From f6b989bd9aa3e3e8f89b5f29e53274b0be856ccd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 20 Jul 2025 12:15:16 -1000 Subject: [PATCH 2/2] cleanup --- esphome/core/scheduler.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 4778ad61a1..05b83a1d0a 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -222,11 +222,8 @@ optional HOT Scheduler::next_schedule_in(uint32_t now) { // It performs cleanup and accesses items_[0] without holding a lock, which is only // safe when called from the main thread. Other threads must not call this method. - // Cleanup removed items first - size_t item_count = this->cleanup_(); - // If no items, return empty optional - if (item_count == 0) + if (this->cleanup_() == 0) return {}; auto &item = this->items_[0];