From 50e8e92f0bf08725127db1ca899911fac1493508 Mon Sep 17 00:00:00 2001 From: tomaszduda23 Date: Thu, 22 Dec 2022 16:51:24 +0900 Subject: [PATCH] Fix race condition in web_server scheduler on ESP32 (#3951) --- esphome/components/web_server/web_server.cpp | 71 +++++++++++++++----- esphome/components/web_server/web_server.h | 13 +++- 2 files changed, 64 insertions(+), 20 deletions(-) diff --git a/esphome/components/web_server/web_server.cpp b/esphome/components/web_server/web_server.cpp index 30ac959e43..1f043b43fc 100644 --- a/esphome/components/web_server/web_server.cpp +++ b/esphome/components/web_server/web_server.cpp @@ -83,6 +83,13 @@ UrlMatch match_url(const std::string &url, bool only_domain = false) { return match; } +WebServer::WebServer(web_server_base::WebServerBase *base) + : base_(base), entities_iterator_(ListEntitiesIterator(this)) { +#ifdef USE_ESP32 + to_schedule_lock_ = xSemaphoreCreateMutex(); +#endif +} + void WebServer::set_css_url(const char *css_url) { this->css_url_ = css_url; } void WebServer::set_css_include(const char *css_include) { this->css_include_ = css_include; } void WebServer::set_js_url(const char *js_url) { this->js_url_ = js_url; } @@ -120,7 +127,25 @@ void WebServer::setup() { this->set_interval(10000, [this]() { this->events_.send("", "ping", millis(), 30000); }); } -void WebServer::loop() { this->entities_iterator_.advance(); } +void WebServer::loop() { +#ifdef USE_ESP32 + if (xSemaphoreTake(this->to_schedule_lock_, 0L)) { + std::function fn; + if (!to_schedule_.empty()) { + // scheduler execute things out of order which may lead to incorrect state + // this->defer(std::move(to_schedule_.front())); + // let's execute it directly from the loop + fn = std::move(to_schedule_.front()); + to_schedule_.pop_front(); + } + xSemaphoreGive(this->to_schedule_lock_); + if (fn) { + fn(); + } + } +#endif + this->entities_iterator_.advance(); +} void WebServer::dump_config() { ESP_LOGCONFIG(TAG, "Web Server:"); ESP_LOGCONFIG(TAG, " Address: %s:%u", network::get_use_address().c_str(), this->base_->get_port()); @@ -413,13 +438,13 @@ void WebServer::handle_switch_request(AsyncWebServerRequest *request, const UrlM std::string data = this->switch_json(obj, obj->state, DETAIL_STATE); request->send(200, "application/json", data.c_str()); } else if (match.method == "toggle") { - this->defer([obj]() { obj->toggle(); }); + this->schedule_([obj]() { obj->toggle(); }); request->send(200); } else if (match.method == "turn_on") { - this->defer([obj]() { obj->turn_on(); }); + this->schedule_([obj]() { obj->turn_on(); }); request->send(200); } else if (match.method == "turn_off") { - this->defer([obj]() { obj->turn_off(); }); + this->schedule_([obj]() { obj->turn_off(); }); request->send(200); } else { request->send(404); @@ -441,7 +466,7 @@ void WebServer::handle_button_request(AsyncWebServerRequest *request, const UrlM if (obj->get_object_id() != match.id) continue; if (request->method() == HTTP_POST && match.method == "press") { - this->defer([obj]() { obj->press(); }); + this->schedule_([obj]() { obj->press(); }); request->send(200); return; } else { @@ -497,7 +522,7 @@ void WebServer::handle_fan_request(AsyncWebServerRequest *request, const UrlMatc std::string data = this->fan_json(obj, DETAIL_STATE); request->send(200, "application/json", data.c_str()); } else if (match.method == "toggle") { - this->defer([obj]() { obj->toggle().perform(); }); + this->schedule_([obj]() { obj->toggle().perform(); }); request->send(200); } else if (match.method == "turn_on") { auto call = obj->turn_on(); @@ -531,10 +556,10 @@ void WebServer::handle_fan_request(AsyncWebServerRequest *request, const UrlMatc return; } } - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); } else if (match.method == "turn_off") { - this->defer([obj]() { obj->turn_off().perform(); }); + this->schedule_([obj]() { obj->turn_off().perform(); }); request->send(200); } else { request->send(404); @@ -558,7 +583,7 @@ void WebServer::handle_light_request(AsyncWebServerRequest *request, const UrlMa std::string data = this->light_json(obj, DETAIL_STATE); request->send(200, "application/json", data.c_str()); } else if (match.method == "toggle") { - this->defer([obj]() { obj->toggle().perform(); }); + this->schedule_([obj]() { obj->toggle().perform(); }); request->send(200); } else if (match.method == "turn_on") { auto call = obj->turn_on(); @@ -590,7 +615,7 @@ void WebServer::handle_light_request(AsyncWebServerRequest *request, const UrlMa call.set_effect(effect); } - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); } else if (match.method == "turn_off") { auto call = obj->turn_off(); @@ -598,7 +623,7 @@ void WebServer::handle_light_request(AsyncWebServerRequest *request, const UrlMa auto length = (uint32_t) request->getParam("transition")->value().toFloat() * 1000; call.set_transition_length(length); } - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); } else { request->send(404); @@ -663,7 +688,7 @@ void WebServer::handle_cover_request(AsyncWebServerRequest *request, const UrlMa if (request->hasParam("tilt")) call.set_tilt(request->getParam("tilt")->value().toFloat()); - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); return; } @@ -708,7 +733,7 @@ void WebServer::handle_number_request(AsyncWebServerRequest *request, const UrlM call.set_value(*value_f); } - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); return; } @@ -765,7 +790,7 @@ void WebServer::handle_select_request(AsyncWebServerRequest *request, const UrlM call.set_option(option.c_str()); // NOLINT(clang-diagnostic-deprecated-declarations) } - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); return; } @@ -833,7 +858,7 @@ void WebServer::handle_climate_request(AsyncWebServerRequest *request, const Url call.set_target_temperature(*value_f); } - this->defer([call]() mutable { call.perform(); }); + this->schedule_([call]() mutable { call.perform(); }); request->send(200); return; } @@ -949,13 +974,13 @@ void WebServer::handle_lock_request(AsyncWebServerRequest *request, const UrlMat std::string data = this->lock_json(obj, obj->state, DETAIL_STATE); request->send(200, "application/json", data.c_str()); } else if (match.method == "lock") { - this->defer([obj]() { obj->lock(); }); + this->schedule_([obj]() { obj->lock(); }); request->send(200); } else if (match.method == "unlock") { - this->defer([obj]() { obj->unlock(); }); + this->schedule_([obj]() { obj->unlock(); }); request->send(200); } else if (match.method == "open") { - this->defer([obj]() { obj->open(); }); + this->schedule_([obj]() { obj->open(); }); request->send(200); } else { request->send(404); @@ -1154,6 +1179,16 @@ void WebServer::handleRequest(AsyncWebServerRequest *request) { bool WebServer::isRequestHandlerTrivial() { return false; } +void WebServer::schedule_(std::function &&f) { +#ifdef USE_ESP32 + xSemaphoreTake(this->to_schedule_lock_, portMAX_DELAY); + to_schedule_.push_back(std::move(f)); + xSemaphoreGive(this->to_schedule_lock_); +#else + this->defer(std::move(f)); +#endif +} + } // namespace web_server } // namespace esphome diff --git a/esphome/components/web_server/web_server.h b/esphome/components/web_server/web_server.h index 78d0597e61..f4122ef754 100644 --- a/esphome/components/web_server/web_server.h +++ b/esphome/components/web_server/web_server.h @@ -9,7 +9,11 @@ #include "esphome/core/controller.h" #include - +#ifdef USE_ESP32 +#include +#include +#include +#endif namespace esphome { namespace web_server { @@ -34,7 +38,7 @@ enum JsonDetail { DETAIL_ALL, DETAIL_STATE }; */ class WebServer : public Controller, public Component, public AsyncWebHandler { public: - WebServer(web_server_base::WebServerBase *base) : base_(base), entities_iterator_(ListEntitiesIterator(this)) {} + WebServer(web_server_base::WebServerBase *base); /** Set the URL to the CSS that's sent to each client. Defaults to * https://esphome.io/_static/webserver-v1.min.css @@ -220,6 +224,7 @@ class WebServer : public Controller, public Component, public AsyncWebHandler { bool isRequestHandlerTrivial() override; protected: + void schedule_(std::function &&f); friend ListEntitiesIterator; web_server_base::WebServerBase *base_; AsyncEventSource events_{"/events"}; @@ -230,6 +235,10 @@ class WebServer : public Controller, public Component, public AsyncWebHandler { const char *js_include_{nullptr}; bool include_internal_{false}; bool allow_ota_{true}; +#ifdef USE_ESP32 + std::deque> to_schedule_; + SemaphoreHandle_t to_schedule_lock_; +#endif }; } // namespace web_server