fix(ws,es): Fix AsyncWebRequest life cycle

Fix double-delete cases when AsyncWebRequest is being transformed in to
another form.   Adoping the AsyncClient from the AsyncWebRequest now
marks it for cleanup internally, so handlers need not delete it
manually.

H/T @mathieucarbou

Author: willmmiles

src/AsyncEventSource.cpp

@@ -147,11 +147,8 @@ size_t AsyncEventSourceMessage::send(AsyncClient *client) {
 
 // Client
 
-AsyncEventSourceClient::AsyncEventSourceClient(AsyncWebServerRequest *request, AsyncEventSource *server) : _client(request->clientRelease()), _server(server) {
-
-  if (request->hasHeader(T_Last_Event_ID)) {
-    _lastId = atoi(request->getHeader(T_Last_Event_ID)->value().c_str());
-  }
+AsyncEventSourceClient::AsyncEventSourceClient(AsyncClient *client, AsyncEventSource *server, uint32_t lastId)
+  : _client(client), _server(server), _lastId(lastId) {
 
   _client->setRxTimeout(0);
   _client->onError(NULL, NULL);
@@ -186,8 +183,6 @@ AsyncEventSourceClient::AsyncEventSourceClient(AsyncWebServerRequest *request, A
 
   _server->_addClient(this);
   _client->setNoDelay(true);
-  // delete AsyncWebServerRequest object (and bound response) since we have the ownership on client connection now
-  delete request;
 }
 
 AsyncEventSourceClient::~AsyncEventSourceClient() {
@@ -478,24 +473,12 @@ AsyncEventSourceResponse::AsyncEventSourceResponse(AsyncEventSource *server) : _
 void AsyncEventSourceResponse::_respond(AsyncWebServerRequest *request) {
   String out;
   _assembleHead(out, request->version());
-  // unbind client's onAck callback from AsyncWebServerRequest's, we will destroy it on next callback and steal the client,
-  // can't do it now 'cause now we are in AsyncWebServerRequest::_onAck 's stack actually
-  // here we are loosing time on one RTT delay, but with current design we can't get rid of Req/Resp objects other way
-  _request = request;
-  request->client()->onAck(
-    [](void *r, AsyncClient *c, size_t len, uint32_t time) {
-      if (len) {
-        static_cast<AsyncEventSourceResponse *>(r)->_switchClient();
-      }
-    },
-    this
-  );
+  uint32_t lastId = 0;
+  if (request->hasHeader(T_Last_Event_ID)) {
+    lastId = strtoul(request->getHeader(T_Last_Event_ID)->value().c_str(), nullptr, 10);
+  }
   request->client()->write(out.c_str(), _headLength);
-  _state = RESPONSE_WAIT_ACK;
+  // Add a new AsyncEventSourceClient to the server's list of clients
+  // This adopts the ownership of the AsyncTCP's client pointer from `request` parameter
+  new AsyncEventSourceClient(request->clientRelease(), _server, lastId);
 }
-
-void AsyncEventSourceResponse::_switchClient() {
-  // AsyncEventSourceClient c-tor will take the ownership of AsyncTCP's client connection
-  new AsyncEventSourceClient(_request, _server);
-  // AsyncEventSourceClient c-tor would also delete _request and *this
-};

src/AsyncEventSource.h

@@ -144,12 +144,13 @@ class AsyncEventSourceClient {
 public:
   /**
    * @brief Construct a new Async Event Source Client object
-   * @note constructor would take the ownership of of AsyncTCP's client pointer from `request` parameter and call delete on it!
+   * @note constructor is normally passed a client object from AsyncWebServerRequest::releaseClient(); see AsyncEventSourceResponse::_respond()
    *
    * @param request
    * @param server
+   * @param lastId
    */
-  AsyncEventSourceClient(AsyncWebServerRequest *request, AsyncEventSource *server);
+  AsyncEventSourceClient(AsyncClient *client, AsyncEventSource *server, uint32_t lastId = 0);
   ~AsyncEventSourceClient();
 
   /**
@@ -319,9 +320,6 @@ class AsyncEventSource : public AsyncWebHandler {
 class AsyncEventSourceResponse : public AsyncWebServerResponse {
 private:
   AsyncEventSource *_server;
-  AsyncWebServerRequest *_request;
-  // this call back will switch AsyncTCP client to SSE
-  void _switchClient();
 
 public:
   AsyncEventSourceResponse(AsyncEventSource *server);

src/AsyncWebSocket.cpp

@@ -997,11 +997,13 @@ void AsyncWebSocket::_handleEvent(AsyncWebSocketClient *client, AwsEventType typ
 
 AsyncWebSocketClient *AsyncWebSocket::_newClient(AsyncWebServerRequest *request) {
   asyncsrv::lock_guard_type lock(_ws_clients_lock);
-  _clients.emplace_back(request, this);
+  // Hold the request in scope for the user callback to inspect
+  std::shared_ptr<AsyncWebServerRequest> req_lock = request->shared_from_this();
+  // Adopt the client object from the request
+  _clients.emplace_back(request->clientRelease(), this);
   // we've just detached AsyncTCP client from AsyncWebServerRequest
   _handleEvent(&_clients.back(), WS_EVT_CONNECT, request, NULL, 0);
-  // after user code completed CONNECT event callback we can delete req/response objects
-  delete request;
+  // req_lock releases the request object at the end of this function scope
   return &_clients.back();
 }
 

src/AsyncWebSocket.h

@@ -241,14 +241,6 @@ class AsyncWebSocketClient {
   void *_tempObject;
 
   AsyncWebSocketClient(AsyncClient *client, AsyncWebSocket *server);
-
-  /**
-   * @brief Construct a new Async Web Socket Client object
-   * @note constructor would take the ownership of of AsyncTCP's client pointer from `request` parameter and call delete on it!
-   * @param request
-   * @param server
-   */
-  AsyncWebSocketClient(AsyncWebServerRequest *request, AsyncWebSocket *server) : AsyncWebSocketClient(request->clientRelease(), server){};
   ~AsyncWebSocketClient();
 
   // client id increments for the given server

src/ESPAsyncWebServer.h

@@ -526,6 +526,10 @@ class AsyncWebServerRequest {
   }
   ~AsyncWebServerRequest();
 
+  std::shared_ptr<AsyncWebServerRequest> shared_from_this() {
+    return _this;
+  }
+
   AsyncClient *client() {
     return _client;
   }
@@ -535,6 +539,8 @@ class AsyncWebServerRequest {
    * AsyncClient pointer will be abandoned in this instance,
    * the further ownership of the connection should be managed out of request's life-time scope
    * could be used for long lived connection like SSE or WebSockets
+   * This causes the request object to self-destruct; make sure you're holding a shared_ptr if
+   * you need to keep it alive any longer (see shared_from_this())
    * @note do not call this method unless you know what you are doing, otherwise it may lead to
    * memory leaks and connections lingering
    *

src/WebRequest.cpp

@@ -1476,6 +1476,15 @@ bool AsyncWebServerRequest::isExpectedRequestedConnType(RequestedConnectionType
 AsyncClient *AsyncWebServerRequest::clientRelease() {
   AsyncClient *c = _client;
   _client = nullptr;
+  // Ensure the client object no longer refers to us
+  c->onError({}, nullptr);
+  c->onAck({}, nullptr);
+  c->onDisconnect({}, nullptr);
+  c->onTimeout({}, nullptr);
+  c->onData({}, nullptr);
+  c->onPoll({}, nullptr);
+  // Now that we are no longer bound to the client, self-destruct at the earliest opportunity
+  _this.reset();
   return c;
 }