[scheduler] Fix cancellation of timers with empty string names (#9641)

This commit is contained in:
J. Nick Koston 2025-07-17 16:20:35 -10:00 committed by GitHub
parent eb8a241a01
commit 158a3b2835
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 123 additions and 12 deletions

View File

@ -255,10 +255,10 @@ void Component::defer(const char *name, std::function<void()> &&f) { // NOLINT
App.scheduler.set_timeout(this, name, 0, std::move(f)); App.scheduler.set_timeout(this, name, 0, std::move(f));
} }
void Component::set_timeout(uint32_t timeout, std::function<void()> &&f) { // NOLINT void Component::set_timeout(uint32_t timeout, std::function<void()> &&f) { // NOLINT
App.scheduler.set_timeout(this, "", timeout, std::move(f)); App.scheduler.set_timeout(this, static_cast<const char *>(nullptr), timeout, std::move(f));
} }
void Component::set_interval(uint32_t interval, std::function<void()> &&f) { // NOLINT void Component::set_interval(uint32_t interval, std::function<void()> &&f) { // NOLINT
App.scheduler.set_interval(this, "", interval, std::move(f)); App.scheduler.set_interval(this, static_cast<const char *>(nullptr), interval, std::move(f));
} }
void Component::set_retry(uint32_t initial_wait_time, uint8_t max_attempts, std::function<RetryResult(uint8_t)> &&f, void Component::set_retry(uint32_t initial_wait_time, uint8_t max_attempts, std::function<RetryResult(uint8_t)> &&f,
float backoff_increase_factor) { // NOLINT float backoff_increase_factor) { // NOLINT

View File

@ -452,7 +452,7 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co
// Helper to cancel items by name - must be called with lock held // Helper to cancel items by name - must be called with lock held
bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) {
// Early return if name is invalid - no items to cancel // Early return if name is invalid - no items to cancel
if (name_cstr == nullptr || name_cstr[0] == '\0') { if (name_cstr == nullptr) {
return false; return false;
} }

View File

@ -121,16 +121,17 @@ class Scheduler {
name_is_dynamic = false; name_is_dynamic = false;
} }
if (!name || !name[0]) { if (!name) {
// nullptr case - no name provided
name_.static_name = nullptr; name_.static_name = nullptr;
} else if (make_copy) { } else if (make_copy) {
// Make a copy for dynamic strings // Make a copy for dynamic strings (including empty strings)
size_t len = strlen(name); size_t len = strlen(name);
name_.dynamic_name = new char[len + 1]; name_.dynamic_name = new char[len + 1];
memcpy(name_.dynamic_name, name, len + 1); memcpy(name_.dynamic_name, name, len + 1);
name_is_dynamic = true; name_is_dynamic = true;
} else { } else {
// Use static string directly // Use static string directly (including empty strings)
name_.static_name = name; name_.static_name = name;
} }
} }

View File

@ -4,9 +4,7 @@ esphome:
priority: -100 priority: -100
then: then:
- logger.log: "Starting scheduler string tests" - logger.log: "Starting scheduler string tests"
platformio_options: debug_scheduler: true # Enable scheduler debug logging
build_flags:
- "-DESPHOME_DEBUG_SCHEDULER" # Enable scheduler debug logging
host: host:
api: api:
@ -32,6 +30,12 @@ globals:
- id: results_reported - id: results_reported
type: bool type: bool
initial_value: 'false' initial_value: 'false'
- id: edge_tests_done
type: bool
initial_value: 'false'
- id: empty_cancel_failed
type: bool
initial_value: 'false'
script: script:
- id: test_static_strings - id: test_static_strings
@ -147,12 +151,106 @@ script:
static TestDynamicDeferComponent test_dynamic_defer_component; static TestDynamicDeferComponent test_dynamic_defer_component;
test_dynamic_defer_component.test_dynamic_defer(); test_dynamic_defer_component.test_dynamic_defer();
- id: test_cancellation_edge_cases
then:
- logger.log: "Testing cancellation edge cases"
- lambda: |-
auto *component1 = id(test_sensor1);
// Use a different component for empty string tests to avoid interference
auto *component2 = id(test_sensor2);
// Test 12: Cancel with empty string - regression test for issue #9599
// First create a timeout with empty name on component2 to avoid interference
App.scheduler.set_timeout(component2, "", 500, []() {
ESP_LOGE("test", "ERROR: Empty name timeout fired - it should have been cancelled!");
id(empty_cancel_failed) = true;
});
// Now cancel it - this should work after our fix
bool cancelled_empty = App.scheduler.cancel_timeout(component2, "");
ESP_LOGI("test", "Cancel empty string result: %s (should be true)", cancelled_empty ? "true" : "false");
if (!cancelled_empty) {
ESP_LOGE("test", "ERROR: Failed to cancel empty string timeout!");
id(empty_cancel_failed) = true;
}
// Test 13: Cancel non-existent timeout
bool cancelled_nonexistent = App.scheduler.cancel_timeout(component1, "does_not_exist");
ESP_LOGI("test", "Cancel non-existent timeout result: %s",
cancelled_nonexistent ? "true (unexpected!)" : "false (expected)");
// Test 14: Multiple timeouts with same name - only last should execute
for (int i = 0; i < 5; i++) {
App.scheduler.set_timeout(component1, "duplicate_timeout", 200 + i*10, [i]() {
ESP_LOGI("test", "Duplicate timeout %d fired", i);
id(timeout_counter) += 1;
});
}
ESP_LOGI("test", "Created 5 timeouts with same name 'duplicate_timeout'");
// Test 15: Multiple intervals with same name - only last should run
for (int i = 0; i < 3; i++) {
App.scheduler.set_interval(component1, "duplicate_interval", 300, [i]() {
ESP_LOGI("test", "Duplicate interval %d fired", i);
id(interval_counter) += 10; // Large increment to detect multiple
// Cancel after first execution
App.scheduler.cancel_interval(id(test_sensor1), "duplicate_interval");
});
}
ESP_LOGI("test", "Created 3 intervals with same name 'duplicate_interval'");
// Test 16: Cancel with nullptr protection (via empty const char*)
const char* null_name = "";
App.scheduler.set_timeout(component2, null_name, 600, []() {
ESP_LOGE("test", "ERROR: Const char* empty timeout fired - should have been cancelled!");
id(empty_cancel_failed) = true;
});
bool cancelled_const_empty = App.scheduler.cancel_timeout(component2, null_name);
ESP_LOGI("test", "Cancel const char* empty result: %s (should be true)",
cancelled_const_empty ? "true" : "false");
if (!cancelled_const_empty) {
ESP_LOGE("test", "ERROR: Failed to cancel const char* empty timeout!");
id(empty_cancel_failed) = true;
}
// Test 17: Rapid create/cancel/create with same name
App.scheduler.set_timeout(component1, "rapid_test", 5000, []() {
ESP_LOGI("test", "First rapid timeout - should not fire");
id(timeout_counter) += 100;
});
App.scheduler.cancel_timeout(component1, "rapid_test");
App.scheduler.set_timeout(component1, "rapid_test", 250, []() {
ESP_LOGI("test", "Second rapid timeout - should fire");
id(timeout_counter) += 1;
});
// Test 18: Cancel all with a specific name (multiple instances)
// Create multiple with same name
App.scheduler.set_timeout(component1, "multi_cancel", 300, []() {
ESP_LOGI("test", "Multi-cancel timeout 1");
});
App.scheduler.set_timeout(component1, "multi_cancel", 350, []() {
ESP_LOGI("test", "Multi-cancel timeout 2");
});
App.scheduler.set_timeout(component1, "multi_cancel", 400, []() {
ESP_LOGI("test", "Multi-cancel timeout 3 - only this should fire");
id(timeout_counter) += 1;
});
// Note: Each set_timeout with same name cancels the previous one automatically
- id: report_results - id: report_results
then: then:
- lambda: |- - lambda: |-
ESP_LOGI("test", "Final results - Timeouts: %d, Intervals: %d", ESP_LOGI("test", "Final results - Timeouts: %d, Intervals: %d",
id(timeout_counter), id(interval_counter)); id(timeout_counter), id(interval_counter));
// Check if empty string cancellation test passed
if (id(empty_cancel_failed)) {
ESP_LOGE("test", "ERROR: Empty string cancellation test FAILED!");
} else {
ESP_LOGI("test", "Empty string cancellation test PASSED");
}
sensor: sensor:
- platform: template - platform: template
name: Test Sensor 1 name: Test Sensor 1
@ -189,12 +287,23 @@ interval:
- delay: 0.2s - delay: 0.2s
- script.execute: test_dynamic_strings - script.execute: test_dynamic_strings
# Run cancellation edge case tests after dynamic tests
- interval: 0.2s
then:
- if:
condition:
lambda: 'return id(dynamic_tests_done) && !id(edge_tests_done);'
then:
- lambda: 'id(edge_tests_done) = true;'
- delay: 0.5s
- script.execute: test_cancellation_edge_cases
# Report results after all tests # Report results after all tests
- interval: 0.2s - interval: 0.2s
then: then:
- if: - if:
condition: condition:
lambda: 'return id(dynamic_tests_done) && !id(results_reported);' lambda: 'return id(edge_tests_done) && !id(results_reported);'
then: then:
- lambda: 'id(results_reported) = true;' - lambda: 'id(results_reported) = true;'
- delay: 1s - delay: 1s

View File

@ -103,13 +103,14 @@ async def test_scheduler_heap_stress(
# Wait for all callbacks to execute (should be quick, but give more time for scheduling) # Wait for all callbacks to execute (should be quick, but give more time for scheduling)
try: try:
await asyncio.wait_for(test_complete_future, timeout=60.0) await asyncio.wait_for(test_complete_future, timeout=10.0)
except TimeoutError: except TimeoutError:
# Report how many we got # Report how many we got
missing_ids = sorted(set(range(1000)) - executed_callbacks)
pytest.fail( pytest.fail(
f"Stress test timed out. Only {len(executed_callbacks)} of " f"Stress test timed out. Only {len(executed_callbacks)} of "
f"1000 callbacks executed. Missing IDs: " f"1000 callbacks executed. Missing IDs: "
f"{sorted(set(range(1000)) - executed_callbacks)[:10]}..." f"{missing_ids[:20]}... (total missing: {len(missing_ids)})"
) )
# Verify all callbacks executed # Verify all callbacks executed