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

@@ -1336,8 +1336,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;
 
@@ -1394,7 +1396,20 @@ class AsyncWebServerResponse {
   virtual bool _failed() const;
   virtual bool _sourceValid() const;
   virtual void _respond(AsyncWebServerRequest *request);
-  virtual size_t _ack(AsyncWebServerRequest *request, size_t len, uint32_t time);
+
+  /**
+   * @brief write next portion of response data to send buffs
+   * this method (re)fills tcp send buffers, it could be called either at will
+   * or from a tcp_recv/tcp_poll callbacks from AsyncTCP
+   *
+   * @param request - used to access client object
+   * @param len - size of acknowledged data from the remote side (TCP window update, not TCP ack!)
+   * @param time - time passed between last sent and received packet
+   * @return size_t amount of response data placed to TCP send buffs for delivery (defined by sdkconfig value CONFIG_LWIP_TCP_SND_BUF_DEFAULT)
+   */
+  virtual size_t _ack(AsyncWebServerRequest *request, size_t len, uint32_t time) {
+    return 0;
+  };
 };
 
 /*

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 destructed 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,31 +217,26 @@ 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
     }
+  } else {
+    // this will close responses that were complete via a single _send() call
+    _client->close();  // this will trigger _onDisconnect() and object destruction
   }
 }
 
@@ -709,7 +704,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,21 +14,46 @@
 #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 {
 private:
   String _content;
+  // buffer to accumulate all response headers
+  String _assembled_headers;
+  // amount of headers buffer writtent to sockbuff
+  size_t _writtenHeadersLength{0};
 
 public:
   explicit AsyncBasicResponse(int code, const char *contentType = asyncsrv::empty, const char *content = asyncsrv::empty);
   AsyncBasicResponse(int code, const String &contentType, const String &content = emptyString)
     : AsyncBasicResponse(code, contentType.c_str(), content.c_str()) {}
   void _respond(AsyncWebServerRequest *request) override final;
-  size_t _ack(AsyncWebServerRequest *request, size_t len, uint32_t time) override final;
+  size_t _ack(AsyncWebServerRequest *request, size_t len, uint32_t time) override final {
+    return write_send_buffs(request, len, time);
+  };
   bool _sourceValid() const override final {
     return true;
   }
+
+protected:
+  /**
+   * @brief write next portion of response data to send buffs
+   * this method (re)fills tcp send buffers, it could be called either at will
+   * or from a tcp_recv/tcp_poll callbacks from AsyncTCP
+   *
+   * @param request - used to access client object
+   * @param len - size of acknowledged data from the remote side (TCP window update, not TCP ack!)
+   * @param time - time passed between last sent and received packet
+   * @return size_t amount of response data placed to TCP send buffs for delivery (defined by sdkconfig value CONFIG_LWIP_TCP_SND_BUF_DEFAULT)
+   */
+  size_t write_send_buffs(AsyncWebServerRequest *request, size_t len, uint32_t time);
 };
 
 class AsyncAbstractResponse : public AsyncWebServerResponse {
@@ -39,23 +64,43 @@ 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 _writtenHeadersLength{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);
 
 protected:
   AwsTemplateProcessor _callback;
+  /**
+   * @brief write next portion of response data to send buffs
+   * this method (re)fills tcp send buffers, it could be called either at will
+   * or from a tcp_recv/tcp_poll callbacks from AsyncTCP
+   *
+   * @param request - used to access client object
+   * @param len - size of acknowledged data from the remote side (TCP window update, not TCP ack!)
+   * @param time - time passed between last sent and received packet
+   * @return size_t amount of response data placed to TCP send buffs for delivery (defined by sdkconfig value CONFIG_LWIP_TCP_SND_BUF_DEFAULT)
+   */
+  size_t write_send_buffs(AsyncWebServerRequest *request, size_t len, uint32_t time);
 
 public:
   AsyncAbstractResponse(AwsTemplateProcessor callback = nullptr);
   virtual ~AsyncAbstractResponse() {}
   void _respond(AsyncWebServerRequest *request) override final;
-  size_t _ack(AsyncWebServerRequest *request, size_t len, uint32_t time) override final;
+  size_t _ack(AsyncWebServerRequest *request, size_t len, uint32_t time) override final {
+    return write_send_buffs(request, len, time);
+  };
   virtual bool _sourceValid() const {
     return false;
   }
@@ -142,7 +187,8 @@ class AsyncChunkedResponse : public AsyncAbstractResponse {
 class AsyncProgmemResponse : public AsyncAbstractResponse {
 private:
   const uint8_t *_content;
-  size_t _readLength;
+  // offset index (how much we've sent already)
+  size_t _index;
 
 public:
   AsyncProgmemResponse(int code, const char *contentType, const uint8_t *content, size_t len, AwsTemplateProcessor callback = nullptr);