diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index bec185951c0..594679f3438 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -548,6 +548,15 @@ static esp_err_t find_and_unregister_handler(esp_event_remove_handler_context_t* return esp_event_post_to(ctx->loop, esp_event_handler_cleanup, 0, ctx, sizeof(esp_event_remove_handler_context_t), portMAX_DELAY); } +static bool event_loop_has_posts_in_flight(esp_event_loop_instance_t* loop) +{ + portENTER_CRITICAL(&loop->state.lock); + bool posts_in_flight = loop->state.posts_in_flight > 0; + portEXIT_CRITICAL(&loop->state.lock); + + return posts_in_flight; +} + /* ---------------------------- Public API --------------------------------- */ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, esp_event_loop_handle_t* event_loop) @@ -587,6 +596,10 @@ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, es goto on_err; } + loop->state.lock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED; + atomic_init(&loop->state.deleting, false); + loop->state.posts_in_flight = 0; + SLIST_INIT(&(loop->loop_nodes)); // Create the loop task if requested @@ -681,6 +694,8 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick // The event has already been unqueued, so ensure it gets executed. xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + bool exec = false; + // check if the event retrieve from the queue is the internal event that is // triggered when a handler needs to be removed.. if (post.base == esp_event_handler_cleanup) { @@ -694,47 +709,45 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick if (ctx->legacy) { free(ctx->handler_ctx); } - } + } else { + loop->running_task = xTaskGetCurrentTaskHandle(); - loop->running_task = xTaskGetCurrentTaskHandle(); + esp_event_handler_node_t *handler, *temp_handler; + esp_event_loop_node_t *loop_node, *temp_node; + esp_event_base_node_t *base_node, *temp_base; + esp_event_id_node_t *id_node, *temp_id_node; - bool exec = false; - - esp_event_handler_node_t *handler, *temp_handler; - esp_event_loop_node_t *loop_node, *temp_node; - esp_event_base_node_t *base_node, *temp_base; - esp_event_id_node_t *id_node, *temp_id_node; - - SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) { - // Execute loop level handlers - SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { - if (!handler->unregistered) { - handler_execute(loop, handler, post); - exec |= true; - } - } - - SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { - if (base_node->base == post.base) { - // Execute base level handlers - SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { - if (!handler->unregistered) { - handler_execute(loop, handler, post); - exec |= true; - } + SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) { + // Execute loop level handlers + SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; } + } - SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) { - if (id_node->id == post.id) { - // Execute id level handlers - SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { - if (!handler->unregistered) { - handler_execute(loop, handler, post); - exec |= true; - } + SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { + if (base_node->base == post.base) { + // Execute base level handlers + SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } + } + + SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) { + if (id_node->id == post.id) { + // Execute id level handlers + SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } + } + // Skip to next base node + break; } - // Skip to next base node - break; } } } @@ -751,6 +764,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick remaining_ticks -= end - marker; // If the ticks to run expired, return to the caller if (remaining_ticks <= 0) { + loop->running_task = NULL; xSemaphoreGiveRecursive(loop->mutex); break; } else { @@ -762,7 +776,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick xSemaphoreGiveRecursive(loop->mutex); - if (!exec) { + if (!exec && base != esp_event_handler_cleanup) { // No handlers were registered, not even loop/base level handlers ESP_LOGD(TAG, "no handlers have been registered for event %s:%"PRIu32" posted to loop %p", base, id, event_loop); } @@ -779,7 +793,15 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop) esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; SemaphoreHandle_t loop_mutex = loop->mutex; - xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + xSemaphoreTakeRecursive(loop_mutex, portMAX_DELAY); + + atomic_store(&loop->state.deleting, true); + while (event_loop_has_posts_in_flight(loop)) { + xSemaphoreGiveRecursive(loop_mutex); + // Wait for the posts in flight to finish + vTaskDelay(1); + xSemaphoreTakeRecursive(loop_mutex, portMAX_DELAY); + } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING portENTER_CRITICAL(&s_event_loops_spinlock); @@ -808,6 +830,12 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop) // Drop existing posts on the queue esp_event_post_instance_t post; while (xQueueReceive(loop->queue, &post, 0) == pdTRUE) { + if (post.base == esp_event_handler_cleanup) { + esp_event_remove_handler_context_t* ctx = (esp_event_remove_handler_context_t*)post.data.ptr; + if (ctx->legacy) { + free(ctx->handler_ctx); + } + } post_instance_delete(&post); } @@ -922,12 +950,26 @@ esp_err_t esp_event_handler_unregister_with_internal(esp_event_loop_handle_t eve /* remove the handler if the mutex is taken successfully. * otherwise it will be removed from the list later */ esp_err_t res = ESP_FAIL; - if (xSemaphoreTake(loop->mutex, 0) == pdTRUE) { - res = loop_remove_handler(&remove_handler_ctx); - xSemaphoreGive(loop->mutex); + if (xSemaphoreTakeRecursive(loop->mutex, 0) == pdTRUE) { + /* We got the mutex. Check whether we are currently inside a handler + * callback for this loop (running_task is set while handler_execute() + * is active). If we are, we MUST NOT free the handler node immediately + * because handler_execute() will still write to handler->invoked / + * handler->time (profiling) after the callback returns. Use the + * deferred cleanup-event path instead so the node is only freed once + * the current dispatch iteration has fully completed. */ + if (loop->running_task == xTaskGetCurrentTaskHandle()) { + res = find_and_unregister_handler(&remove_handler_ctx); + } else { + res = loop_remove_handler(&remove_handler_ctx); + } + xSemaphoreGiveRecursive(loop->mutex); } else { + /* Another task holds the mutex (e.g. the loop task is dispatching an + * event). Wait until it is released; by that point running_task will + * have been cleared, so direct removal is safe. */ xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); - res = find_and_unregister_handler(&remove_handler_ctx); + res = loop_remove_handler(&remove_handler_ctx); xSemaphoreGiveRecursive(loop->mutex); } @@ -965,15 +1007,25 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; + portENTER_CRITICAL(&loop->state.lock); + if (atomic_load(&loop->state.deleting)) { + portEXIT_CRITICAL(&loop->state.lock); + return ESP_ERR_INVALID_STATE; + } + loop->state.posts_in_flight++; + portEXIT_CRITICAL(&loop->state.lock); + esp_event_post_instance_t post; memset((void*)(&post), 0, sizeof(post)); + esp_err_t err = ESP_OK; if (event_data != NULL && event_data_size != 0) { #if CONFIG_ESP_EVENT_POST_FROM_ISR if (event_data_size > sizeof(post.data.val)) { post.data.ptr = calloc(1, event_data_size); if (post.data.ptr == NULL) { - return ESP_ERR_NO_MEM; + err = ESP_ERR_NO_MEM; + goto on_err; } post.data_allocated = true; memcpy(post.data.ptr, event_data, event_data_size); @@ -986,7 +1038,8 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t void* event_data_copy = esp_event_calloc(1, event_data_size); if (event_data_copy == NULL) { - return ESP_ERR_NO_MEM; + err = ESP_ERR_NO_MEM; + goto on_err; } memcpy(event_data_copy, event_data, event_data_size); @@ -1028,14 +1081,20 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_fetch_add(&loop->events_dropped, 1); #endif - return ESP_ERR_TIMEOUT; + err = ESP_ERR_TIMEOUT; + goto on_err; } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_fetch_add(&loop->events_received, 1); #endif - return ESP_OK; +on_err: + portENTER_CRITICAL(&loop->state.lock); + loop->state.posts_in_flight--; + portEXIT_CRITICAL(&loop->state.lock); + + return err; } #if CONFIG_ESP_EVENT_POST_FROM_ISR @@ -1050,6 +1109,10 @@ esp_err_t esp_event_isr_post_to(esp_event_loop_handle_t event_loop, esp_event_ba esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; + if (atomic_load(&loop->state.deleting)) { + return ESP_ERR_INVALID_STATE; + } + esp_event_post_instance_t post; memset((void*)(&post), 0, sizeof(post)); @@ -1152,7 +1215,7 @@ esp_err_t esp_event_dump(FILE* file) portEXIT_CRITICAL(&s_event_loops_spinlock); // Print the contents of the buffer to the file - fprintf(file, buf); + fprintf(file, "%s", buf); // Free the allocated buffer free(buf); diff --git a/components/esp_event/esp_event_private.c b/components/esp_event/esp_event_private.c index 773909bc42f..1f09e53dda8 100644 --- a/components/esp_event/esp_event_private.c +++ b/components/esp_event/esp_event_private.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2018-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2018-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -20,6 +20,8 @@ bool esp_event_is_handler_registered(esp_event_loop_handle_t event_loop, esp_eve esp_event_id_node_t* id_node; esp_event_handler_node_t* handler; + xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) { SLIST_FOREACH(handler, &(loop_node->handlers), next) { if (event_base == ESP_EVENT_ANY_BASE && event_id == ESP_EVENT_ANY_ID && handler->handler_ctx->handler == event_handler) { @@ -52,6 +54,6 @@ bool esp_event_is_handler_registered(esp_event_loop_handle_t event_loop, esp_eve } out: - xSemaphoreGive(loop->mutex); + xSemaphoreGiveRecursive(loop->mutex); return result; } diff --git a/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp b/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp index 692edd08505..b74e3f8ec6e 100644 --- a/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp +++ b/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp @@ -17,6 +17,7 @@ extern "C" { #include "Mocktask.h" #include "Mockqueue.h" +#include "Mockportmacro.h" } namespace { @@ -102,6 +103,8 @@ TEST_CASE("test esp_event_loop_create no_task(void)") xQueueTakeMutexRecursive_IgnoreAndReturn(0); xQueueGiveMutexRecursive_IgnoreAndReturn(0); xQueueReceive_IgnoreAndReturn(0); + vPortEnterCritical_Ignore(); + vPortExitCritical_Ignore(); esp_event_loop_handle_t loop = nullptr; esp_event_loop_args_t loop_args = test_event_get_default_loop_args(); @@ -115,6 +118,8 @@ TEST_CASE("test esp_event_loop_create no_task(void)") xQueueReceive_StopIgnore(); xQueueTakeMutexRecursive_StopIgnore(); xQueueGiveMutexRecursive_StopIgnore(); + vPortEnterCritical_StopIgnore(); + vPortExitCritical_StopIgnore(); } TEST_CASE("test esp_event_loop_create with_task(void)") @@ -125,6 +130,8 @@ TEST_CASE("test esp_event_loop_create with_task(void)") xQueueTakeMutexRecursive_IgnoreAndReturn(0); xQueueGiveMutexRecursive_IgnoreAndReturn(0); xQueueReceive_IgnoreAndReturn(0); + vPortEnterCritical_Ignore(); + vPortExitCritical_Ignore(); esp_event_loop_handle_t loop = nullptr; esp_event_loop_args_t loop_args = test_event_get_default_loop_args(); @@ -138,6 +145,8 @@ TEST_CASE("test esp_event_loop_create with_task(void)") xQueueReceive_StopIgnore(); xQueueTakeMutexRecursive_StopIgnore(); xQueueGiveMutexRecursive_StopIgnore(); + vPortEnterCritical_StopIgnore(); + vPortExitCritical_StopIgnore(); } TEST_CASE("registering with ANY_BASE but specific ID fails") diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index 85d8b6d58fb..7ff60828024 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2018-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2018-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -66,6 +66,12 @@ typedef struct esp_event_loop_node { typedef SLIST_HEAD(esp_event_loop_nodes, esp_event_loop_node) esp_event_loop_nodes_t; +typedef struct esp_event_loop_state { + portMUX_TYPE lock; /**< spinlock protecting deleting and posts_in_flight */ + atomic_bool deleting; /**< true when loop deletion has started; read lock-free from ISR */ + uint32_t posts_in_flight; /**< task-context posts that passed the post-entry gate */ +} esp_event_loop_state_t; + /// Event loop typedef struct esp_event_loop_instance { const char* name; /**< name of this event loop */ @@ -76,6 +82,7 @@ typedef struct esp_event_loop_instance { SemaphoreHandle_t mutex; /**< mutex for updating the events linked list */ esp_event_loop_nodes_t loop_nodes; /**< set of linked lists containing the registered handlers for the loop */ + esp_event_loop_state_t state; /**< loop deletion and post-entry state */ #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_uint_least32_t events_received; /**< number of events successfully posted to the loop */ atomic_uint_least32_t events_dropped; /**< number of events dropped due to queue being full */ diff --git a/components/esp_event/test_apps/main/test_event_common.cpp b/components/esp_event/test_apps/main/test_event_common.cpp index cc3d665ba4a..da509cf1ceb 100644 --- a/components/esp_event/test_apps/main/test_event_common.cpp +++ b/components/esp_event/test_apps/main/test_event_common.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -183,6 +183,30 @@ TEST_CASE("registering event twice with same handler yields updated handler arg" TEST_ASSERT_EQUAL(1, count_second); } +static void self_unregister_legacy_handler(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data) +{ + esp_event_loop_handle_t loop = (esp_event_loop_handle_t) event_handler_arg; + TEST_ESP_OK(esp_event_handler_unregister_with(loop, event_base, event_id, self_unregister_legacy_handler)); +} + +/* A legacy handler unregistering itself defers removal via an internal cleanup event + * left on the queue. Deleting the loop must drain it without leaking. */ +TEST_CASE("deleting loop with queued legacy cleanup event does not leak", "[event][linux]") +{ + EV_LoopFix loop_fix; + + TEST_ESP_OK(esp_event_handler_register_with(loop_fix.loop, + s_test_base1, + TEST_EVENT_BASE1_EV1, + self_unregister_legacy_handler, + loop_fix.loop)); + + TEST_ESP_OK(esp_event_post_to(loop_fix.loop, s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, portMAX_DELAY)); + + /* ZERO_DELAY runs a single event, leaving the deferred cleanup event queued. */ + TEST_ESP_OK(esp_event_loop_run(loop_fix.loop, ZERO_DELAY)); +} + TEST_CASE("registering event handler instance twice works", "[event][linux]") { EV_LoopFix loop_fix;