From b7795720b2d3d56d0349e6f0454bb93acbd13e0c Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 18 Jul 2026 10:22:34 -0700 Subject: [PATCH] feat(webconfig): improve request handling and server teardown logic Enhance the WebConfigServer to track in-flight requests, ensuring that the server is only freed once all requests have completed or a hard timeout has been reached. This prevents crashes due to live connections during server deletion. Update route handlers to log requests and manage their lifecycle more effectively, improving stability and performance of the web configuration portal. --- src/helpers/esp32/WebConfigServer.cpp | 68 ++++++++++++++++++++------- src/helpers/esp32/WebConfigServer.h | 17 ++++++- 2 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index f7ee3354..73be678f 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -217,13 +217,27 @@ void WebConfigServer::requestStop() { if (_dns) _dns->stop(); _mode = MODE_OFF; _stopping = true; - // Deleting an AsyncWebServer with live connections is a known crash source; - // give in-flight responses a grace period before freeing. - _delete_at = millis() + 2000; + // Deleting an AsyncWebServer with a live request is a known crash source (the + // route lambdas capture `this`). end() above stops accepting new connections; + // tick() then frees the server once in-flight requests have drained (see + // _inflight), bounded by a settle window and a hard cap. + uint32_t now = millis(); + _delete_at = now + STOP_SETTLE_MS; if (_delete_at == 0) _delete_at = 1; + _delete_deadline = now + STOP_MAX_WAIT_MS; + if (_delete_deadline == 0) _delete_deadline = 1; } void WebConfigServer::finalizeTeardown() { + // Design note: we free the server on stop rather than keeping one persistent + // instance for the firmware lifetime. A persistent server would avoid this + // delete entirely (no UAF window, no start/stop heap churn), but the route + // lambdas capture `this`, so keeping them alive means keeping the whole + // ~10 KB WebConfigServer (the _batch[] array dominates) resident forever on + // any node that ever opened the portal — on top of AsyncTCP's ~16 KB worker + // task, which is never reclaimed either way. That permanent heap cost was + // ruled out (fleet includes tight non-PSRAM boards); instead we free on stop + // and gate the delete on _inflight so a live request is never freed under it. delete _server; _server = NULL; delete _dns; @@ -241,6 +255,8 @@ void WebConfigServer::finalizeTeardown() { } _stopping = false; _delete_at = 0; + _delete_deadline = 0; + _inflight = 0; _reboot_at = 0; _batch_state = BATCH_IDLE; _batch_next = 0; @@ -252,7 +268,14 @@ void WebConfigServer::finalizeTeardown() { void WebConfigServer::tick(uint32_t now) { if (_stopping) { - if (_delete_at && (int32_t)(now - _delete_at) >= 0) finalizeTeardown(); + if (_delete_at) { + // Free once the settle window has passed AND nothing is in flight, or + // unconditionally at the hard cap. The settle window lets any request + // already queued when we closed the listener get counted first. + bool settled = (int32_t)(now - _delete_at) >= 0; + bool capped = (int32_t)(now - _delete_deadline) >= 0; + if ((settled && _inflight <= 0) || capped) finalizeTeardown(); + } return; } if (_mode == MODE_OFF) return; @@ -346,26 +369,37 @@ static void wcLogReq(AsyncWebServerRequest* r) { Serial.printf("WC: http %s %s\n", r->methodToString(), r->url().c_str()); } +// Called at the top of every route handler (async_tcp task). Counts the request +// as in flight until its connection closes, so requestStop() can wait for the +// last response to finish before freeing the server instead of guessing with a +// fixed delay. Responses send `Connection: close`, so onDisconnect fires as soon +// as the reply is delivered — no lingering keep-alive connections to over-count. +void WebConfigServer::beginRequest(AsyncWebServerRequest* req) { + wcLogReq(req); + _inflight++; + req->onDisconnect([this]() { if (_inflight > 0) _inflight--; }); +} + void WebConfigServer::registerRoutes() { - _server->on("/", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleRoot(r); }); - _server->on("/api/status", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleStatus(r); }); - _server->on("/api/presets", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handlePresets(r); }); - _server->on("/api/login", HTTP_POST, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleLogin(r); }, + _server->on("/", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleRoot(r); }); + _server->on("/api/status", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleStatus(r); }); + _server->on("/api/presets", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handlePresets(r); }); + _server->on("/api/login", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleLogin(r); }, NULL, collectBody); - _server->on("/api/logout", HTTP_POST, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleLogout(r); }); + _server->on("/api/logout", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleLogout(r); }); // NB: plain-string routes match sub-paths too ("/api/config" matches // "/api/config/result") and handlers run in registration order, so the more // specific route MUST be registered first or it never fires. This was why // save confirmations were lost: result polls were answered with config JSON. - _server->on("/api/config/result", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleConfigResult(r); }); - _server->on("/api/config", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleConfigGet(r); }); - _server->on("/api/config", HTTP_POST, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleConfigPost(r); }, + _server->on("/api/config/result", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleConfigResult(r); }); + _server->on("/api/config", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleConfigGet(r); }); + _server->on("/api/config", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleConfigPost(r); }, NULL, collectBody); - _server->on("/api/stats", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleStats(r); }); - _server->on("/api/scan", HTTP_GET, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleScan(r); }); - _server->on("/api/reboot", HTTP_POST, [this](AsyncWebServerRequest* r) { wcLogReq(r); handleReboot(r); }); - _server->on("/api/portal/exit", HTTP_POST, [this](AsyncWebServerRequest* r) { wcLogReq(r); handlePortalExit(r); }); - _server->onNotFound([this](AsyncWebServerRequest* r) { wcLogReq(r); handleNotFound(r); }); + _server->on("/api/stats", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleStats(r); }); + _server->on("/api/scan", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleScan(r); }); + _server->on("/api/reboot", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleReboot(r); }); + _server->on("/api/portal/exit", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handlePortalExit(r); }); + _server->onNotFound([this](AsyncWebServerRequest* r) { beginRequest(r); handleNotFound(r); }); } // Accumulate a small JSON body into request->_tempObject (freed automatically diff --git a/src/helpers/esp32/WebConfigServer.h b/src/helpers/esp32/WebConfigServer.h index 79b195c8..cc0d09eb 100644 --- a/src/helpers/esp32/WebConfigServer.h +++ b/src/helpers/esp32/WebConfigServer.h @@ -85,6 +85,13 @@ public: private: static const int MAX_BATCH = 24; static const size_t MAX_BODY = 4096; + // Teardown timing. requestStop() closes the listener immediately; the server + // object is freed once (a) the settle window has elapsed so any request + // already queued on the async_tcp task has been counted, and (b) no request + // is still in flight — but never later than the hard cap, so a stalled client + // can't pin the server (and its heap) open forever. + static const uint32_t STOP_SETTLE_MS = 2000; + static const uint32_t STOP_MAX_WAIT_MS = 10000; enum BatchState : uint8_t { BATCH_IDLE = 0, BATCH_PENDING, BATCH_DONE }; struct BatchEntry { char key[24]; // allowlisted `set` key (echoed back to the UI) @@ -141,14 +148,20 @@ private: uint32_t _diag_last = 0; volatile uint32_t _last_activity = 0; - uint32_t _reboot_at = 0; // 0 = none scheduled - uint32_t _delete_at = 0; // deferred teardown deadline + uint32_t _reboot_at = 0; // 0 = none scheduled + uint32_t _delete_at = 0; // earliest teardown time (settle window ends) + uint32_t _delete_deadline = 0; // hard cap: free even if requests are still in flight + // Requests currently being processed/streamed. Mutated only on the async_tcp + // task (increment in beginRequest, decrement in the per-request onDisconnect), + // read on the loop task in tick(); a 32-bit read is atomic so no lock needed. + volatile int _inflight = 0; volatile uint32_t _stats_wanted_until = 0; uint32_t _stats_built_at = 0; char _stats_json[1024] = {0}; void createServer(); void registerRoutes(); + void beginRequest(AsyncWebServerRequest* req); // log + in-flight tracking void drainBatch(uint32_t now); void finalizeTeardown(); bool checkAuth(AsyncWebServerRequest* req);