From 020b295f06be92c2ced91ef9206592450632e686 Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Tue, 29 Jan 2019 10:52:53 +0800 Subject: [PATCH 1/3] esp_event: fix memory leaks Closes https://github.com/espressif/esp-idf/issues/2886 --- components/esp_event/esp_event.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 8c5c0570e..149f36430 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -528,6 +528,8 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick exec |= true; } + post_instance_delete(&post); + if (ticks_to_run != portMAX_DELAY) { end = xTaskGetTickCount(); remaining_ticks -= end - marker; @@ -559,10 +561,14 @@ 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; +#ifdef CONFIG_EVENT_LOOP_PROFILING + SemaphoreHandle_t loop_profiling_mutex = loop->profiling_mutex; +#endif xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); #ifdef CONFIG_EVENT_LOOP_PROFILING + xSemaphoreTakeRecursive(loop->profiling_mutex, portMAX_DELAY); portENTER_CRITICAL(&s_event_loops_spinlock); SLIST_REMOVE(&s_event_loops, loop, esp_event_loop_instance, loop_entry); portEXIT_CRITICAL(&s_event_loops_spinlock); @@ -588,6 +594,10 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop) free(loop); // Free loop mutex before deleting xSemaphoreGiveRecursive(loop_mutex); +#ifdef CONFIG_EVENT_LOOP_PROFILING + xSemaphoreGiveRecursive(loop_profiling_mutex); + vSemaphoreDelete(loop_profiling_mutex); +#endif vSemaphoreDelete(loop_mutex); ESP_LOGD(TAG, "deleted loop %p", (void*) event_loop); From f49f5ff35af6da251483d07600fd0e4f22e9999a Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Tue, 29 Jan 2019 10:53:26 +0800 Subject: [PATCH 2/3] esp_event: fix post data type inconsistency --- components/esp_event/esp_event.c | 2 +- components/esp_event/private_include/esp_event_internal.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 149f36430..eba9bcc67 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -286,7 +286,7 @@ static esp_event_base_instance_t* loop_find_event_base_instance(esp_event_loop_i // Functions that operate on post instance static esp_err_t post_instance_create(esp_event_base_t event_base, int32_t event_id, void* event_data, int32_t event_data_size, esp_event_post_instance_t* post) { - void** event_data_copy = NULL; + void* event_data_copy = NULL; // Make persistent copy of event data on heap. if (event_data != NULL && event_data_size != 0) { diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index 28cd88203..8b29c8628 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -90,7 +90,7 @@ typedef struct esp_event_loop_instance { typedef struct esp_event_post_instance { esp_event_base_t base; /**< the event base */ int32_t id; /**< the event id */ - void** data; /**< data associated with the event */ + void* data; /**< data associated with the event */ } esp_event_post_instance_t; #ifdef __cplusplus From eae2baa0f17b3392a57f70de1f7d59339c0930aa Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Wed, 30 Jan 2019 17:14:01 +0800 Subject: [PATCH 3/3] esp_event: detect leaks in unit test --- components/esp_event/test/test_event.c | 29 +++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/components/esp_event/test/test_event.c b/components/esp_event/test/test_event.c index b69476ba0..fc7f0781d 100644 --- a/components/esp_event/test/test_event.c +++ b/components/esp_event/test/test_event.c @@ -27,14 +27,19 @@ static const char* TAG = "test_event"; #define TEST_CONFIG_WAIT_MULTIPLIER 5 +// The initial logging "initializing test" is to ensure mutex allocation is not counted against memory not being freed +// during teardown. #define TEST_SETUP() \ + ESP_LOGI(TAG, "initializing test"); \ + size_t free_mem_before = heap_caps_get_free_size(MALLOC_CAP_DEFAULT); \ test_setup(); \ s_test_core_id = xPortGetCoreID(); \ - s_test_priority = uxTaskPriorityGet(NULL); \ + s_test_priority = uxTaskPriorityGet(NULL); #define TEST_TEARDOWN() \ test_teardown(); \ - vTaskDelay(pdMS_TO_TICKS(CONFIG_INT_WDT_TIMEOUT_MS * TEST_CONFIG_WAIT_MULTIPLIER)); + vTaskDelay(pdMS_TO_TICKS(CONFIG_INT_WDT_TIMEOUT_MS * TEST_CONFIG_WAIT_MULTIPLIER)); \ + TEST_ASSERT_EQUAL(free_mem_before, heap_caps_get_free_size(MALLOC_CAP_DEFAULT)); typedef struct { void* data; @@ -679,6 +684,10 @@ static void loop_run_task(void* args) static void performance_test(bool dedicated_task) { + // rand() seems to do a one-time allocation. Call it here so that the memory it allocates + // is not counted as a leak. + unsigned int _rand __attribute__((unused)) = rand(); + TEST_SETUP(); const char test_base[] = "qwertyuiopasdfghjklzxvbnmmnbvcxzqwertyuiopasdfghjklzxvbnmmnbvcxz"; @@ -775,6 +784,14 @@ static void performance_test(bool dedicated_task) int average = (int) (running_sum / (running_count)); + if (!dedicated_task) { + ((esp_event_loop_instance_t*) loop)->task = mtask; + } + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_delete(loop)); + + TEST_TEARDOWN(); + #ifdef CONFIG_EVENT_LOOP_PROFILING ESP_LOGI(TAG, "events dispatched/second with profiling enabled: %d", average); // Enabling profiling will slow down event dispatch, so the set threshold @@ -786,14 +803,6 @@ static void performance_test(bool dedicated_task) TEST_PERFORMANCE_GREATER_THAN(EVENT_DISPATCH_PSRAM, "%d", average); #endif // CONFIG_SPIRAM_SUPPORT #endif // CONFIG_EVENT_LOOP_PROFILING - - if (!dedicated_task) { - ((esp_event_loop_instance_t*) loop)->task = mtask; - } - - TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_delete(loop)); - - TEST_TEARDOWN(); } TEST_CASE("performance test - dedicated task", "[event]")