From f345b297ac002a1ea93836982fed7c53675ab542 Mon Sep 17 00:00:00 2001 From: Jacopo Labardi <2527836+labodj@users.noreply.github.com> Date: Tue, 10 Nov 2026 23:18:58 +0100 Subject: [PATCH 1/4] fix: count partially accepted response bytes --- .github/workflows/response-regressions.yml | 20 ++++ .gitignore | 2 + src/WebResponses.cpp | 2 +- tests/native/response_fixture.h | 125 +++++++++++++++++++++ tests/response_harness.py | 33 ++++++ tests/test_response_accounting.py | 50 +++++++++ 6 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/response-regressions.yml create mode 100644 tests/native/response_fixture.h create mode 100644 tests/response_harness.py create mode 100644 tests/test_response_accounting.py diff --git a/.github/workflows/response-regressions.yml b/.github/workflows/response-regressions.yml new file mode 100644 index 000000000..9b9d7070f --- /dev/null +++ b/.github/workflows/response-regressions.yml @@ -0,0 +1,20 @@ +name: Response regression tests +on: + workflow_dispatch: + pull_request: + push: + branches: [main, 'release/*'] +permissions: + contents: read +jobs: + native: + runs-on: ubuntu-latest + strategy: + matrix: + cxx: [g++, clang++] + steps: + - uses: actions/checkout@v7 + - name: Run response tests with AddressSanitizer and UBSan + env: + CXX: ${{ matrix.cxx }} + run: python3 -m unittest discover -s tests -p 'test_response_*.py' -v diff --git a/.gitignore b/.gitignore index 2a6f6f74c..52406c767 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,5 @@ /build /examples/arduino/**/*idf_component.yml /examples/arduino/**/*idf_component.yml.orig + +__pycache__/ diff --git a/src/WebResponses.cpp b/src/WebResponses.cpp index 0eb437a80..afbf16148 100644 --- a/src/WebResponses.cpp +++ b/src/WebResponses.cpp @@ -437,6 +437,7 @@ size_t AsyncAbstractResponse::write_send_buffs(AsyncWebServerRequest *request, s // data is pending in buffer from a previous call or previous iteration; attempt to send it size_t const added_len = request->client()->add(reinterpret_cast(_send_buffer->data() + _send_buffer_offset), _send_buffer_len - _send_buffer_offset); + payloadlen += added_len; if (added_len != _send_buffer_len - _send_buffer_offset) { // we were not able to add entire buffer's content to tcp buffs, leave it for later // (this should not happen normally unless connection's TCP window suddenly changed from remote or mem pressure) @@ -445,7 +446,6 @@ size_t AsyncAbstractResponse::write_send_buffs(AsyncWebServerRequest *request, s } else { _send_buffer_len = _send_buffer_offset = 0; // consider buffer empty } - payloadlen += added_len; tcp_win -= added_len; } diff --git a/tests/native/response_fixture.h b/tests/native/response_fixture.h new file mode 100644 index 000000000..15c2e957f --- /dev/null +++ b/tests/native/response_fixture.h @@ -0,0 +1,125 @@ +// Test doubles for response state and the AsyncClient add/write contract. +// Production method bodies are read from src/, never duplicated here. +#pragma once +#include +#include +#include +#include +#include +#include +#include +#include +#include +using String = std::string; +#define ASYNC_WRITE_FLAG_COPY 1 +#define CONFIG_LWIP_TCP_MSS 536 +#define CONFIG_LWIP_TCP_WND_DEFAULT 2144 +#define ASYNC_RESPONCE_BUFF_SIZE 1072 +#ifndef TEST_TCP_DEFAULT_FLAGS +#define TEST_TCP_DEFAULT_FLAGS 0 +#endif +#define async_ws_log_e(...) \ + do { \ + } while (0) +constexpr size_t RESPONSE_TRY_AGAIN = std::numeric_limits::max(); +enum { + RESPONSE_SETUP, + RESPONSE_HEADERS, + RESPONSE_CONTENT, + RESPONSE_WAIT_ACK, + RESPONSE_END, + RESPONSE_FAILED +}; +struct Client { + struct Pending { + const char *data; + size_t length; + std::string copy; + }; + size_t quota = 1072, sends = 0; + bool deferred = false, closed = false; + std::string wire; + std::vector pending; + size_t space() const { + return 1072; + } + size_t add(const char *data, size_t length, uint8_t flags = TEST_TCP_DEFAULT_FLAGS) { + const size_t accepted = std::min(length, quota); + quota -= accepted; + if (accepted) { + if (deferred) { + pending.push_back({data, accepted, (flags & ASYNC_WRITE_FLAG_COPY) ? std::string(data, accepted) : std::string()}); + } else { + wire.append(data, accepted); + } + } + return accepted; + } + bool send() { + ++sends; + return true; + } + size_t write(const char *data, size_t length, uint8_t flags = TEST_TCP_DEFAULT_FLAGS) { + const size_t accepted = add(data, length, flags); + send(); + return accepted; + } + void close() { + closed = true; + } + template void onAck(Callback, void *) {} + void flush() { + for (const auto &p : pending) { + const char *data = p.copy.empty() ? p.data : p.copy.data(); + for (size_t i = 0; i < p.length; ++i) { + wire.push_back(data[i]); + } + } + pending.clear(); + } +}; +using AsyncClient = Client; +struct AsyncWebServerRequest { + Client tcp; + unsigned failures = 0; + Client *client() { + return &tcp; + } + Client *clientRelease() { + return &tcp; + } + unsigned version() const { + return 1; + } + void sendRawResponse(const char *) { + ++failures; + } + bool hasHeader(const char *) const { + return false; + } + struct Header { + String value() const { + return "0"; + } + } header; + Header *getHeader(const char *) { + return &header; + } +}; +struct AsyncAbstractResponse { + size_t _ackedLength = 0, _writtenLength = 0, _writtenHeadersLength = 0, _sentLength = 0, _contentLength = 0; + size_t _in_flight = 0; + unsigned _in_flight_credit = 2; + bool _chunked = false, _sendContentLength = true; + int _state = RESPONSE_WAIT_ACK; + String _assembled_headers; + std::unique_ptr> _send_buffer; + size_t _send_buffer_len = 0, _send_buffer_offset = 0; + bool _sourceValid() const { + return true; + } + size_t _fillBufferAndProcessTemplates(uint8_t *, size_t) { + return 0; + } + size_t write_send_buffs(AsyncWebServerRequest *, size_t, uint32_t); +}; diff --git a/tests/response_harness.py b/tests/response_harness.py new file mode 100644 index 000000000..ee3498e1a --- /dev/null +++ b/tests/response_harness.py @@ -0,0 +1,33 @@ +"""Compile the actual response methods with a controllable TCP peer.""" +import os +from pathlib import Path +import subprocess +import tempfile + +ROOT = Path(__file__).resolve().parents[1] + + +def method(filename, signature): + source = (ROOT / "src" / filename).read_text() + start = source.index(signature + "(") + # Production definitions use clang-format's unindented closing brace. + end = source.index("\n}", start) + 2 + return source[start:end] + + +def run_cases(test, source, cases, definitions): + with tempfile.TemporaryDirectory(prefix="async-response-test-") as directory: + path = Path(directory) + cpp = path / "response.cpp" + cpp.write_text(source) + binary = path / "response" + subprocess.run([ + os.environ.get("CXX", "g++"), "-std=c++17", "-O1", "-g", + "-fsanitize=address,undefined", "-fno-sanitize-recover=all", + "-fno-omit-frame-pointer", "-I", str(ROOT / "tests/native"), + *definitions, str(cpp), "-o", str(binary), + ], check=True) + for case in cases: + with test.subTest(case=case, definitions=definitions): + result = subprocess.run([str(binary), case], capture_output=True, text=True, timeout=10) + test.assertEqual(result.returncode, 0, result.stdout + result.stderr) diff --git a/tests/test_response_accounting.py b/tests/test_response_accounting.py new file mode 100644 index 000000000..a3979657a --- /dev/null +++ b/tests/test_response_accounting.py @@ -0,0 +1,50 @@ +"""Partial TCP acceptance must update return values and in-flight accounting.""" +import unittest +from response_harness import method, run_cases + +SOURCE = '#include "response_fixture.h"\n' + method("WebResponses.cpp", "size_t AsyncAbstractResponse::write_send_buffs") + r""" +int main(int argc, char **argv) { + assert(argc == 2); + const size_t quota = std::stoul(argv[1]); + const std::string expected = "0123456789abcdefghij"; + AsyncWebServerRequest request; + AsyncAbstractResponse response; + response._contentLength = response._sentLength = expected.size(); + response._send_buffer = std::make_unique>(); + memcpy(response._send_buffer->data(), expected.data(), expected.size()); + response._send_buffer_len = expected.size(); + // space() advertises a full window, but add() can accept less (including 0). + request.tcp.quota = quota; + size_t ack = 0; + for (unsigned attempt = 0; attempt < 32 && response._state != RESPONSE_END; ++attempt) { + const size_t before = request.tcp.wire.size(); + const size_t returned = response.write_send_buffs(&request, ack, 0); + const size_t accepted = request.tcp.wire.size() - before; + assert(returned == accepted); + assert(response._writtenLength == request.tcp.wire.size()); +#if ASYNCWEBSERVER_USE_CHUNK_INFLIGHT + assert(response._in_flight == accepted); +#endif + if (response._state != RESPONSE_END) { + assert(response._send_buffer); + assert(response._send_buffer_offset == request.tcp.wire.size()); + } + ack = accepted; + request.tcp.quota = std::max(1, quota); + } + assert(response._state == RESPONSE_END); + assert(!response._send_buffer && !request.tcp.closed); + assert(request.tcp.wire == expected); +} +""" + + +class ResponseAccountingTests(unittest.TestCase): + def test_partial_and_zero_acceptance(self): + for enabled in (0, 1): + run_cases(self, SOURCE, ("0", "1", "7", "19", "20"), + [f"-DASYNCWEBSERVER_USE_CHUNK_INFLIGHT={enabled}"]) + + +if __name__ == "__main__": + unittest.main() From 8ebaf4ab3a88c9c1a86f7cc63932496da5940958 Mon Sep 17 00:00:00 2001 From: Mathieu Carbou Date: Wed, 11 Nov 2026 22:14:57 +0100 Subject: [PATCH 2/4] Cleanup PR --- .github/workflows/response-regressions.yml | 20 ---- .gitignore | 2 - tests/native/response_fixture.h | 125 --------------------- tests/response_harness.py | 33 ------ tests/test_response_accounting.py | 50 --------- 5 files changed, 230 deletions(-) delete mode 100644 .github/workflows/response-regressions.yml delete mode 100644 tests/native/response_fixture.h delete mode 100644 tests/response_harness.py delete mode 100644 tests/test_response_accounting.py diff --git a/.github/workflows/response-regressions.yml b/.github/workflows/response-regressions.yml deleted file mode 100644 index 9b9d7070f..000000000 --- a/.github/workflows/response-regressions.yml +++ /dev/null @@ -1,20 +0,0 @@ -name: Response regression tests -on: - workflow_dispatch: - pull_request: - push: - branches: [main, 'release/*'] -permissions: - contents: read -jobs: - native: - runs-on: ubuntu-latest - strategy: - matrix: - cxx: [g++, clang++] - steps: - - uses: actions/checkout@v7 - - name: Run response tests with AddressSanitizer and UBSan - env: - CXX: ${{ matrix.cxx }} - run: python3 -m unittest discover -s tests -p 'test_response_*.py' -v diff --git a/.gitignore b/.gitignore index 52406c767..2a6f6f74c 100644 --- a/.gitignore +++ b/.gitignore @@ -9,5 +9,3 @@ /build /examples/arduino/**/*idf_component.yml /examples/arduino/**/*idf_component.yml.orig - -__pycache__/ diff --git a/tests/native/response_fixture.h b/tests/native/response_fixture.h deleted file mode 100644 index 15c2e957f..000000000 --- a/tests/native/response_fixture.h +++ /dev/null @@ -1,125 +0,0 @@ -// Test doubles for response state and the AsyncClient add/write contract. -// Production method bodies are read from src/, never duplicated here. -#pragma once -#include -#include -#include -#include -#include -#include -#include -#include -#include -using String = std::string; -#define ASYNC_WRITE_FLAG_COPY 1 -#define CONFIG_LWIP_TCP_MSS 536 -#define CONFIG_LWIP_TCP_WND_DEFAULT 2144 -#define ASYNC_RESPONCE_BUFF_SIZE 1072 -#ifndef TEST_TCP_DEFAULT_FLAGS -#define TEST_TCP_DEFAULT_FLAGS 0 -#endif -#define async_ws_log_e(...) \ - do { \ - } while (0) -constexpr size_t RESPONSE_TRY_AGAIN = std::numeric_limits::max(); -enum { - RESPONSE_SETUP, - RESPONSE_HEADERS, - RESPONSE_CONTENT, - RESPONSE_WAIT_ACK, - RESPONSE_END, - RESPONSE_FAILED -}; -struct Client { - struct Pending { - const char *data; - size_t length; - std::string copy; - }; - size_t quota = 1072, sends = 0; - bool deferred = false, closed = false; - std::string wire; - std::vector pending; - size_t space() const { - return 1072; - } - size_t add(const char *data, size_t length, uint8_t flags = TEST_TCP_DEFAULT_FLAGS) { - const size_t accepted = std::min(length, quota); - quota -= accepted; - if (accepted) { - if (deferred) { - pending.push_back({data, accepted, (flags & ASYNC_WRITE_FLAG_COPY) ? std::string(data, accepted) : std::string()}); - } else { - wire.append(data, accepted); - } - } - return accepted; - } - bool send() { - ++sends; - return true; - } - size_t write(const char *data, size_t length, uint8_t flags = TEST_TCP_DEFAULT_FLAGS) { - const size_t accepted = add(data, length, flags); - send(); - return accepted; - } - void close() { - closed = true; - } - template void onAck(Callback, void *) {} - void flush() { - for (const auto &p : pending) { - const char *data = p.copy.empty() ? p.data : p.copy.data(); - for (size_t i = 0; i < p.length; ++i) { - wire.push_back(data[i]); - } - } - pending.clear(); - } -}; -using AsyncClient = Client; -struct AsyncWebServerRequest { - Client tcp; - unsigned failures = 0; - Client *client() { - return &tcp; - } - Client *clientRelease() { - return &tcp; - } - unsigned version() const { - return 1; - } - void sendRawResponse(const char *) { - ++failures; - } - bool hasHeader(const char *) const { - return false; - } - struct Header { - String value() const { - return "0"; - } - } header; - Header *getHeader(const char *) { - return &header; - } -}; -struct AsyncAbstractResponse { - size_t _ackedLength = 0, _writtenLength = 0, _writtenHeadersLength = 0, _sentLength = 0, _contentLength = 0; - size_t _in_flight = 0; - unsigned _in_flight_credit = 2; - bool _chunked = false, _sendContentLength = true; - int _state = RESPONSE_WAIT_ACK; - String _assembled_headers; - std::unique_ptr> _send_buffer; - size_t _send_buffer_len = 0, _send_buffer_offset = 0; - bool _sourceValid() const { - return true; - } - size_t _fillBufferAndProcessTemplates(uint8_t *, size_t) { - return 0; - } - size_t write_send_buffs(AsyncWebServerRequest *, size_t, uint32_t); -}; diff --git a/tests/response_harness.py b/tests/response_harness.py deleted file mode 100644 index ee3498e1a..000000000 --- a/tests/response_harness.py +++ /dev/null @@ -1,33 +0,0 @@ -"""Compile the actual response methods with a controllable TCP peer.""" -import os -from pathlib import Path -import subprocess -import tempfile - -ROOT = Path(__file__).resolve().parents[1] - - -def method(filename, signature): - source = (ROOT / "src" / filename).read_text() - start = source.index(signature + "(") - # Production definitions use clang-format's unindented closing brace. - end = source.index("\n}", start) + 2 - return source[start:end] - - -def run_cases(test, source, cases, definitions): - with tempfile.TemporaryDirectory(prefix="async-response-test-") as directory: - path = Path(directory) - cpp = path / "response.cpp" - cpp.write_text(source) - binary = path / "response" - subprocess.run([ - os.environ.get("CXX", "g++"), "-std=c++17", "-O1", "-g", - "-fsanitize=address,undefined", "-fno-sanitize-recover=all", - "-fno-omit-frame-pointer", "-I", str(ROOT / "tests/native"), - *definitions, str(cpp), "-o", str(binary), - ], check=True) - for case in cases: - with test.subTest(case=case, definitions=definitions): - result = subprocess.run([str(binary), case], capture_output=True, text=True, timeout=10) - test.assertEqual(result.returncode, 0, result.stdout + result.stderr) diff --git a/tests/test_response_accounting.py b/tests/test_response_accounting.py deleted file mode 100644 index a3979657a..000000000 --- a/tests/test_response_accounting.py +++ /dev/null @@ -1,50 +0,0 @@ -"""Partial TCP acceptance must update return values and in-flight accounting.""" -import unittest -from response_harness import method, run_cases - -SOURCE = '#include "response_fixture.h"\n' + method("WebResponses.cpp", "size_t AsyncAbstractResponse::write_send_buffs") + r""" -int main(int argc, char **argv) { - assert(argc == 2); - const size_t quota = std::stoul(argv[1]); - const std::string expected = "0123456789abcdefghij"; - AsyncWebServerRequest request; - AsyncAbstractResponse response; - response._contentLength = response._sentLength = expected.size(); - response._send_buffer = std::make_unique>(); - memcpy(response._send_buffer->data(), expected.data(), expected.size()); - response._send_buffer_len = expected.size(); - // space() advertises a full window, but add() can accept less (including 0). - request.tcp.quota = quota; - size_t ack = 0; - for (unsigned attempt = 0; attempt < 32 && response._state != RESPONSE_END; ++attempt) { - const size_t before = request.tcp.wire.size(); - const size_t returned = response.write_send_buffs(&request, ack, 0); - const size_t accepted = request.tcp.wire.size() - before; - assert(returned == accepted); - assert(response._writtenLength == request.tcp.wire.size()); -#if ASYNCWEBSERVER_USE_CHUNK_INFLIGHT - assert(response._in_flight == accepted); -#endif - if (response._state != RESPONSE_END) { - assert(response._send_buffer); - assert(response._send_buffer_offset == request.tcp.wire.size()); - } - ack = accepted; - request.tcp.quota = std::max(1, quota); - } - assert(response._state == RESPONSE_END); - assert(!response._send_buffer && !request.tcp.closed); - assert(request.tcp.wire == expected); -} -""" - - -class ResponseAccountingTests(unittest.TestCase): - def test_partial_and_zero_acceptance(self): - for enabled in (0, 1): - run_cases(self, SOURCE, ("0", "1", "7", "19", "20"), - [f"-DASYNCWEBSERVER_USE_CHUNK_INFLIGHT={enabled}"]) - - -if __name__ == "__main__": - unittest.main() From dd43e1d16188d8a666bf7e0196cab84231efaef7 Mon Sep 17 00:00:00 2001 From: Mathieu Carbou Date: Wed, 11 Nov 2026 22:33:42 +0100 Subject: [PATCH 3/4] Removing unused _writtenLength --- src/ESPAsyncWebServer.h | 2 -- src/WebResponses.cpp | 6 +----- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/ESPAsyncWebServer.h b/src/ESPAsyncWebServer.h index b688528cc..b29684bd9 100644 --- a/src/ESPAsyncWebServer.h +++ b/src/ESPAsyncWebServer.h @@ -1717,8 +1717,6 @@ class AsyncWebServerResponse { // amount of data sent for content part of the response (excluding all headers) size_t _sentLength; size_t _ackedLength; - // amount of response bytes (including all headers) written to sockbuff for delivery - size_t _writtenLength; WebResponseState _state; static bool headerMustBePresentOnce(const String &name); diff --git a/src/WebResponses.cpp b/src/WebResponses.cpp index 1ab7db7c5..3a198ef77 100644 --- a/src/WebResponses.cpp +++ b/src/WebResponses.cpp @@ -74,7 +74,7 @@ STR_RETURN_TYPE AsyncWebServerResponse::responseCodeToString(int code) { } AsyncWebServerResponse::AsyncWebServerResponse() - : _code(0), _contentType(), _contentLength(0), _sendContentLength(true), _chunked(false), _headLength(0), _sentLength(0), _ackedLength(0), _writtenLength(0), + : _code(0), _contentType(), _contentLength(0), _sendContentLength(true), _chunked(false), _headLength(0), _sentLength(0), _ackedLength(0), _state(RESPONSE_SETUP) { for (const auto &header : DefaultHeaders::Instance()) { _headers.emplace_back(header); @@ -303,7 +303,6 @@ size_t AsyncBasicResponse::write_send_buffs(AsyncWebServerRequest *request, size if (_state == RESPONSE_HEADERS) { // copy headers buffer to sock buffer size_t const pcb_written = request->client()->add(_assembled_headers.c_str() + _writtenHeadersLength, _assembled_headers.length() - _writtenHeadersLength); - _writtenLength += pcb_written; _writtenHeadersLength += pcb_written; if (_writtenHeadersLength < _assembled_headers.length()) { // we were not able to fit all headers in current buff, send this part here and return later for the rest @@ -322,7 +321,6 @@ size_t AsyncBasicResponse::write_send_buffs(AsyncWebServerRequest *request, size if (_state == RESPONSE_CONTENT) { size_t const pcb_written = request->client()->write(_content.c_str() + _sentLength, _content.length() - _sentLength); - _writtenLength += pcb_written; // total written data (hdrs + body) _sentLength += pcb_written; // body written data payloadlen += pcb_written; // data writtent in current buff if (_sentLength >= _content.length()) { @@ -404,7 +402,6 @@ size_t AsyncAbstractResponse::write_send_buffs(AsyncWebServerRequest *request, s if (_state == RESPONSE_HEADERS) { // copy headers buffer to sock buffer size_t const pcb_written = request->client()->add(_assembled_headers.c_str() + _writtenHeadersLength, _assembled_headers.length() - _writtenHeadersLength); - _writtenLength += pcb_written; _writtenHeadersLength += pcb_written; if (_writtenHeadersLength < _assembled_headers.length()) { // we were not able to fit all headers in current buff, send this part here and return later for the rest @@ -519,7 +516,6 @@ size_t AsyncAbstractResponse::write_send_buffs(AsyncWebServerRequest *request, s // execute sending whatever we have in sock buffs now request->client()->send(); - _writtenLength += payloadlen; #if ASYNCWEBSERVER_USE_CHUNK_INFLIGHT // Take a credit only for data actually put in flight. When the source // had nothing yet (RESPONSE_TRY_AGAIN), nothing will be acked to return From cdfb5eea8f3f46ac7a3629e6e54288226576a3a7 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci-lite[bot]" <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Date: Wed, 11 Nov 2026 22:12:43 +0000 Subject: [PATCH 4/4] ci(pre-commit): Apply automatic fixes --- src/WebResponses.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/WebResponses.cpp b/src/WebResponses.cpp index 3a198ef77..9af04e744 100644 --- a/src/WebResponses.cpp +++ b/src/WebResponses.cpp @@ -321,8 +321,8 @@ size_t AsyncBasicResponse::write_send_buffs(AsyncWebServerRequest *request, size if (_state == RESPONSE_CONTENT) { size_t const pcb_written = request->client()->write(_content.c_str() + _sentLength, _content.length() - _sentLength); - _sentLength += pcb_written; // body written data - payloadlen += pcb_written; // data writtent in current buff + _sentLength += pcb_written; // body written data + payloadlen += pcb_written; // data writtent in current buff if (_sentLength >= _content.length()) { // we've just sent all the (remainder) data in current buff, complete the response _state = RESPONSE_END;