Lack of proper cleanup in ArduinoOTA class (missing end() in ~ArduinoOTA)

[cziter15]

Board

Not strictly related to hardware, so I'm skipping this section

Description

I'm the author of ksIotFrameworkLib (iot library for arduino esp), working extensively with various ESP devices. Recently, I encountered a frustrating issue that led to crashes under specific conditions. The problem was challenging to pinpoint due to its random nature.

During the transition between the provisioning app and the regular operational app, I faced random exceptions:

Guru Meditation Error: Core 0 panic'ed (LoadProhibited). Exception was unhandled.

My code is written in modern C++, utilizing unique_ptr as the standard practice. After investigating, I discovered that the root cause of the crash was linked to the mDNS service (an IDF component). Since I use ArduinoOTA without any global instances enabled, this was another area I needed to explore.

And - bingo!

It turns out that the ArduinoOTAClass does not correctly clean up mDNS in its destructor. It should be calling the end() method, but it doesn't. This issue also impacts the ESP32 framework.

Arduino/libraries/ArduinoOTA/ArduinoOTA.cpp

Line 39 in 1a13ab9

ArduinoOTAClass::~ArduinoOTAClass(){

Workaround
Explicitly invoke the end() method in the code that owns ArduinoOTA code before the object is destroyed.

[mcspr]

Does it actually crash for ESP8266? Guru meditation Error: ... is ESP32-specific crash report format

Not sure I got the reasoning, though. ArduinoOTA constructor does not need MDNS, only ::begin() does.
It calls MDNS.start(_hostname) and MDNS.enableArduino(), MDNS.end() closes everything related to MDNS for everyone else.

MDNS.removeService("arduino", "tcp", PORT); // or MDNS.disableArduino();

Might be more appropriate here, replacing current MDNS.end() in the code?
(should we want to continue use global objects at all here)

[cziter15]

Ah, that was my copy-paste mistake. It likely doesn’t crash, but it's not ideal to leave it as is as it may break MDNS functionality. If ArduinoOTA instantiates the MDNS object, it should also release it upon destruction, regardless of its creation method. This behavior is standard practice. Failing to do so would be akin to expecting a file to remain open even after closing Notepad, requiring you to reopen it just to close it properly. That would be both inconsistent and poorly designed. Additionally, there's another dependency: MDNS updates are managed through ArduinoOTA's handle method. There's no clear indication that the user should start manually calling MDNS.update after destruction.