search

espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
610 stars 257 forks source link

Deadlock caused by esp_mqtt_client_stop() (IDFGH-1598) #122

Closed thomas0bernard closed 5 years ago

thomas0bernard commented 5 years ago

The main while loop in esp_mqtt_task() blocks at line 987 trying to acquire the API lock semaphore. https://github.com/espressif/esp-mqtt/blob/2fef1a07c528067ca48036d328f1ee6f39b0cbd0/mqtt_client.c#L986-L987

Meanwhile, the semaphore is never released in esp_mqtt_client_stop() because it is blocked by xEventGroupWaitBits() at line 1171. https://github.com/espressif/esp-mqtt/blob/2fef1a07c528067ca48036d328f1ee6f39b0cbd0/mqtt_client.c#L1157-L1180

At this point client->run is already set to false by esp_mqtt_client_stop() so esp_mqtt_task() will exit the while loop at the end of its current repetition and set the eventBits as it should. Releasing the semaphore before calling xEventGroupWaitBits() and acquiring it back immediately after allows the loop to exit and fixes the issue for me.

Correct me if I'm wrong, as xEventGroupWaitBits() is thread-safe I don't see a problem in calling it without the lock. This is my first time opening an issue... Should I submit a PR with my fix or is it generally better to wait for feedback on the issue first?

david-cermak commented 5 years ago

Hi @thomas0bernard

Thanks for reporting this. Yes, indeed, there seems to be a deadlock introduced recently. Please feel free to file a PR with your fix. Thanks!