# Code Review: `USE_BERRY_WEBCLIENT_ASYNC` async web client

**Status:** Not mergeable as-is. Device-crashing/hanging defects present.
**Scope reviewed:**
- `lib/libesp32/AsyncHttpClientLight/src/AsyncHttpClientLight.{h,cpp}` (~1600 lines)
- `tasmota/tasmota_xdrv_driver/xdrv_52_3_berry_webclient.ino` (Berry bindings)
- `lib/libesp32/berry_tasmota/src/be_webclient_lib.c` (class definition)
- Cross-checked against `lib/lib_ssl/tls_mini/src/WiFiClientSecureLightBearSSL.{h,cpp}`

---

## What it is

A new, self-contained async HTTP/HTTPS client (`AsyncHttpClientLight`) by an external
contributor (HexaMaster), wired into the existing Berry `webclient` class as a
compile-time alternative to `HTTPClientLight`. It adds:

- `async_get_start()`, `async_post_start()`, `async_state()`, `async_abort()`
- TLS public-key pinning: `tls_pin_pubkey()`, `tls_clear_pins()`, `tls_set_rsa_only()`

It is **off by default** (commented out in `my_user_config.h`), which is the right call
given the state below. Existing sync Berry scripts are meant to remain compatible.

---

## Architecture concern: a FreeRTOS task per request

Every operation — including **synchronous** `GET/POST/getString/writeToStream` — runs by
spawning a new FreeRTOS task (`WorkerTask`, 12 KB stack via `ASYNCHTTP_WKR_STACK`) and then
the caller busy-waits on a semaphore (`runJob`).

- For **sync** calls this is pure overhead: the main loop is blocked the whole time anyway
  (it spins on `xSemaphoreTake` + `delay(0)`), so you pay 12 KB stack + task create/teardown
  per request and gain nothing.
- For **async** it legitimately offloads blocking I/O to the worker.

The cost model (a fresh 12 KB-stack task for each job, low priority `tskIDLE_PRIORITY+1`) is
heavy for an ESP32 and atypical for Tasmota, which normally does cooperative networking on
the main loop / `fast_loop`. **This central design decision deserves a discussion before the
feature goes further.**

---

## Critical bugs (blocking)

### C1. Use-after-free on destruction while a job is in flight
`~AsyncHttpClientLight()` calls `end()` (which does `_transport->stop()` + `_transport.reset()`)
and deletes `_busy`/`_asyncDone`, but it **never aborts or joins a running worker task**.

If the Berry `webclient` object is GC'd or `deinit`'d while `async_state()` is still
`RUNNING` (script drops its reference, or calls `close()`/reuses incorrectly), the worker
keeps dereferencing `self->_transport`, `self->_asyncBody`, and the freed semaphores.
Guaranteed crash.

**Fix:** destructor (and `end()`/`deinit`) must set `_asyncAbortReq`, wait for the worker to
exit, then free.

### C2. Deadlock if response accessors are called while `RUNNING`
`getString()` / `writeToStream()` only take the fast path when `_asyncState == ASYNC_DONE`.
If called while still `RUNNING`, they fall through to `runJob()` →
`xSemaphoreTake(_busy, portMAX_DELAY)`. But `_busy` is only released inside `asyncState()` on
the *same* (now-blocked) main thread. The main loop hangs forever → WDT reset.

The docs say "don't do this," but a library should not deadlock the device on misuse.

### C3. No timeout/abort in the sync body reader → infinite loop
`writeToStreamDataBlock()` (used by sync `getString`, `writeToStream`, `get_bytes`,
`write_file`, `write_flash`) loops:

```cpp
while (_transport && _transport->connected() && (len > 0 || len == -1)) {
  int avail = _transport->available();
  if (avail <= 0) { delay(1); continue; }   // no timeout, no abort check
  ...
}
```

With an identity response, keep-alive, and no `Content-Length` (`_size == -1`), a server that
holds the socket open with no further data spins forever. The readme acknowledges this
"identity with no data" hazard, but only the **async** path (`readBodyToStreamString`) guards
it (abort flag + 16 KB cap). The sync path has neither a timeout nor an abort check.

### C4. Stale `Content-Length` on a 302/303 redirect-to-GET
In `Job_SendBuf`/`Job_SendStr`, each redirect iteration does **not** reset `_headers`;
`Content-Length` is only (re)added when a payload is present. On a 302/303 the code sets
`payload=nullptr`/`size=0` and switches to GET, so the previously injected
`Content-Length: N` stays in `_headers` and is sent on the bodyless GET. The server then
waits for a body that never arrives → hang/timeout.

Related: `beginInternal()` (reached via `setURL()` for absolute redirect targets) never clears
`_headers`, contradicting the readme's "headers reset after redirect."

### C5. Stale async body returned after a later sync call
`_asyncState` is never reset to `IDLE` once `DONE`, and sync `GET()` doesn't touch it. After
one async job, a subsequent **sync** `GET()` on the same instance populates `_size`/transport,
but `getString()` still sees `_asyncState == ASYNC_DONE` and returns the **old** `_asyncBody`.

**Fix:** any new request (sync or async) must invalidate the DONE fast-path.

---

## High / medium issues

### H1. Async POST from `bytes` does not copy the payload
`asyncPOSTStart(uint8_t*, size)` stores the raw pointer; the worker reads it later. The
binding guards GC by stashing the bytes object in `.__async_hold`, but nothing prevents the
script from **mutating/resizing** that `bytes` while the job runs, which can realloc the
buffer → worker reads freed memory. The `String` overload copies (safe); the buffer overload
should too, or the contract must be enforced.

### H2. Unbounded RAM growth for chunked sync responses
`wc_getstring`/`wc_getbytes` guard with `if (sz >= 32767)` using `getSize()`, but a chunked
response reports `_size == -1`, so the guard is bypassed and `Job_GetString` accumulates the
entire body into a `StreamString` with no cap → OOM on a large chunked download. (The async
path is capped at 16 KB; the sync path is not.)

### H3. IPv4-only HTTP transport
`AsyncTcpAdapter` uses `sockaddr_in` / `AF_INET` and `(in_addr_t)ip`. Tasmota supports IPv6;
the previous `WiFiClient`-based path did too. Plain-HTTP async over IPv6 will not work.

### M1. TLS transmit buffer effectively 85 bytes
`new BearSSL::WiFiClientSecure_light(16384, 0)` → `setBufferSizes(recv, xmit)` yields
`_iobuf_out_size = 0 + MAX_OUT_OVERHEAD(85)`. So a 16 KB receive buffer (heavy heap, allocated
up front) is paired with a near-zero send buffer (every TLS record carries almost no app data
— poor POST throughput). Tasmota normally uses balanced sizes (e.g. 1024/1024). Needs tuning.

### M2. `SO_LINGER` RST-close is unconditional and aggressive
`AsyncTcpAdapter::stop()` always sets `l_onoff=1, l_linger=0`, forcing a TCP RST on every close
to cut TIME_WAIT. That can discard unread data and surface as connection-reset on the peer.
Fine as an *option*, questionable as an unconditional default.

### M3. Default HTTPS is unauthenticated
With no pins, `BearSslAdapter` calls `setPubKeyFingerprint(any, any, allow_all=true)` — accepts
any server key (MITM-open). This matches existing Tasmota `_light` behavior, so it is not a
regression, but combined with SHA-1 pubkey pinning it should be documented clearly that
security requires explicit pinning.

---

## Code quality / process

- **License mismatch:** source headers say `GPL-3.0-or-later`; `library.json` says
  `"license": "MIT"`. Must be reconciled before merge.
- **`library.json`** description ("Async extension for HttpClientLight by Stephan Hadinger")
  misattributes authorship, and the `authors` field is a single object, not the array
  PlatformIO expects.
- **Non-English comments and typos** throughout (Slovak/Czech: "telo", "drž payload, aby ho GC
  nevyhodil kým beží job", "za chunkom"; typos: "wheen", "writin", "succesful",
  "contetn-length", "metho"). Tasmota upstream is English-only.
- **Massive `#ifdef USE_BERRY_WEBCLIENT_ASYNC` forking** in `xdrv_52_3` (~20 duplicated
  `wc_getclient` blocks). Selecting the client type with a single typedef/helper would remove
  almost all of it and shrink the diff dramatically.
- **`asyncState()` is declared `const` but `const_cast`s `this`** to mutate state and free
  resources — misleading signature.
- Worker task names are fixed strings, so multiple concurrent clients collide (harmless, but
  indicative).

---

## What's actually fine

- lwIP non-blocking connect with `select()` timeout.
- Error-code table mirroring `HTTPClientLight`.
- Chunked / identity decoding.
- Abort-aware async body reader with a hard 16 KB cap.
- Holding the Berry payload via `.__async_hold` to prevent GC.
- Cross-thread result handoff is correctly ordered: the worker writes results **before**
  `xSemaphoreGive`, and the main thread reads them **after** `xSemaphoreTake`, so the published
  `_asyncHttpCode` / `_asyncRxBytes` / `_asyncBody` are not racy *as long as the documented call
  sequence is followed*.
- Feature is default-off, which is appropriate.