mirror of
https://github.com/esphome/esphome.git
synced 2025-07-28 14:16:40 +00:00
[scheduler] Fix cancellation of timers with empty string names (#9641)
This commit is contained in:
parent
4a43f922c6
commit
c602f3082e
@ -252,10 +252,10 @@ void Component::defer(const char *name, std::function<void()> &&f) { // NOLINT
|
||||
App.scheduler.set_timeout(this, name, 0, std::move(f));
|
||||
}
|
||||
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
|
||||
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,
|
||||
float backoff_increase_factor) { // NOLINT
|
||||
|
@ -446,7 +446,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
|
||||
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
|
||||
if (name_cstr == nullptr || name_cstr[0] == '\0') {
|
||||
if (name_cstr == nullptr) {
|
||||
return false;
|
||||
}
|
||||
|
||||
|
@ -114,16 +114,17 @@ class Scheduler {
|
||||
name_is_dynamic = false;
|
||||
}
|
||||
|
||||
if (!name || !name[0]) {
|
||||
if (!name) {
|
||||
// nullptr case - no name provided
|
||||
name_.static_name = nullptr;
|
||||
} else if (make_copy) {
|
||||
// Make a copy for dynamic strings
|
||||
// Make a copy for dynamic strings (including empty strings)
|
||||
size_t len = strlen(name);
|
||||
name_.dynamic_name = new char[len + 1];
|
||||
memcpy(name_.dynamic_name, name, len + 1);
|
||||
name_is_dynamic = true;
|
||||
} else {
|
||||
// Use static string directly
|
||||
// Use static string directly (including empty strings)
|
||||
name_.static_name = name;
|
||||
}
|
||||
}
|
||||
|
@ -4,9 +4,7 @@ esphome:
|
||||
priority: -100
|
||||
then:
|
||||
- logger.log: "Starting scheduler string tests"
|
||||
platformio_options:
|
||||
build_flags:
|
||||
- "-DESPHOME_DEBUG_SCHEDULER" # Enable scheduler debug logging
|
||||
debug_scheduler: true # Enable scheduler debug logging
|
||||
|
||||
host:
|
||||
api:
|
||||
@ -32,6 +30,12 @@ globals:
|
||||
- id: results_reported
|
||||
type: bool
|
||||
initial_value: 'false'
|
||||
- id: edge_tests_done
|
||||
type: bool
|
||||
initial_value: 'false'
|
||||
- id: empty_cancel_failed
|
||||
type: bool
|
||||
initial_value: 'false'
|
||||
|
||||
script:
|
||||
- id: test_static_strings
|
||||
@ -147,12 +151,106 @@ script:
|
||||
static TestDynamicDeferComponent test_dynamic_defer_component;
|
||||
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
|
||||
then:
|
||||
- lambda: |-
|
||||
ESP_LOGI("test", "Final results - Timeouts: %d, Intervals: %d",
|
||||
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:
|
||||
- platform: template
|
||||
name: Test Sensor 1
|
||||
@ -189,12 +287,23 @@ interval:
|
||||
- delay: 0.2s
|
||||
- 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
|
||||
- interval: 0.2s
|
||||
then:
|
||||
- if:
|
||||
condition:
|
||||
lambda: 'return id(dynamic_tests_done) && !id(results_reported);'
|
||||
lambda: 'return id(edge_tests_done) && !id(results_reported);'
|
||||
then:
|
||||
- lambda: 'id(results_reported) = true;'
|
||||
- delay: 1s
|
||||
|
@ -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)
|
||||
try:
|
||||
await asyncio.wait_for(test_complete_future, timeout=60.0)
|
||||
await asyncio.wait_for(test_complete_future, timeout=10.0)
|
||||
except asyncio.TimeoutError:
|
||||
# Report how many we got
|
||||
missing_ids = sorted(set(range(1000)) - executed_callbacks)
|
||||
pytest.fail(
|
||||
f"Stress test timed out. Only {len(executed_callbacks)} of "
|
||||
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
|
||||
|
Loading…
x
Reference in New Issue
Block a user