Compare commits

...

7 Commits

Author SHA1 Message Date
J. Nick Koston
60028036ed bot nits 2026-01-30 14:28:53 -06:00
J. Nick Koston
c6a7616de0 be explict 2026-01-30 14:16:16 -06:00
J. Nick Koston
b3d5961ae4 Merge branch 'dev' into http_request_not_finished_till_timeout 2026-01-30 13:59:21 -06:00
J. Nick Koston
30c94c2c11 address bot review comments 2026-01-30 13:58:23 -06:00
J. Nick Koston
f36f171647 add comment 2026-01-30 13:56:18 -06:00
J. Nick Koston
cb91215e03 200,304,204,1xx 2026-01-30 13:52:27 -06:00
J. Nick Koston
2c99652f35 [http_request] Fix requests taking full timeout when response is already complete 2026-01-30 12:34:24 -06:00
5 changed files with 78 additions and 26 deletions

View File

@@ -196,11 +196,14 @@ bool Esp32HostedUpdate::fetch_manifest_() {
int read_or_error = container->read(buf, sizeof(buf));
App.feed_wdt();
yield();
auto result = http_request::http_read_loop_result(read_or_error, last_data_time, read_timeout);
auto result =
http_request::http_read_loop_result(read_or_error, last_data_time, read_timeout, container->is_read_complete());
if (result == http_request::HttpReadLoopResult::RETRY)
continue;
// Note: COMPLETE is currently unreachable since the loop condition checks bytes_read < content_length,
// but this is defensive code in case chunked transfer encoding support is added in the future.
if (result != http_request::HttpReadLoopResult::DATA)
break; // ERROR or TIMEOUT
break; // COMPLETE, ERROR, or TIMEOUT
json_str.append(reinterpret_cast<char *>(buf), read_or_error);
}
container->end();
@@ -321,9 +324,14 @@ bool Esp32HostedUpdate::stream_firmware_to_coprocessor_() {
App.feed_wdt();
yield();
auto result = http_request::http_read_loop_result(read_or_error, last_data_time, read_timeout);
auto result =
http_request::http_read_loop_result(read_or_error, last_data_time, read_timeout, container->is_read_complete());
if (result == http_request::HttpReadLoopResult::RETRY)
continue;
// Note: COMPLETE is currently unreachable since the loop condition checks bytes_read < content_length,
// but this is defensive code in case chunked transfer encoding support is added in the future.
if (result == http_request::HttpReadLoopResult::COMPLETE)
break;
if (result != http_request::HttpReadLoopResult::DATA) {
if (result == http_request::HttpReadLoopResult::TIMEOUT) {
ESP_LOGE(TAG, "Timeout reading firmware data");

View File

@@ -26,6 +26,7 @@ struct Header {
enum HttpStatus {
HTTP_STATUS_OK = 200,
HTTP_STATUS_NO_CONTENT = 204,
HTTP_STATUS_RESET_CONTENT = 205,
HTTP_STATUS_PARTIAL_CONTENT = 206,
/* 3xx - Redirection */
@@ -126,19 +127,21 @@ struct HttpReadResult {
/// Result of processing a non-blocking read with timeout (for manual loops)
enum class HttpReadLoopResult : uint8_t {
DATA, ///< Data was read, process it
RETRY, ///< No data yet, already delayed, caller should continue loop
ERROR, ///< Read error, caller should exit loop
TIMEOUT, ///< Timeout waiting for data, caller should exit loop
DATA, ///< Data was read, process it
COMPLETE, ///< All content has been read, caller should exit loop
RETRY, ///< No data yet, already delayed, caller should continue loop
ERROR, ///< Read error, caller should exit loop
TIMEOUT, ///< Timeout waiting for data, caller should exit loop
};
/// Process a read result with timeout tracking and delay handling
/// @param bytes_read_or_error Return value from read() - positive for bytes read, negative for error
/// @param last_data_time Time of last successful read, updated when data received
/// @param timeout_ms Maximum time to wait for data
/// @return DATA if data received, RETRY if should continue loop, ERROR/TIMEOUT if should exit
inline HttpReadLoopResult http_read_loop_result(int bytes_read_or_error, uint32_t &last_data_time,
uint32_t timeout_ms) {
/// @param is_read_complete Whether all expected content has been read (from HttpContainer::is_read_complete())
/// @return How the caller should proceed - see HttpReadLoopResult enum
inline HttpReadLoopResult http_read_loop_result(int bytes_read_or_error, uint32_t &last_data_time, uint32_t timeout_ms,
bool is_read_complete) {
if (bytes_read_or_error > 0) {
last_data_time = millis();
return HttpReadLoopResult::DATA;
@@ -146,7 +149,10 @@ inline HttpReadLoopResult http_read_loop_result(int bytes_read_or_error, uint32_
if (bytes_read_or_error < 0) {
return HttpReadLoopResult::ERROR;
}
// bytes_read_or_error == 0: no data available yet
// bytes_read_or_error == 0: either "no data yet" or "all content read"
if (is_read_complete) {
return HttpReadLoopResult::COMPLETE;
}
if (millis() - last_data_time >= timeout_ms) {
return HttpReadLoopResult::TIMEOUT;
}
@@ -159,9 +165,9 @@ class HttpRequestComponent;
class HttpContainer : public Parented<HttpRequestComponent> {
public:
virtual ~HttpContainer() = default;
size_t content_length;
int status_code;
uint32_t duration_ms;
size_t content_length{0};
int status_code{-1}; ///< -1 indicates no response received yet
uint32_t duration_ms{0};
/**
* @brief Read data from the HTTP response body.
@@ -194,9 +200,24 @@ class HttpContainer : public Parented<HttpRequestComponent> {
virtual void end() = 0;
void set_secure(bool secure) { this->secure_ = secure; }
void set_chunked(bool chunked) { this->is_chunked_ = chunked; }
size_t get_bytes_read() const { return this->bytes_read_; }
/// Check if all expected content has been read
/// For chunked responses, returns false (completion detected via read() returning error/EOF)
bool is_read_complete() const {
// Per RFC 9112, these responses have no body:
// - 1xx (Informational), 204 No Content, 205 Reset Content, 304 Not Modified
if ((this->status_code >= 100 && this->status_code < 200) || this->status_code == HTTP_STATUS_NO_CONTENT ||
this->status_code == HTTP_STATUS_RESET_CONTENT || this->status_code == HTTP_STATUS_NOT_MODIFIED) {
return true;
}
// For non-chunked responses, complete when bytes_read >= content_length
// This handles both Content-Length: 0 and Content-Length: N cases
return !this->is_chunked_ && this->bytes_read_ >= this->content_length;
}
/**
* @brief Get response headers.
*
@@ -209,6 +230,7 @@ class HttpContainer : public Parented<HttpRequestComponent> {
protected:
size_t bytes_read_{0};
bool secure_{false};
bool is_chunked_{false}; ///< True if response uses chunked transfer encoding
std::map<std::string, std::list<std::string>> response_headers_{};
};
@@ -219,7 +241,7 @@ class HttpContainer : public Parented<HttpRequestComponent> {
/// @param total_size Total bytes to read
/// @param chunk_size Maximum bytes per read call
/// @param timeout_ms Read timeout in milliseconds
/// @return HttpReadResult with status and error_code on failure
/// @return HttpReadResult with status and error_code on failure; use container->get_bytes_read() for total bytes read
inline HttpReadResult http_read_fully(HttpContainer *container, uint8_t *buffer, size_t total_size, size_t chunk_size,
uint32_t timeout_ms) {
size_t read_index = 0;
@@ -231,9 +253,11 @@ inline HttpReadResult http_read_fully(HttpContainer *container, uint8_t *buffer,
App.feed_wdt();
yield();
auto result = http_read_loop_result(read_bytes_or_error, last_data_time, timeout_ms);
auto result = http_read_loop_result(read_bytes_or_error, last_data_time, timeout_ms, container->is_read_complete());
if (result == HttpReadLoopResult::RETRY)
continue;
if (result == HttpReadLoopResult::COMPLETE)
break; // Server sent less data than requested, but transfer is complete
if (result == HttpReadLoopResult::ERROR)
return {HttpReadStatus::ERROR, read_bytes_or_error};
if (result == HttpReadLoopResult::TIMEOUT)
@@ -393,11 +417,12 @@ template<typename... Ts> class HttpRequestSendAction : public Action<Ts...> {
int read_or_error = container->read(buf + read_index, std::min<size_t>(max_length - read_index, 512));
App.feed_wdt();
yield();
auto result = http_read_loop_result(read_or_error, last_data_time, read_timeout);
auto result =
http_read_loop_result(read_or_error, last_data_time, read_timeout, container->is_read_complete());
if (result == HttpReadLoopResult::RETRY)
continue;
if (result != HttpReadLoopResult::DATA)
break; // ERROR or TIMEOUT
break; // COMPLETE, ERROR, or TIMEOUT
read_index += read_or_error;
}
response_body.reserve(read_index);

View File

@@ -135,9 +135,23 @@ std::shared_ptr<HttpContainer> HttpRequestArduino::perform(const std::string &ur
// When cast to size_t, -1 becomes SIZE_MAX (4294967295 on 32-bit).
// The read() method handles this: bytes_read_ can never reach SIZE_MAX, so the
// early return check (bytes_read_ >= content_length) will never trigger.
//
// TODO: Chunked transfer encoding is NOT properly supported on Arduino.
// The implementation in #7884 was incomplete - it only works correctly on ESP-IDF where
// esp_http_client_read() decodes chunks internally. On Arduino, using getStreamPtr()
// returns raw TCP data with chunk framing (e.g., "12a\r\n{json}\r\n0\r\n\r\n") instead
// of decoded content. This wasn't noticed because requests would complete and payloads
// were only examined on IDF. The long transfer times were also masked by the misleading
// "HTTP on Arduino version >= 3.1 is **very** slow" warning above. This causes two issues:
// 1. Response body is corrupted - contains chunk size headers mixed with data
// 2. Cannot detect end of transfer - connection stays open (keep-alive), causing timeout
// The proper fix would be to use getString() for chunked responses, which decodes chunks
// internally, but this buffers the entire response in memory.
int content_length = container->client_.getSize();
ESP_LOGD(TAG, "Content-Length: %d", content_length);
container->content_length = (size_t) content_length;
// -1 (SIZE_MAX when cast to size_t) means chunked transfer encoding
container->set_chunked(content_length == -1);
container->duration_ms = millis() - start;
return container;
@@ -178,9 +192,9 @@ int HttpContainerArduino::read(uint8_t *buf, size_t max_len) {
if (bufsize == 0) {
this->duration_ms += (millis() - start);
// Check if we've read all expected content (only valid when content_length is known and not SIZE_MAX)
// For chunked encoding (content_length == SIZE_MAX), we can't use this check
if (this->content_length > 0 && this->bytes_read_ >= this->content_length) {
// Check if we've read all expected content (non-chunked only)
// For chunked encoding (content_length == SIZE_MAX), is_read_complete() returns false
if (this->is_read_complete()) {
return 0; // All content read successfully
}
// No data available - check if connection is still open

View File

@@ -160,6 +160,7 @@ std::shared_ptr<HttpContainer> HttpRequestIDF::perform(const std::string &url, c
// esp_http_client_fetch_headers() returns 0 for chunked transfer encoding (no Content-Length header).
// The read() method handles content_length == 0 specially to support chunked responses.
container->content_length = esp_http_client_fetch_headers(client);
container->set_chunked(esp_http_client_is_chunked_response(client));
container->feed_wdt();
container->status_code = esp_http_client_get_status_code(client);
container->feed_wdt();
@@ -195,6 +196,7 @@ std::shared_ptr<HttpContainer> HttpRequestIDF::perform(const std::string &url, c
container->feed_wdt();
container->content_length = esp_http_client_fetch_headers(client);
container->set_chunked(esp_http_client_is_chunked_response(client));
container->feed_wdt();
container->status_code = esp_http_client_get_status_code(client);
container->feed_wdt();
@@ -239,10 +241,9 @@ int HttpContainerIDF::read(uint8_t *buf, size_t max_len) {
const uint32_t start = millis();
watchdog::WatchdogManager wdm(this->parent_->get_watchdog_timeout());
// Check if we've already read all expected content
// Skip this check when content_length is 0 (chunked transfer encoding or unknown length)
// For chunked responses, esp_http_client_read() will return 0 when all data is received
if (this->content_length > 0 && this->bytes_read_ >= this->content_length) {
// Check if we've already read all expected content (non-chunked only)
// For chunked responses (content_length == 0), esp_http_client_read() handles EOF
if (this->is_read_complete()) {
return 0; // All content read successfully
}

View File

@@ -130,9 +130,13 @@ uint8_t OtaHttpRequestComponent::do_ota_() {
App.feed_wdt();
yield();
auto result = http_read_loop_result(bufsize_or_error, last_data_time, read_timeout);
auto result = http_read_loop_result(bufsize_or_error, last_data_time, read_timeout, container->is_read_complete());
if (result == HttpReadLoopResult::RETRY)
continue;
// Note: COMPLETE is currently unreachable since the loop condition checks bytes_read < content_length,
// but this is defensive code in case chunked transfer encoding support is added for OTA in the future.
if (result == HttpReadLoopResult::COMPLETE)
break;
if (result != HttpReadLoopResult::DATA) {
if (result == HttpReadLoopResult::TIMEOUT) {
ESP_LOGE(TAG, "Timeout reading data");