fix(mqtt): stop a wedged client before deleting it on the dirty-stop path
The stop timeout bounded only the cooperative attempt. Its fallback tears down clients from the calling task, and both helpers skipped the stop entirely when the client reported not-connected — then deleted it anyway. That is the exact shape of the dangerous case: a client whose DISCONNECTED callback has already cleared its connected flag while it sits inside esp_mqtt_client_stop() reports not-connected, so the stop was skipped precisely when it mattered and the object was freed from under a live ESP-MQTT task. Both helpers take a force flag, and only the dirty branch passes it. force is deliberately not gated on connected(), and routes through the existing forceStop() rather than disconnect(), whose wait for the DISCONNECTED event is unbounded. Every other caller — the non-ESP32 release path, the Core-0 cooperative teardown, reconfigure, and the slot-cap path — keeps its current behaviour byte for byte. This does not make the path bounded. esp_mqtt_client_stop() waits on the client's STOPPED_BIT with portMAX_DELAY on the pinned framework, so a client wedged inside mbedTLS can still block the caller. The change trades a free-under-a-live-task for a wait, which is the safer of the two failure modes. Bounding it properly, and the surrounding destroy-while-still-stopping hazard, need the ownership handoff reworked; that is tracked separately.
This commit is contained in:
1 parent
bd91ff80ea
commit
539ae72bca
2 files changed
+27
-9
No files matched your search
@@ -1175,8 +1175,10 @@ void MQTTBridge::LifecycleOps::releaseResources() {
|
||||
if (b->_mqtt_task_handle != nullptr) {
|
||||
vTaskDelete(b->_mqtt_task_handle);
|
||||
}
|
||||
for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) b->teardownSlot(i);
|
||||
b->destroySlotClients();
|
||||
// force: the task is already gone and the client is presumed wedged, so waiting on a
|
||||
// DISCONNECTED event that may never arrive would hang this task (the app loop) forever.
|
||||
for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) b->teardownSlot(i, /*force=*/true);
|
||||
b->destroySlotClients(/*force=*/true);
|
||||
}
|
||||
// Clean path (or a task that acked right at the deadline): the MQTT task
|
||||
// already disconnected/deleted its clients on Core 0 and self-terminated, so
|
||||
@@ -1724,11 +1726,17 @@ void MQTTBridge::releaseSlotAuthToken(int index) {
|
||||
slot.last_token_renewal = 0;
|
||||
}
|
||||
|
||||
void MQTTBridge::destroySlotClients() {
|
||||
void MQTTBridge::destroySlotClients(bool force) {
|
||||
for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) {
|
||||
MQTTSlot& slot = _slots[i];
|
||||
if (slot.client != nullptr) {
|
||||
if (slot.client->connected()) {
|
||||
// force is deliberately NOT gated on connected(). The state it exists for — a client
|
||||
// that already took its DISCONNECTED callback and is now stuck inside
|
||||
// esp_mqtt_client_stop() — reports not-connected, so gating skipped the stop exactly
|
||||
// when it mattered and left the object to be deleted from under a live IDF task.
|
||||
if (force) {
|
||||
slot.client->forceStop();
|
||||
} else if (slot.client->connected()) {
|
||||
slot.client->disconnect();
|
||||
}
|
||||
#ifdef ESP_PLATFORM
|
||||
@@ -1986,12 +1994,18 @@ bool MQTTBridge::setupSlot(int index) {
|
||||
// the client object alive so a subsequent setupSlot() can reuse its mbedTLS
|
||||
// context. This is called both on reconfigure (preset change) and at shutdown;
|
||||
// destruction of the underlying client happens once in destroySlotClients().
|
||||
void MQTTBridge::teardownSlot(int index) {
|
||||
void MQTTBridge::teardownSlot(int index, bool force) {
|
||||
if (index < 0 || index >= RUNTIME_MQTT_SLOTS) return;
|
||||
MQTTSlot& slot = _slots[index];
|
||||
|
||||
if (slot.client && slot.client->connected()) {
|
||||
slot.client->disconnect();
|
||||
// As in destroySlotClients(): force is not gated on connected(), because the wedged
|
||||
// mid-stop state it exists for already reports not-connected.
|
||||
if (slot.client && (force || slot.client->connected())) {
|
||||
if (force) {
|
||||
slot.client->forceStop();
|
||||
} else {
|
||||
slot.client->disconnect();
|
||||
}
|
||||
#ifdef ESP_PLATFORM
|
||||
vTaskDelay(pdMS_TO_TICKS(50));
|
||||
#else
|
||||
|
||||
@@ -442,7 +442,10 @@ private:
|
||||
bool ensureSlotClient(int index); // Allocate this slot's persistent client + callbacks on first use
|
||||
bool ensureSlotAuthToken(int index); // Allocate this slot's JWT token buffer on first token creation
|
||||
void releaseSlotAuthToken(int index);// Free the token buffer (only with the client — see MQTTSlot)
|
||||
void destroySlotClients(); // Delete all persistent clients (shutdown only)
|
||||
// force=true stops each client without waiting for its DISCONNECTED event. Only the
|
||||
// dirty-stop fallback passes it: disconnect()'s wait is unbounded, so a client already
|
||||
// wedged in mbedTLS would block the caller — MyMesh::loop() — indefinitely.
|
||||
void destroySlotClients(bool force = false); // Delete all persistent clients (shutdown only)
|
||||
bool setupSlot(int index); // Configure and connect the slot; false = not activated
|
||||
// Single definition of "this slot holds one of the _max_active_slots positions":
|
||||
// it is enabled and has been through a successful setupSlot(). Startup, the
|
||||
@@ -450,7 +453,8 @@ private:
|
||||
// exceeded by one route while another enforces it.
|
||||
int activatedSlotCount() const;
|
||||
bool canActivateSlot(int index) const;
|
||||
void teardownSlot(int index); // Disconnect the slot's client (keeps the object alive)
|
||||
// force as in destroySlotClients(): skip the unbounded wait, dirty-stop path only.
|
||||
void teardownSlot(int index, bool force = false); // Disconnect the slot's client (keeps the object alive)
|
||||
// Reconnect a slot, starting it instead when the client is stopped (reconnect() is a
|
||||
// no-op on a stopped client). See the definition.
|
||||
void reconnectSlotClient(int index);
|
||||
|
||||
Reference in new issue
Block a user