fix: AsyncAbstractResponse might loose part of send buffer

AsyncAbstractResponse::_ack could allocate temp buffer with size larger than
available sock buffer (i.e. to fit headers) and eventually lossing the remainder on transfer
due to not checking if the complete data was added to sock buff.

Refactoring code in favor of having a dedicated std::vector object acting as accumulating
buffer and more carefull control on amount of data actually copied to sockbuff

Closes #315

Author: vortigont

src/ESPAsyncWebServer.h

@@ -1105,8 +1105,10 @@ class AsyncWebServerResponse {
   bool _sendContentLength;
   bool _chunked;
   size_t _headLength;
+  // 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;
 

src/WebRequest.cpp

@@ -33,66 +33,66 @@ AsyncWebServerRequest::AsyncWebServerRequest(AsyncWebServer *s, AsyncClient *c)
     [](void *r, AsyncClient *c, int8_t error) {
       (void)c;
       // async_ws_log_e("AsyncWebServerRequest::_onError");
-      AsyncWebServerRequest *req = (AsyncWebServerRequest *)r;
-      req->_onError(error);
+      static_cast<AsyncWebServerRequest*>(r)->_onError(error);
     },
     this
   );
   c->onAck(
     [](void *r, AsyncClient *c, size_t len, uint32_t time) {
       (void)c;
       // async_ws_log_e("AsyncWebServerRequest::_onAck");
-      AsyncWebServerRequest *req = (AsyncWebServerRequest *)r;
-      req->_onAck(len, time);
+      static_cast<AsyncWebServerRequest*>(r)->_onAck(len, time);
     },
     this
   );
   c->onDisconnect(
     [](void *r, AsyncClient *c) {
       // async_ws_log_e("AsyncWebServerRequest::_onDisconnect");
-      AsyncWebServerRequest *req = (AsyncWebServerRequest *)r;
-      req->_onDisconnect();
-      delete c;
+      static_cast<AsyncWebServerRequest*>(r)->_onDisconnect();
     },
     this
   );
   c->onTimeout(
     [](void *r, AsyncClient *c, uint32_t time) {
       (void)c;
       // async_ws_log_e("AsyncWebServerRequest::_onTimeout");
-      AsyncWebServerRequest *req = (AsyncWebServerRequest *)r;
-      req->_onTimeout(time);
+      static_cast<AsyncWebServerRequest*>(r)->_onTimeout(time);
     },
     this
   );
   c->onData(
     [](void *r, AsyncClient *c, void *buf, size_t len) {
       (void)c;
       // async_ws_log_e("AsyncWebServerRequest::_onData");
-      AsyncWebServerRequest *req = (AsyncWebServerRequest *)r;
-      req->_onData(buf, len);
+      static_cast<AsyncWebServerRequest*>(r)->_onData(buf, len);
     },
     this
   );
   c->onPoll(
     [](void *r, AsyncClient *c) {
       (void)c;
       // async_ws_log_e("AsyncWebServerRequest::_onPoll");
-      AsyncWebServerRequest *req = (AsyncWebServerRequest *)r;
-      req->_onPoll();
+      static_cast<AsyncWebServerRequest*>(r)->_onPoll();
     },
     this
   );
 }
 
 AsyncWebServerRequest::~AsyncWebServerRequest() {
-  // async_ws_log_e("AsyncWebServerRequest::~AsyncWebServerRequest");
+  if (_client){
+    // usually it is _client's disconnect triggers object destruct, but for completeness we define behavior
+    // if for some reason *this will be desturcted while client is still connected
+    _client->onDisconnect(nullptr);
+    delete _client;
+    _client = nullptr;
+  }
 
-  _this.reset();
+  if (_response){
+    delete _response;
+    _response = nullptr;
+  }
 
-  AsyncWebServerResponse *r = _response;
-  _response = NULL;
-  delete r;
+  _this.reset();
 
   if (_tempObject != NULL) {
     free(_tempObject);
@@ -217,30 +217,20 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) {
 
 void AsyncWebServerRequest::_onPoll() {
   // os_printf("p\n");
-  if (_response != NULL && _client != NULL && _client->canSend()) {
-    if (!_response->_finished()) {
-      _response->_ack(this, 0, 0);
-    } else {
-      AsyncWebServerResponse *r = _response;
-      _response = NULL;
-      delete r;
-
-      _client->close();
-    }
+  if (_response && _client && _client->canSend()) {
+    _response->_ack(this, 0, 0);
   }
 }
 
 void AsyncWebServerRequest::_onAck(size_t len, uint32_t time) {
   // os_printf("a:%u:%u\n", len, time);
-  if (_response != NULL) {
-    if (!_response->_finished()) {
-      _response->_ack(this, len, time);
-    } else if (_response->_finished()) {
-      AsyncWebServerResponse *r = _response;
-      _response = NULL;
-      delete r;
-
-      _client->close();
+  if (!_response) return;
+
+  if (!_response->_finished()) {
+    _response->_ack(this, len, time);
+    // recheck if response has just completed, close connection
+    if (_response->_finished()) {
+      _client->close(); // this will trigger _onDisconnect() and object destruction
     }
   }
 }
@@ -709,7 +699,7 @@ void AsyncWebServerRequest::_send() {
       send(500, T_text_plain, "Invalid data in handler");
     }
 
-    // here, we either have a response give nfrom user or one of the two above
+    // here, we either have a response given from user or one of the two above
     _client->setRxTimeout(0);
     _response->_respond(this);
     _sent = true;

src/WebResponseImpl.h

@@ -14,6 +14,12 @@
 #include <memory>
 #include <vector>
 
+#ifndef CONFIG_LWIP_TCP_MSS
+// as it is defined for ESP32's Arduino LWIP
+#define CONFIG_LWIP_TCP_MSS 1436
+#endif
+
+#define ASYNC_RESPONCE_BUFF_SIZE CONFIG_LWIP_TCP_MSS * 2
 // It is possible to restore these defines, but one can use _min and _max instead. Or std::min, std::max.
 
 class AsyncBasicResponse : public AsyncWebServerResponse {
@@ -39,12 +45,19 @@ class AsyncAbstractResponse : public AsyncWebServerResponse {
   // in-flight queue credits
   size_t _in_flight_credit{2};
 #endif
-  String _head;
+  // buffer to accumulate all response headers
+  String _assembled_headers;
+  // amount of headers buffer writtent to sockbuff
+  size_t _assembled_headers_written{0};
   // Data is inserted into cache at begin().
   // This is inefficient with vector, but if we use some other container,
   // we won't be able to access it as contiguous array of bytes when reading from it,
   // so by gaining performance in one place, we'll lose it in another.
   std::vector<uint8_t> _cache;
+  // intermediate buffer to copy outbound data to, also it will keep pending data between _send calls
+  std::unique_ptr< std::array<uint8_t, ASYNC_RESPONCE_BUFF_SIZE> > _send_buffer;
+  // buffer data size specifiers
+  size_t _send_buffer_offset{0}, _send_buffer_len{0};
   size_t _readDataFromCacheOrContent(uint8_t *data, const size_t len);
   size_t _fillBufferAndProcessTemplates(uint8_t *buf, size_t maxLen);