Skip to content

AsyncHttpClientLight: end a body read when the server goes silent or closes - #25141

Merged
s-hadinger merged 3 commits into
arendst:developmentfrom
adamsthws:asynchttpclientlight-read-timeout
Oct 15, 2026
Merged

s-hadinger merged 3 commits into
arendst:developmentfrom
adamsthws:asynchttpclientlight-read-timeout

Conversation

@adamsthws

Copy link
Copy Markdown
Contributor

Description:

Follow-up to #25139, as asked there: the same fix for AsyncHttpClientLight::writeToStreamDataBlock(), used when Tasmota is built with USE_BERRY_WEBCLIENT_ASYNC.

The read runs on a worker task, but runJob() waits for it with no limit, so a read that never returns leaves the device answering nothing, as with the sync client. There are two ways it never returned:

  1. The server goes silent part way through a body without closing (the same case as HttpClientLight: time out reading a body when the server goes silent #25139). The loop now gives up after _tcpTimeout without data and returns HTTPC_ERROR_READ_TIMEOUT. Each piece of data resets the clock, and set_timeouts() still lets a caller wait longer.
  2. The server sends a body with no Content-Length and closes. AsyncTcpAdapter only noticed a close inside read(), and the loop only calls read() when available() > 0, so after the last byte connected() stayed true forever. connected() now peeks at the socket, as WiFiClient::connected() does. Data still waiting keeps it connected, so the body is read to the end first. Without this the timeout alone would turn these responses into an error after 5 s and drop the body.

How to reproduce: build with USE_BERRY_WEBCLIENT_ASYNC. Have a server send the headers (Content-Length: 8192) and the first 1000 bytes, then nothing while keeping the connection open (or send 3000 bytes with no Content-Length and close). Then:

var c = webclient()
c.begin("http://<server>/stall")
c.GET()
c.get_string()   # never returns before this change; returns after 5 s with it

Testing: on an emulated ESP32-C3 (QEMU), tasmota32c3 from development with USE_BERRY_WEBCLIENT_ASYNC, with and without this change. Core 3.3.12.

Server behaviour Before After
Body cut off part way, connection left open (Content-Length, and no length) hangs, device unreachable returns after 5.0 s, device fine
No Content-Length, 3000 bytes, then the connection closes hangs, device unreachable returns the 3000 bytes at once
Full body completes completes
Body in 8 pieces, 2 s apart (and 4.5 s apart) completes completes
Body in 8 pieces, 5.5 s or 7 s apart completes times out after 5 s
7 s apart with set_timeouts(10000) completes completes
Body cut off, connection left open, set_timeouts(10000) hangs returns after 10.0 s
write_file(), body cut off part way hangs returns -11 after 5 s

Free heap after 30 timeouts in a row fell less than after 30 normal downloads, and the normal downloads used the same as before this change.

The same cases on a real ESP32-C3 plug (Sonoff S60TPG), same builds:

Server behaviour Before After
Body cut off part way, connection left open (Content-Length, and no length) device unreachable until power-cycled returns after 5.0 s, device fine
No Content-Length, 3000 bytes, then the connection closes device unreachable until power-cycled returns the 3000 bytes at once
Full body; 8 pieces 2 s apart completes completes
8 pieces 7 s apart not run times out after 5 s; completes with set_timeouts(10000)
Body cut off, connection left open, set_timeouts(10000) not run returns after 10.0 s
write_file(), body cut off part way not run returns -11 after 5 s

tasmota32, tasmota32s3 and tasmota32c3 with USE_BERRY_WEBCLIENT_ASYNC build without warnings. The library is ESP32-only; the ESP8266 tasmota build is unaffected and builds without warnings.

Chunked bodies are not affected by this PR: they are read elsewhere and already time out. They also fail at the end of every body, for a separate reason; I'll send that fix in its own PR.

Related issue (if applicable): #25139

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.8
  • The code change is tested and works with Tasmota core ESP32 V.3.3.12 from Platform 2026.09.50
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@s-hadinger

Copy link
Copy Markdown
Collaborator

Thanks @adamsthws, I ran the PR though Opus 5.5 and there are some additional changes suggested before merge:

1. Please don't call stop() from AsyncTcpAdapter::connected()

connected() used to be a plain flag check; with this PR it can close(fd). It is public and reachable from Berry (webclient.connected() → wc_connected) on the main thread, while an async_get_start() / async_post_start() job is using the same _fd on the worker task. If the peer closes at that moment, the main thread closes the fd under the worker, and since lwIP reuses fd numbers, the worker could end up reading from or writing to an unrelated socket.

Returning false is enough. Every caller that gets false either bails out or reconnects, and connect_fd() already calls stop() first:

bool AsyncHttpClientLight::AsyncTcpAdapter::connected() {
  if (_fd < 0 || !_isConnected) return false;
  // read() only sees the peer close when it is called, and callers only read
  // when available() > 0: peek, so a close after the last byte is noticed.
  // No stop() here: connected() may be called from another task than the worker.
  uint8_t c;
  int r = ::recv(_fd, &c, 1, MSG_PEEK | MSG_DONTWAIT);
  if (r == 0) return false;
  if (r < 0 && errno != EAGAIN && errno != EWOULDBLOCK) return false;
  return true;
}

2. Close the connection when writeToStreamDataBlock() times out

The chunked readers call _transport->stop() and clear _bodyPending before they return HTTPC_ERROR_READ_TIMEOUT, and so does returnError() in the sync HTTPClientLight. The new timeout path doesn't. With Content-Length that's harmless because _bodyPending stays true, but with no Content-Length _bodyPending is false and the stalled connection can be reused by the next request on the same object.

if (avail <= 0) {
  if ((millis() - lastDataTime) > (uint32_t)_tcpTimeout) {
    _transport->stop();
    _bodyPending = false;
    return HTTPC_ERROR_READ_TIMEOUT;
  }
  delay(1);
  continue;
}

3. Same problem in the async path readBodyToStreamString(), non-chunked branch

The identity branch used by async_get_start() / async_post_start() still has if (avail <= 0) { delay(1); continue; } with no timeout and no close detection. With no Content-Length it waits for size_limit bytes, so a server that closes early or goes silent leaves the job RUNNING until the script calls async_abort(). While it is stuck it holds _busy, so any later sync call on the same object blocks the main loop in runJob().

Something like this would match the sync path. A close ends a body that has no Content-Length, and is an error if Content-Length was given:

uint32_t lastDataTime = millis();
while (remaining != 0) {
  if (_asyncAbortReq) return bail(HTTPC_ERROR_CONNECTION_LOST);
  int avail = _transport ? _transport->available() : 0;
  if (avail <= 0) {
    if (!_transport || !_transport->connected()) {
      if (_size > 0) return bail(HTTPC_ERROR_CONNECTION_LOST);  // body cut short
      break;                                                     // no length: close ends the body
    }
    if ((millis() - lastDataTime) > (uint32_t)_tcpTimeout) return bail(HTTPC_ERROR_READ_TIMEOUT);
    delay(1);
    continue;
  }
  // ... existing read/push code ...
  if (r > 0) { /* existing */ lastDataTime = millis(); }
}

…lent

writeToStreamDataBlock() waited for the rest of a body for as long as
connected() held. A server that stops sending part way through without
closing leaves connected() true, so the worker never finishes and the
caller, waiting in runJob(), never returns. Give up after _tcpTimeout
without data and return HTTPC_ERROR_READ_TIMEOUT, as HttpClientLight
now does (arendst#25139).
AsyncTcpAdapter only saw the peer close inside read(), and callers only
read when available() > 0. A body without Content-Length that ends with
the server closing left connected() true for ever, so
writeToStreamDataBlock() never returned. connected() now peeks at the
socket, as WiFiClient::connected() does: data still buffered keeps it
connected, an orderly close after it does not.
- connected() only reports a peer close and no longer calls stop(): it can
  be called from another task than the worker using the socket.
- writeToStreamDataBlock() closes the connection on a read timeout, as the
  chunked readers do, so a stalled connection is not reused.
- readBodyToStreamString(): the identity branch used by async GET/POST now
  times out after _tcpTimeout without data, and notices a close: with no
  Content-Length it ends the body, with one it is a lost connection.
@adamsthws
adamsthws force-pushed the asynchttpclientlight-read-timeout branch from e88ddc3 to 72c67b5 Compare October 15, 2026 12:49
@adamsthws

Copy link
Copy Markdown
Contributor Author

Thanks, all three done in 72c67b5, and the branch is rebased on development (after #25142):

  1. connected() only reports the close, no stop().
  2. The writeToStreamDataBlock() timeout now calls _transport->stop() and clears _bodyPending.
  3. The identity branch of readBodyToStreamString() times out after _tcpTimeout without data and notices a close: with no Content-Length it ends the body, with one it is HTTPC_ERROR_CONNECTION_LOST.

Async results on an emulated ESP32-C3, tasmota32c3 with USE_BERRY_WEBCLIENT_ASYNC, polling async_state() every 200 ms:

Server behaviour development previous PR head now
Full body; chunked; 8 pieces 2 s apart done done done
Content-Length, cut off, left open still running after 40 s still running after 40 s done, last_rx -11 after 5.2 s
No length, 3000 bytes, then close still running after 40 s still running after 40 s done, 3000 bytes
No length, cut off, left open still running after 40 s still running after 40 s done, -11 after 5.1 s
Chunked, cut off done, -11 after 5 s done, -11 after 5 s done, -11 after 5 s

The sync cases are unchanged from before (stalls return -11 after 5 s, full and chunked bodies complete). I couldn't make the reuse in point 2 happen in a test: a stalled request with no Content-Length followed by a second GET on the same object worked with the previous head too, so that one is a safeguard.

One thing I noticed: wc_connected is declared in xdrv_52_3_berry_webclient.ino, but connected isn't in the webclient class map in be_webclient_lib.c, so Berry scripts can't call it at the moment. Point 1 still matters for any C++ caller on the main thread.

@s-hadinger
s-hadinger merged commit ece53a6 into arendst:development Oct 15, 2026
66 checks passed
@adamsthws

Copy link
Copy Markdown
Contributor Author

For context, this came out of Sonoff-OTA, an over-the-air Sonoff S60 to Tasmota converter. The same stalled-download problem turned up in several other projects; the related pull requests, and how each fix was tested, are collected in docs/upstream-contributions.md.

@s-hadinger

Copy link
Copy Markdown
Collaborator

Thanks.
webclient.connected() is fixed in #25145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants