From 1d38498ca7c27609dd166610397909cdcad8d174 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Mon, 15 Jun 2026 17:05:50 -0400 Subject: [PATCH] [openthread] Fix InstanceLock releasing the lock twice on try_acquire (#16980) --- esphome/components/openthread/openthread.cpp | 2 +- esphome/components/openthread/openthread.h | 23 +++++++++++++++---- .../components/openthread/openthread_esp.cpp | 17 +++++++------- .../openthread_info_text_sensor.h | 2 +- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/esphome/components/openthread/openthread.cpp b/esphome/components/openthread/openthread.cpp index bf14514636..c8ffc02131 100644 --- a/esphome/components/openthread/openthread.cpp +++ b/esphome/components/openthread/openthread.cpp @@ -227,7 +227,7 @@ bool OpenThreadComponent::teardown() { ESP_LOGW(TAG, "Failed to acquire OpenThread lock during teardown, leaking memory"); return true; } - otInstance *instance = lock->get_instance(); + otInstance *instance = lock.get_instance(); otSrpClientClearHostAndServices(instance); otSrpClientBuffersFreeAllServices(instance); global_openthread_component = nullptr; diff --git a/esphome/components/openthread/openthread.h b/esphome/components/openthread/openthread.h index 5898492a50..96f1abdb92 100644 --- a/esphome/components/openthread/openthread.h +++ b/esphome/components/openthread/openthread.h @@ -86,19 +86,32 @@ class OpenThreadSrpComponent : public Component { void *pool_alloc_(size_t size); }; +// RAII guard for the OpenThread API lock. Modeled on std::unique_lock: the +// guard may or may not own the lock (try_acquire can fail), so check it with +// operator bool before use. Non-copyable and non-movable: the factories return +// by value via guaranteed copy elision, so a guard is never duplicated and the +// lock is released exactly once, when the owning guard goes out of scope. class InstanceLock { public: - static std::optional try_acquire(int delay); + // May fail to acquire within delay ms; check the returned guard with operator bool. + static InstanceLock try_acquire(int delay); + // Blocks until the lock is held. static InstanceLock acquire(); + InstanceLock(const InstanceLock &) = delete; + InstanceLock(InstanceLock &&) = delete; + InstanceLock &operator=(const InstanceLock &) = delete; + InstanceLock &operator=(InstanceLock &&) = delete; ~InstanceLock(); - // Returns the global openthread instance guarded by this lock + explicit operator bool() const { return this->owns_; } + + // Returns the global openthread instance. Only valid on an owning guard + // (operator bool is true); the instance must not be used without the lock held. otInstance *get_instance(); private: - // Use a private constructor in order to force the handling - // of acquisition failure - InstanceLock() {} + explicit InstanceLock(bool owns) : owns_(owns) {} + bool owns_; }; } // namespace esphome::openthread diff --git a/esphome/components/openthread/openthread_esp.cpp b/esphome/components/openthread/openthread_esp.cpp index cf1288d90c..4d88cbd226 100644 --- a/esphome/components/openthread/openthread_esp.cpp +++ b/esphome/components/openthread/openthread_esp.cpp @@ -216,14 +216,11 @@ network::IPAddresses OpenThreadComponent::get_ip_addresses() { // not thread safe, only use in read-only use cases otInstance *OpenThreadComponent::get_openthread_instance_() { return esp_openthread_get_instance(); } -std::optional InstanceLock::try_acquire(int delay) { +InstanceLock InstanceLock::try_acquire(int delay) { if (!global_openthread_component->is_lock_initialized()) { - return {}; + return InstanceLock(false); } - if (esp_openthread_lock_acquire(delay)) { - return InstanceLock(); - } - return {}; + return InstanceLock(esp_openthread_lock_acquire(delay)); } InstanceLock InstanceLock::acquire() { @@ -242,12 +239,16 @@ InstanceLock InstanceLock::acquire() { while (!esp_openthread_lock_acquire(100)) { esp_task_wdt_reset(); } - return InstanceLock(); + return InstanceLock(true); } otInstance *InstanceLock::get_instance() { return esp_openthread_get_instance(); } -InstanceLock::~InstanceLock() { esp_openthread_lock_release(); } +InstanceLock::~InstanceLock() { + if (this->owns_) { + esp_openthread_lock_release(); + } +} } // namespace esphome::openthread #endif diff --git a/esphome/components/openthread_info/openthread_info_text_sensor.h b/esphome/components/openthread_info/openthread_info_text_sensor.h index 10e83281f0..ef7c5cc8e9 100644 --- a/esphome/components/openthread_info/openthread_info_text_sensor.h +++ b/esphome/components/openthread_info/openthread_info_text_sensor.h @@ -17,7 +17,7 @@ class OpenThreadInstancePollingComponent : public PollingComponent { return; } - this->update_instance(lock->get_instance()); + this->update_instance(lock.get_instance()); } float get_setup_priority() const override { return setup_priority::AFTER_WIFI; }