search

memfault / memfault-firmware-sdk

Memfault SDK for embedded systems. Observability, logging, crash reporting, and OTA all in one service. More information at https://docs.memfault.com.
https://memfault.com
Other
149 stars 75 forks source link

always clean up ESP HTTP client #71

Closed mykmelez closed 5 months ago

mykmelez commented 5 months ago

If a device is connected to a wifi access point, but the AP isn't connected to the internet, then a call to memfault_esp_port_http_client_post_data() will leak memory if data is available to post, because memfault_platform_http_client_destroy()'s call to esp_http_client_close() will fail, so it won't call esp_http_client_cleanup() to free memory associated with the HTTP client.

In my testing on a device that calls memfault_esp_port_http_client_post_data() once per minute, this leaks ~2.5K bytes per invocation. (Eventually the device runs out of heap and crashes.)

Since esp_http_client_cleanup() itself calls esp_http_client_close() (and ignores its return value), it seems like this could be fixed by making memfault_platform_http_client_destroy() call esp_http_client_cleanup() unconditionally, without first calling esp_http_client_close() and checking its return value.

noahp commented 5 months ago

Thanks @mykmelez for chasing this down, this is a great fix! We'll include it in our next SDK release.

noahp commented 5 months ago

Hi @mykmelez , FYI this fix was included in the 1.9.2 release of the SDK: https://github.com/memfault/memfault-firmware-sdk/blob/master/CHANGELOG.md#chart_with_upwards_trend-improvements

Closing the ticket now, let us know if you hit any issue!