little correction

Author: playmiel

README.md

@@ -470,9 +470,19 @@ Common HTTPS errors:
 
 ## Thread Safety
 
-- AsyncTCP callbacks run on the lwIP/WiFi task while `loop()` (or the auto-loop task) runs on a different core. Since v2.1 the library guards against use-after-free by holding `RequestContext` in `std::shared_ptr` (captured by transport lambdas) and using an `std::atomic<bool> cancelled` flag that is set before cleanup erases the context.
-- On ESP32 with `ASYNC_HTTP_ENABLE_AUTOLOOP`, a recursive mutex protects shared containers (`_activeRequests`, `_pendingQueue`, etc.).
-- Callbacks are still executed in the context of the network event loop — keep them lightweight and non-blocking.
+The client is internally synchronized on ESP32. The model has three actors:
+
+- **lwIP/WiFi task (`tcpip_thread`)** — where AsyncTCP fires data/disconnect/error callbacks. These handlers do *no* heavy work and never take the client lock: they only copy the payload into a thread-safe `WorkerBuffer` (PSRAM-backed) and return immediately, so the network task is never blocked.
+- **Worker task (`AsyncHttpWorker`)** — drains the `WorkerBuffer`, takes the recursive client mutex, runs the actual response parsing (`handleData`/`handleDisconnect`/`handleTransportError`), then releases the lock and dispatches user callbacks.
+- **Your task(s)** — the public API (`get`, `post`, `setHeader`, …) takes the same recursive mutex, so configuration and request submission are safe to call from any task.
+
+Key guarantees and details:
+
+- **Single recursive mutex** (`_reqMutex`) protects all shared state (`_activeRequests`, `_pendingQueue`, headers, etc.). It is recursive so a user callback may re-enter the client (start/abort a request, change config) without deadlocking. Ownership is queried via FreeRTOS's native `xSemaphoreGetMutexHolder()` — no manual depth bookkeeping.
+- **User callbacks run outside the lock.** Success/error/body-chunk callbacks are queued and dispatched by `dispatchCallbacks()` after the mutex is released, so you can safely call client methods from inside a callback. A callback may run either on the worker task or on the task that submitted work — do not assume a fixed thread, and keep callbacks non-blocking (a blocking callback can stall the worker).
+- **Use-after-free protection.** `RequestContext` is held in `std::shared_ptr` captured by the transport lambdas, and an `std::atomic<bool> cancelled` flag (set before cleanup erases the context) makes in-flight callbacks no-op safely.
+- **Back-pressure.** If the `WorkerBuffer` reaches its hard ceiling (`ASYNC_HTTP_RING_BUFFER_MAX`, default 64 KiB) the transport is closed, which surfaces as a normal disconnect/error on that request.
+- **Clean shutdown.** The destructor does not `vTaskDelete()` the worker (which could be mid-parse holding the lock). It sets an exit flag, wakes the worker, and waits for it to leave its loop and self-delete before destroying the mutex. Note: destroying an `AsyncHttpClient` while requests are still in flight is still best avoided — abort outstanding requests (or let them finish) before destruction, since open transports hold lambdas that reference the client.
 
 ## Dependencies
 

src/AsyncHttpClient.cpp

@@ -35,6 +35,8 @@ AsyncHttpClient::AsyncHttpClient()
     _reqMutex = xSemaphoreCreateRecursiveMutex();
 #endif
 #ifdef ARDUINO_ARCH_ESP32
+    // Lets the destructor block until the worker has actually left its loop before we tear down state.
+    _workerDoneSem = xSemaphoreCreateBinary();
     xTaskCreatePinnedToCore(_workerTaskThunk,   // entry
                             "AsyncHttpWorker",  // name
                             8192,               // stack words
@@ -58,13 +60,6 @@ AsyncHttpClient::AsyncHttpClient()
 }
 
 AsyncHttpClient::~AsyncHttpClient() {
-#ifdef ARDUINO_ARCH_ESP32
-    if (_workerTaskHandle) {
-        TaskHandle_t h = _workerTaskHandle;
-        _workerTaskHandle = nullptr;
-        vTaskDelete(h);
-    }
-#endif
 #if !ASYNC_TCP_HAS_TIMEOUT && defined(ARDUINO_ARCH_ESP32) && defined(ASYNC_HTTP_ENABLE_AUTOLOOP)
     if (_autoLoopTaskHandle) {
         TaskHandle_t h = _autoLoopTaskHandle;
@@ -73,6 +68,20 @@ AsyncHttpClient::~AsyncHttpClient() {
     }
 #endif
 #ifdef ARDUINO_ARCH_ESP32
+    // Stop the worker cooperatively: it may be holding _reqMutex while in handleData(), so we must
+    // not vTaskDelete() it from here. Signal exit, wake it, and wait until it has left its loop
+    // (and self-deleted) before destroying the mutex it could otherwise still be holding.
+    if (_workerTaskHandle) {
+        _workerShouldExit.store(true, std::memory_order_release);
+        _workerBuffer.wake(); // unblock waitForItem()
+        if (_workerDoneSem)
+            xSemaphoreTake(_workerDoneSem, portMAX_DELAY);
+        _workerTaskHandle = nullptr;
+    }
+    if (_workerDoneSem) {
+        vSemaphoreDelete(_workerDoneSem);
+        _workerDoneSem = nullptr;
+    }
     if (_reqMutex) {
         vSemaphoreDelete(_reqMutex);
         _reqMutex = nullptr;
@@ -86,29 +95,12 @@ AsyncHttpClient::~AsyncHttpClient() {
 
 #ifdef ARDUINO_ARCH_ESP32
 void AsyncHttpClient::lock() const {
-    if (_reqMutex) {
+    if (_reqMutex)
         xSemaphoreTakeRecursive(_reqMutex, portMAX_DELAY);
-        TaskHandle_t current = xTaskGetCurrentTaskHandle();
-        if (_reqMutexOwner.load(std::memory_order_relaxed) == current) {
-            _reqMutexDepth.fetch_add(1, std::memory_order_relaxed);
-        } else {
-            _reqMutexOwner.store(current, std::memory_order_relaxed);
-            _reqMutexDepth.store(1, std::memory_order_relaxed);
-        }
-    }
 }
 void AsyncHttpClient::unlock() const {
-    if (_reqMutex) {
-        TaskHandle_t current = xTaskGetCurrentTaskHandle();
-        uint16_t depth = _reqMutexDepth.load(std::memory_order_relaxed);
-        if (_reqMutexOwner.load(std::memory_order_relaxed) == current && depth > 0) {
-            depth = static_cast<uint16_t>(depth - 1);
-            _reqMutexDepth.store(depth, std::memory_order_relaxed);
-            if (depth == 0)
-                _reqMutexOwner.store(nullptr, std::memory_order_relaxed);
-        }
+    if (_reqMutex)
         xSemaphoreGiveRecursive(_reqMutex);
-    }
 }
 #else
 void AsyncHttpClient::lock() const {}
@@ -117,8 +109,8 @@ void AsyncHttpClient::unlock() const {}
 
 bool AsyncHttpClient::isLockHeldByCurrentTask() const {
 #ifdef ARDUINO_ARCH_ESP32
-    return _reqMutexOwner.load(std::memory_order_relaxed) == xTaskGetCurrentTaskHandle() &&
-           _reqMutexDepth.load(std::memory_order_relaxed) > 0;
+    // FreeRTOS already records the owning task of a (recursive) mutex; no manual tracking needed.
+    return _reqMutex && xSemaphoreGetMutexHolder(_reqMutex) == xTaskGetCurrentTaskHandle();
 #else
     return false;
 #endif
@@ -144,6 +136,8 @@ void AsyncHttpClient::_workerTaskThunk(void* param) {
 void AsyncHttpClient::_workerLoop() {
     while (true) {
         _workerBuffer.waitForItem();
+        if (_workerShouldExit.load(std::memory_order_acquire))
+            break;
         WorkerItem item;
         while (_workerBuffer.pop(item)) {
             auto ctx = std::static_pointer_cast<RequestContext>(item.ctx);
@@ -173,6 +167,11 @@ void AsyncHttpClient::_workerLoop() {
             dispatchCallbacks();
         }
     }
+    // Shutdown requested: tell the destructor we are out of the loop, then self-delete so we never
+    // get vTaskDelete()'d while holding _reqMutex. Any items still queued are freed by ~WorkerBuffer.
+    if (_workerDoneSem)
+        xSemaphoreGive(_workerDoneSem);
+    vTaskDelete(nullptr);
 }
 #endif // ARDUINO_ARCH_ESP32
 

src/AsyncHttpClient.h

@@ -224,14 +224,16 @@ class AsyncHttpClient {
 #ifdef ARDUINO_ARCH_ESP32
     WorkerBuffer _workerBuffer;
     TaskHandle_t _workerTaskHandle = nullptr;
+    std::atomic<bool> _workerShouldExit{false}; // set by destructor to request worker shutdown
+    SemaphoreHandle_t _workerDoneSem = nullptr; // worker gives this once it has exited its loop
     static void _workerTaskThunk(void* param);
     void _workerLoop();
 #endif
 
 #ifdef ARDUINO_ARCH_ESP32
-    mutable SemaphoreHandle_t _reqMutex = nullptr; // recursive mutex
-    mutable std::atomic<TaskHandle_t> _reqMutexOwner{nullptr};
-    mutable std::atomic<uint16_t> _reqMutexDepth{0};
+    // Recursive mutex. Ownership/recursion depth are tracked natively by FreeRTOS; query the
+    // holder with xSemaphoreGetMutexHolder() (see isLockHeldByCurrentTask) — no manual bookkeeping.
+    mutable SemaphoreHandle_t _reqMutex = nullptr;
 #endif
 
     // Internal methods

src/WorkerBuffer.cpp

@@ -109,4 +109,9 @@ void WorkerBuffer::waitForItem() {
     xSemaphoreTake(_semaphore, portMAX_DELAY);
 }
 
+void WorkerBuffer::wake() {
+    if (!_semaphore) return;
+    xSemaphoreGive(_semaphore);
+}
+
 #endif // ARDUINO_ARCH_ESP32

src/WorkerBuffer.h

@@ -57,6 +57,10 @@ class WorkerBuffer {
     // Blocks worker task until an item is available.
     void waitForItem();
 
+    // Wakes a worker blocked in waitForItem() without enqueuing anything.
+    // Used to unblock the worker for cooperative shutdown.
+    void wake();
+
   private:
     void enqueue(WorkerItem&& item);