Skip to content

ESPAsyncWebServer on Linux and MacOS hosts#416

Open
MitchBradley wants to merge 5 commits intoESP32Async:mainfrom
MitchBradley:Hosted
Open

ESPAsyncWebServer on Linux and MacOS hosts#416
MitchBradley wants to merge 5 commits intoESP32Async:mainfrom
MitchBradley:Hosted

Conversation

@MitchBradley
Copy link
Copy Markdown

PR Draft: HOST support + connection state-machine fixes

Title

HOST portability and connection-closing state-machine fixes

Target

  • Base: ESP32Async/ESPAsyncWebServer:main
  • Head: MitchBradley/ESPAsyncWebServer:Hosted

Summary

This PR adds and hardens HOST platform support and fixes connection-closing state-machine behavior that could cause disconnect instability under hosted/POSIX execution. It is used with https://github.com/pschatzmann/Arduino-Emulator for Arduino emulation on host machines and https://github.com/MitchBradley/PosixAsyncTCP for the async TCP layer using BSD networking.

The branch is based on main and includes 5 commits:

  1. 13643fd emptyString -> local _emptyString
  2. fd98dd1 hosted ifdefs and locking alignment
  3. e8297d7 additional #if defined(HOST) guards
  4. 00a50cb missing overrides/includes fixes
  5. 38dcb2a connection closing state-machine fixes

Main changes

  • Added/adjusted HOST guards where required for hosted builds.
  • Aligned locking behavior for hosted code paths with ESP32 expectations.
  • Fixed include/override issues found during hosted compilation.
  • Corrected connection-closing sequencing in async request/response/websocket/event paths to avoid premature end/close transitions.
  • Improved response finalization behavior around ACK/WAIT_ACK and chunked completion handling.

Files changed

  • src/AsyncEventSource.cpp
  • src/AsyncEventSource.h
  • src/AsyncWebServerLogging.h
  • src/AsyncWebSocket.cpp
  • src/AsyncWebSocket.h
  • src/ESPAsyncWebServer.h
  • src/WebAuthentication.cpp
  • src/WebAuthentication.h
  • src/WebHandlers.cpp
  • src/WebRequest.cpp
  • src/WebResponseImpl.h
  • src/WebResponses.cpp
  • src/WebServer.cpp

Testing / validation

  • Hosted integration tested in FluidNC POSIX environment.
  • Verified stable websocket keepalive behavior and disconnect handling after state-machine fixes.
  • No API-surface changes intended for ESP32 users; changes are compatibility and correctness oriented.

Notes for reviewers

  • The final commit (38dcb2a) contains the core connection state-machine fixes.
  • Earlier commits provide the hosted portability and compile/runtime prerequisites needed for that behavior.

Copilot AI review requested due to automatic review settings April 3, 2026 01:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds HOST (Linux/macOS POSIX) build support to ESPAsyncWebServer and adjusts response/request connection-closing state transitions to improve disconnect stability under hosted execution.

Changes:

  • Added/expanded HOST platform guards and compatibility shims across the WebServer, WebSocket, and EventSource code paths.
  • Introduced a library-owned empty String (_emptyString) to replace framework-specific emptyString.
  • Updated response write/close sequencing (WAIT_ACK/END transitions) and chunked-response finalization behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/AsyncEventSource.cpp Adds HOST locking guards; tweaks message queue handling.
src/AsyncEventSource.h Enables AsyncTCP/mutex usage for HOST; override specifier fixes.
src/AsyncWebServerLogging.h Adds HOST logging macro fallbacks (no-op).
src/AsyncWebSocket.cpp Adds HOST SHA1 builder path and extends ESP32-style locking to HOST.
src/AsyncWebSocket.h Adds HOST platform include path and compatibility macros; adjusts defaults/locking.
src/ESPAsyncWebServer.h Adds HOST lwIP include guard, __unused fallback, HOST AsyncTCP include, and declares _emptyString.
src/WebAuthentication.cpp Adds HOST MD5 include path; replaces emptyString with _emptyString.
src/WebAuthentication.h Adds using namespace arduino; to address hosted namespace differences.
src/WebHandlers.cpp Replaces emptyString usage with _emptyString for file responses.
src/WebRequest.cpp Replaces emptyString usage with _emptyString; extends “ancient core” String-clear guard to HOST.
src/WebResponseImpl.h Switches default params to _emptyString; adds _finalChunkQueued; adds printf shim for ArduinoCore-API.
src/WebResponses.cpp Implements _finalChunkQueued; modifies WAIT_ACK/END transition logic; adjusts in-flight credit accounting; adds AsyncResponseStream::printf shim.
src/WebServer.cpp Adds HOST WiFi include path; defines global _emptyString.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +525 to +526
// For fixed-length responses, make sure all content has been ACK'd
if (_ackedLength >= _contentLength) {
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In RESPONSE_WAIT_ACK, the completion condition uses _ackedLength >= _contentLength, but _ackedLength is incremented from the AsyncClient onAck len value which includes all bytes freed from the TCP send buffer (headers + body). This can mark the response as END before the body has actually been acknowledged (e.g., headers alone can satisfy _contentLength for small bodies), causing premature connection close. Consider tracking/deriving the acknowledged body length (e.g., subtract _headLength with saturation) or comparing against the total bytes queued/expected to be acknowledged (_headLength + _contentLength).

Suggested change
// For fixed-length responses, make sure all content has been ACK'd
if (_ackedLength >= _contentLength) {
// For fixed-length responses, _ackedLength includes both headers and body.
// Only count ACK'd body bytes after the full header has been acknowledged.
if (_ackedLength >= _headLength && (_ackedLength - _headLength) >= _contentLength) {

Copilot uses AI. Check for mistakes.
Comment on lines +530 to +531
// For streaming/chunked responses, once all buffered data is gone and sent, we're done
if (_send_buffer_len == 0) {
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For streaming/chunked responses in RESPONSE_WAIT_ACK, transitioning to END when _send_buffer_len == 0 does not guarantee that all bytes previously queued to AsyncClient have been acknowledged; _send_buffer_len only reflects the library’s temporary buffer, not the TCP send buffer/in-flight data. This can still close the connection while data remains unacknowledged. Suggest tracking the total number of bytes queued to the client (including chunk framing) and only transitioning to END once that total has been fully acknowledged, or otherwise using an API/metric that reflects the client TX buffer draining rather than _send_buffer_len.

Suggested change
// For streaming/chunked responses, once all buffered data is gone and sent, we're done
if (_send_buffer_len == 0) {
// For streaming/chunked responses, an empty staging buffer alone is not enough:
// AsyncClient may still have queued or in-flight bytes in the TCP send buffer.
// Only end the response once both our temporary buffer is empty and the client TX
// buffer has fully drained.
if (_send_buffer_len == 0 && _client && _client->space() >= CONFIG_LWIP_TCP_WND_DEFAULT) {

Copilot uses AI. Check for mistakes.
Comment on lines +205 to +208
if (_messageQueue.size() >= SSE_MAX_QUEUED_MESSAGES) {
async_ws_log_e("Event message queue overflow: discard message");
return false;
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_queueMessage() checks _messageQueue.size() before acquiring _lockmq under the ESP32/HOST guard, then checks again after the lock. The pre-lock access is a data race on platforms where the mutex is needed, and the duplicated overflow checks also produce inconsistent logging levels (error vs warning). Acquire the lock before any size check/mutation and keep a single overflow check/log path.

Suggested change
if (_messageQueue.size() >= SSE_MAX_QUEUED_MESSAGES) {
async_ws_log_e("Event message queue overflow: discard message");
return false;
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : please fix. Copilot is right here. The queue has to be accessed AFTER the lock. The check can also be removed this it is already done later.


#include "Arduino.h"

using namespace arduino;
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding using namespace arduino; in a public header leaks the arduino namespace into all including translation units and can create name collisions/ambiguity for consumers. Prefer avoiding using namespace in headers; if needed, use targeted using arduino::String; (and any other required types) or fully qualify where necessary.

Suggested change
using namespace arduino;
using arduino::String;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley I agree with copilot here. Also surround that by #if defined(HOST)

// for compatibility, but its implementation is cumbersome due to the
// lack of a convenient place to put the definition. For maximum
// portablity, we just make our own renamed version.
extern const String _emptyString;
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_emptyString is declared in the global namespace with a leading underscore. Identifiers beginning with an underscore are reserved to the implementation in the global namespace in standard C++, so this name can technically invoke undefined behavior or collide with toolchain internals. Consider renaming to a non-reserved identifier (e.g., emptyStringCompat, kEmptyString) and/or placing it inside a library namespace to avoid global reserved identifiers.

Suggested change
extern const String _emptyString;
extern const String emptyStringCompat;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley copilot is right and the right way to do that is to move this to our string bundle (literals.h) under the namespace asyncsrv, and use it namespaced.
In fact, we already asyncsrv:empty

#define FPSTR (const char *)
#endif

#include <AsyncTCP.h>
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the HOST include branch, AsyncWebSocket.h uses std::recursive_mutex (guarded by #if defined(ESP32) || defined(HOST)), but the HOST branch does not include <mutex>. Relying on transitive includes from AsyncTCP.h/other headers is brittle and can fail to compile on some toolchains. Add an explicit #include <mutex> for the HOST branch (or move the <mutex> include outside the platform ifdefs).

Suggested change
#include <AsyncTCP.h>
#include <AsyncTCP.h>
#include <mutex>

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : use instead #if defined(ESP32) || defined(LIBRETINY) || defined(HOST).

The FPSTR macro def could be moved inside also.

Copy link
Copy Markdown
Member

@mathieucarbou mathieucarbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley please check copilot comments and mine.

  • I suggest you keep this PR opened for the HOST compatiibility.

  • Please open a new PR regarding the state machine changes but also explain what you have fixed - this one needs a careful review

  • for the_emptyString stuff I will open another PR to refactor our code base: we already have asyncsrv::empty

Thank you for your PR 👍

Comment on lines +205 to +208
if (_messageQueue.size() >= SSE_MAX_QUEUED_MESSAGES) {
async_ws_log_e("Event message queue overflow: discard message");
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : please fix. Copilot is right here. The queue has to be accessed AFTER the lock. The check can also be removed this it is already done later.

Comment on lines +35 to +39
#define async_ws_log_e(format, ...)
#define async_ws_log_w(format, ...)
#define async_ws_log_i(format, ...)
#define async_ws_log_d(format, ...)
#define async_ws_log_v(format, ...)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need to be properly implemented. Can't you do something like 8266 ?

#define FPSTR (const char *)
#endif

#include <AsyncTCP.h>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : use instead #if defined(ESP32) || defined(LIBRETINY) || defined(HOST).

The FPSTR macro def could be moved inside also.

Comment on lines +22 to +24
#ifndef __unused
#define __unused __attribute__((unused))
#endif
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would surround that by #if defined(HOST) to not leak this definition in whatever includes this header.

#if defined(ESP32) || defined(LIBRETINY)
#include <AsyncTCP.h>
#include <assert.h>
#elif defined(HOST)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use instead #if defined(ESP32) || defined(LIBRETINY) | defined(HOST) - keep consistency with what exists please

// for compatibility, but its implementation is cumbersome due to the
// lack of a convenient place to put the definition. For maximum
// portablity, we just make our own renamed version.
extern const String _emptyString;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley copilot is right and the right way to do that is to move this to our string bundle (literals.h) under the namespace asyncsrv, and use it namespaced.
In fact, we already asyncsrv:empty


#include "Arduino.h"

using namespace arduino;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley I agree with copilot here. Also surround that by #if defined(HOST)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : the changes in this file have to be extracted in a separate and correctly documented PR explaining what is fixed and this has to be reviewed by team members - so this one will take more time than just the HOST compatibility.

@mathieucarbou
Copy link
Copy Markdown
Member

@MitchBradley : I have opened PR #417 - you should be able to rebase on top of this branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants