Sitelet https://github.com/ESP32Async/ESPAsyncWebServer/commit/5fdce2a1ae9fb3a01abbfde8d0db1316f45c223b
Skip to content

Commit 5fdce2a

Browse files
willmmileswillmmiles
committed
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
1 parent 89af998 commit 5fdce2a

6 files changed

Lines changed: 35 additions & 43 deletions

File tree

‎src/AsyncEventSource.cpp‎

Lines changed: 9 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -147,11 +147,8 @@ size_t AsyncEventSourceMessage::send(AsyncClient *client) {
147147

148148
// Client
149149

150-
AsyncEventSourceClient::AsyncEventSourceClient(AsyncWebServerRequest *request, AsyncEventSource *server) : _client(request->clientRelease()), _server(server) {
151-
152-
if (request->hasHeader(T_Last_Event_ID)) {
153-
_lastId = atoi(request->getHeader(T_Last_Event_ID)->value().c_str());
154-
}
150+
AsyncEventSourceClient::AsyncEventSourceClient(AsyncClient *client, AsyncEventSource *server, uint32_t lastId)
151+
: _client(client), _server(server), _lastId(lastId) {
155152

156153
_client->setRxTimeout(0);
157154
_client->onError(NULL, NULL);
@@ -186,8 +183,6 @@ AsyncEventSourceClient::AsyncEventSourceClient(AsyncWebServerRequest *request, A
186183

187184
_server->_addClient(this);
188185
_client->setNoDelay(true);
189-
// delete AsyncWebServerRequest object (and bound response) since we have the ownership on client connection now
190-
delete request;
191186
}
192187

193188
AsyncEventSourceClient::~AsyncEventSourceClient() {
@@ -478,24 +473,12 @@ AsyncEventSourceResponse::AsyncEventSourceResponse(AsyncEventSource *server) : _
478473
void AsyncEventSourceResponse::_respond(AsyncWebServerRequest *request) {
479474
String out;
480475
_assembleHead(out, request->version());
481-
// unbind client's onAck callback from AsyncWebServerRequest's, we will destroy it on next callback and steal the client,
482-
// can't do it now 'cause now we are in AsyncWebServerRequest::_onAck 's stack actually
483-
// here we are loosing time on one RTT delay, but with current design we can't get rid of Req/Resp objects other way
484-
_request = request;
485-
request->client()->onAck(
486-
[](void *r, AsyncClient *c, size_t len, uint32_t time) {
487-
if (len) {
488-
static_cast<AsyncEventSourceResponse *>(r)->_switchClient();
489-
}
490-
},
491-
this
492-
);
476+
uint32_t lastId = 0;
477+
if (request->hasHeader(T_Last_Event_ID)) {
478+
lastId = strtoul(request->getHeader(T_Last_Event_ID)->value().c_str(), nullptr, 10);
479+
}
493480
request->client()->write(out.c_str(), _headLength);
494-
_state = RESPONSE_WAIT_ACK;
481+
// Add a new AsyncEventSourceClient to the server's list of clients
482+
// This adopts the ownership of the AsyncTCP's client pointer from `request` parameter
483+
new AsyncEventSourceClient(request->clientRelease(), _server, lastId);
495484
}
496-
497-
void AsyncEventSourceResponse::_switchClient() {
498-
// AsyncEventSourceClient c-tor will take the ownership of AsyncTCP's client connection
499-
new AsyncEventSourceClient(_request, _server);
500-
// AsyncEventSourceClient c-tor would also delete _request and *this
501-
};

‎src/AsyncEventSource.h‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,12 +144,13 @@ class AsyncEventSourceClient {
144144
public:
145145
/**
146146
* @brief Construct a new Async Event Source Client object
147-
* @note constructor would take the ownership of of AsyncTCP's client pointer from `request` parameter and call delete on it!
147+
* @note constructor is normally passed a client object from AsyncWebServerRequest::releaseClient(); see AsyncEventSourceResponse::_respond()
148148
*
149-
* @param request
149+
* @param client
150150
* @param server
151+
* @param lastId
151152
*/
152-
AsyncEventSourceClient(AsyncWebServerRequest *request, AsyncEventSource *server);
153+
AsyncEventSourceClient(AsyncClient *client, AsyncEventSource *server, uint32_t lastId = 0);
153154
~AsyncEventSourceClient();
154155

155156
/**
@@ -319,9 +320,6 @@ class AsyncEventSource : public AsyncWebHandler {
319320
class AsyncEventSourceResponse : public AsyncWebServerResponse {
320321
private:
321322
AsyncEventSource *_server;
322-
AsyncWebServerRequest *_request;
323-
// this call back will switch AsyncTCP client to SSE
324-
void _switchClient();
325323

326324
public:
327325
AsyncEventSourceResponse(AsyncEventSource *server);

‎src/AsyncWebSocket.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -997,11 +997,13 @@ void AsyncWebSocket::_handleEvent(AsyncWebSocketClient *client, AwsEventType typ
997997

998998
AsyncWebSocketClient *AsyncWebSocket::_newClient(AsyncWebServerRequest *request) {
999999
asyncsrv::lock_guard_type lock(_ws_clients_lock);
1000-
_clients.emplace_back(request, this);
1000+
// Hold the request in scope for the user callback to inspect
1001+
std::shared_ptr<AsyncWebServerRequest> req_lock = request->shared_from_this();
1002+
// Adopt the client object from the request
1003+
_clients.emplace_back(request->clientRelease(), this);
10011004
// we've just detached AsyncTCP client from AsyncWebServerRequest
10021005
_handleEvent(&_clients.back(), WS_EVT_CONNECT, request, NULL, 0);
1003-
// after user code completed CONNECT event callback we can delete req/response objects
1004-
delete request;
1006+
// req_lock releases the request object at the end of this function scope
10051007
return &_clients.back();
10061008
}
10071009

‎src/AsyncWebSocket.h‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -241,14 +241,6 @@ class AsyncWebSocketClient {
241241
void *_tempObject;
242242

243243
AsyncWebSocketClient(AsyncClient *client, AsyncWebSocket *server);
244-
245-
/**
246-
* @brief Construct a new Async Web Socket Client object
247-
* @note constructor would take the ownership of of AsyncTCP's client pointer from `request` parameter and call delete on it!
248-
* @param request
249-
* @param server
250-
*/
251-
AsyncWebSocketClient(AsyncWebServerRequest *request, AsyncWebSocket *server) : AsyncWebSocketClient(request->clientRelease(), server){};
252244
~AsyncWebSocketClient();
253245

254246
// client id increments for the given server

‎src/ESPAsyncWebServer.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,10 @@ class AsyncWebServerRequest {
526526
}
527527
~AsyncWebServerRequest();
528528

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

‎src/WebRequest.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1476,6 +1476,17 @@ bool AsyncWebServerRequest::isExpectedRequestedConnType(RequestedConnectionType
14761476
AsyncClient *AsyncWebServerRequest::clientRelease() {
14771477
AsyncClient *c = _client;
14781478
_client = nullptr;
1479+
// Ensure the client object no longer refers to us
1480+
c->onError({}, nullptr);
1481+
c->onAck({}, nullptr);
1482+
c->onDisconnect({}, nullptr);
1483+
c->onTimeout({}, nullptr);
1484+
c->onData({}, nullptr);
1485+
c->onPoll({}, nullptr);
1486+
// Now that we are no longer bound to the client, self-destruct at the earliest opportunity by moving the shared pointer to a local.
1487+
// This will decrement the reference count and delete the object when this function ends if there are no other references.
1488+
// If this function is called by an _onAck handler or the like, the local lock will keep the request object in scope until the handler returns, preventing a use-after-free.
1489+
std::shared_ptr<AsyncWebServerRequest> destroy_self = std::move(_this);
14791490
return c;
14801491
}
14811492

0 commit comments

Comments
 (0)