Merge branch 'refactor/tcp_transport' into 'master'

tcp_transport: Fix error propagation to higher layers

Closes IDF-1291

See merge request espressif/esp-idf!16394

components/esp_common/src/esp_err_to_name.c

@@ -53,6 +53,9 @@
 #if __has_include("esp_tls_errors.h")
 #include "esp_tls_errors.h"
 #endif
+#if __has_include("esp_transport.h")
+#include "esp_transport.h"
+#endif
 #if __has_include("esp_wifi.h")
 #include "esp_wifi.h"
 #endif
@@ -796,6 +799,23 @@ static const esp_err_msg_t esp_err_msg_table[] = {
 #   endif
 #   ifdef      ESP_ERR_MEMPROT_AREA_INVALID
     ERR_TBL_IT(ESP_ERR_MEMPROT_AREA_INVALID),                   /* 53255 0xd007 */
+#   endif
+    // components/tcp_transport/include/esp_transport.h
+#   ifdef      ESP_ERR_TCP_TRANSPORT_BASE
+    ERR_TBL_IT(ESP_ERR_TCP_TRANSPORT_BASE),                     /* 57344 0xe000 Starting number of TCP Transport error codes */
+#   endif
+#   ifdef      ESP_ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT
+    ERR_TBL_IT(ESP_ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT),       /* 57345 0xe001 Connection has timed out */
+#   endif
+#   ifdef      ESP_ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN
+    ERR_TBL_IT(ESP_ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN), /* 57346 0xe002 Read FIN from peer and the connection
+                                                                                has closed (in a clean way) */
+#   endif
+#   ifdef      ESP_ERR_TCP_TRANSPORT_CONNECTION_FAILED
+    ERR_TBL_IT(ESP_ERR_TCP_TRANSPORT_CONNECTION_FAILED),        /* 57347 0xe003 Failed to connect to the peer */
+#   endif
+#   ifdef      ESP_ERR_TCP_TRANSPORT_NO_MEM
+    ERR_TBL_IT(ESP_ERR_TCP_TRANSPORT_NO_MEM),                   /* 57348 0xe004 Memory allocation failed */
 #   endif
 };
 #endif //CONFIG_ESP_ERR_TO_NAME_LOOKUP

components/esp_http_client/esp_http_client.c

@@ -1063,24 +1063,30 @@ int esp_http_client_read(esp_http_client_handle_t client, char *buffer, int len)
         if (rlen <= 0) {
             if (errno != 0) {
                 esp_log_level_t sev = ESP_LOG_WARN;
-                /* On connection close from server, recv should ideally return 0 but we have error conversion
-                 * in `tcp_transport` SSL layer which translates it `-1` and hence below additional checks */
-                if (rlen == -1 && errno == ENOTCONN && client->response->is_chunked) {
+                /* Check for cleanly closed connection */
+                if (rlen == ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN && client->response->is_chunked) {
                     /* Explicit call to parser for invoking `message_complete` callback */
                     http_parser_execute(client->parser, client->parser_settings, res_buffer->data, 0);
                     /* ...and lowering the message severity, as closed connection from server side is expected in chunked transport */
                     sev = ESP_LOG_DEBUG;
                 }
                 ESP_LOG_LEVEL(sev, TAG, "esp_transport_read returned:%d and errno:%d ", rlen, errno);
             }
-#ifdef CONFIG_ESP_HTTP_CLIENT_ENABLE_HTTPS
-            if (rlen == ESP_TLS_ERR_SSL_WANT_READ || errno == EAGAIN) {
-#else
-            if (errno == EAGAIN) {
-#endif
-                ESP_LOGD(TAG, "Received EAGAIN! rlen = %d, errno %d", rlen, errno);
-                return ridx;
+
+            if (rlen == ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT) {
+                ESP_LOGD(TAG, "Connection timed out before data was ready!");
+                /* Returning the number of bytes read upto the point where connection timed out */
+                if (ridx) {
+                    return ridx;
+                }
+                return -ESP_ERR_HTTP_EAGAIN;
             }
+
+            if (rlen != ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN) {
+                esp_err_t err = esp_transport_translate_error(rlen);
+                ESP_LOGE(TAG, "transport_read: error - %d | %s", err, esp_err_to_name(err));
+            }
+
             if (rlen < 0 && ridx == 0 && !esp_http_client_is_complete_data_received(client)) {
                 http_dispatch_event(client, HTTP_EVENT_ERROR, esp_transport_get_error_handle(client->transport), 0);
                 return ESP_FAIL;
@@ -1142,8 +1148,9 @@ esp_err_t esp_http_client_perform(esp_http_client_handle_t client)
                 /* Disable caching response body, as data should
                  * be handled by application event handler */
                 client->cache_data_in_fetch_hdr = 0;
-                if (esp_http_client_fetch_headers(client) < 0) {
-                    if (client->is_async && errno == EAGAIN) {
+                int64_t ret = esp_http_client_fetch_headers(client);
+                if (ret < 0) {
+                    if ((client->is_async && errno == EAGAIN) || ret == -ESP_ERR_HTTP_EAGAIN) {
                         return ESP_ERR_HTTP_EAGAIN;
                     }
                     /* Enable caching after error condition because next
@@ -1219,6 +1226,10 @@ int64_t esp_http_client_fetch_headers(esp_http_client_handle_t client)
     while (client->state < HTTP_STATE_RES_COMPLETE_HEADER) {
         buffer->len = esp_transport_read(client->transport, buffer->data, client->buffer_size_rx, client->timeout_ms);
         if (buffer->len <= 0) {
+            if (buffer->len == ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT) {
+                ESP_LOGW(TAG, "Connection timed out before data was ready!");
+                return -ESP_ERR_HTTP_EAGAIN;
+            }
             return ESP_FAIL;
         }
         http_parser_execute(client->parser, client->parser_settings, buffer->data, buffer->len);

components/esp_http_client/include/esp_http_client.h

@@ -427,6 +427,7 @@ int esp_http_client_write(esp_http_client_handle_t client, const char *buffer, i
  * @return
  *     - (0) if stream doesn't contain content-length header, or chunked encoding (checked by `esp_http_client_is_chunked` response)
  *     - (-1: ESP_FAIL) if any errors
+ *     - (-ESP_ERR_HTTP_EAGAIN = -0x7007) if call is timed-out before any data was ready
  *     - Download data length defined by content-length header
  */
 int64_t esp_http_client_fetch_headers(esp_http_client_handle_t client);
@@ -451,6 +452,8 @@ bool esp_http_client_is_chunked_response(esp_http_client_handle_t client);
  * @return
  *     - (-1) if any errors
  *     - Length of data was read
+ *
+ * @note  (-ESP_ERR_HTTP_EAGAIN = -0x7007) is returned when call is timed-out before any data was ready
  */
 int esp_http_client_read(esp_http_client_handle_t client, char *buffer, int len);
 

components/esp_https_ota/src/esp_https_ota.c

@@ -361,11 +361,11 @@ static esp_err_t read_header(esp_https_ota_t *handle)
         data_read = esp_http_client_read(handle->http_client,
                                           (handle->ota_upgrade_buf + bytes_read),
                                           data_read_size);
-        /*
-         * As esp_http_client_read doesn't return negative error code if select fails, we rely on
-         * `errno` to check for underlying transport connectivity closure if any
-         */
-        if (errno == ENOTCONN || errno == ECONNRESET || errno == ECONNABORTED || data_read < 0) {
+        if (data_read < 0) {
+            if (data_read == -ESP_ERR_HTTP_EAGAIN) {
+                ESP_LOGD(TAG, "ESP_ERR_HTTP_EAGAIN invoked: Call timed out before data was ready");
+                continue;
+            }
             ESP_LOGE(TAG, "Connection closed, errno = %d", errno);
             break;
         }
@@ -491,19 +491,9 @@ esp_err_t esp_https_ota_perform(esp_https_ota_handle_t https_ota_handle)
                  *  esp_http_client_is_complete_data_received is added to check whether
                  *  complete image is received.
                  */
-                bool is_recv_complete = esp_http_client_is_complete_data_received(handle->http_client);
-                /*
-                 * As esp_http_client_read doesn't return negative error code if select fails, we rely on
-                 * `errno` to check for underlying transport connectivity closure if any.
-                 * Incase the complete data has not been received but the server has sent
-                 * an ENOTCONN or ECONNRESET, failure is returned. We close with success
-                 * if complete data has been received.
-                 */
-                if ((errno == ENOTCONN || errno == ECONNRESET || errno == ECONNABORTED) && !is_recv_complete) {
-                    ESP_LOGE(TAG, "Connection closed, errno = %d", errno);
+                if (!esp_http_client_is_complete_data_received(handle->http_client)) {
+                    ESP_LOGE(TAG, "Connection closed before complete data was received!");
                     return ESP_FAIL;
-                } else if (!is_recv_complete) {
-                    return ESP_ERR_HTTPS_OTA_IN_PROGRESS;
                 }
                 ESP_LOGD(TAG, "Connection closed");
             } else if (data_read > 0) {
@@ -523,6 +513,10 @@ esp_err_t esp_https_ota_perform(esp_https_ota_handle_t https_ota_handle)
 #endif // CONFIG_ESP_HTTPS_OTA_DECRYPT_CB
                 return _ota_write(handle, data_buf, data_len);
             } else {
+                if (data_read == -ESP_ERR_HTTP_EAGAIN) {
+                    ESP_LOGD(TAG, "ESP_ERR_HTTP_EAGAIN invoked: Call timed out before data was ready");
+                    return ESP_ERR_HTTPS_OTA_IN_PROGRESS;
+                }
                 ESP_LOGE(TAG, "data read %d, errno %d", data_read, errno);
                 return ESP_FAIL;
             }

components/tcp_transport/include/esp_transport.h

@@ -37,6 +37,22 @@ typedef esp_transport_handle_t (*payload_transfer_func)(esp_transport_handle_t);
 
 typedef struct esp_tls_last_error* esp_tls_error_handle_t;
 
+/**
+ * @brief Error types for TCP connection issues not covered in socket's errno
+ */
+enum esp_tcp_transport_err_t {
+    ERR_TCP_TRANSPORT_NO_MEM = -3,
+    ERR_TCP_TRANSPORT_CONNECTION_FAILED = -2,
+    ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN = -1,
+    ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT = 0,
+};
+
+#define ESP_ERR_TCP_TRANSPORT_BASE                      (0xe000)                          /*!< Starting number of TCP Transport error codes */
+#define ESP_ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT        (ESP_ERR_TCP_TRANSPORT_BASE + 1)  /*!< Connection has timed out */
+#define ESP_ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN  (ESP_ERR_TCP_TRANSPORT_BASE + 2)  /*!< Read FIN from peer and the connection has closed (in a clean way) */
+#define ESP_ERR_TCP_TRANSPORT_CONNECTION_FAILED         (ESP_ERR_TCP_TRANSPORT_BASE + 3)  /*!< Failed to connect to the peer */
+#define ESP_ERR_TCP_TRANSPORT_NO_MEM                    (ESP_ERR_TCP_TRANSPORT_BASE + 4)  /*!< Memory allocation failed */
+
 /**
  * @brief      Create transport list
  *
@@ -169,7 +185,11 @@ int esp_transport_connect_async(esp_transport_handle_t t, const char *host, int
  *
  * @return
  *  - Number of bytes was read
- *  - (-1) if there are any errors, should check errno
+ *  - 0    Read timed-out
+ *  - (<0) For other errors
+ *
+ * @note: Please refer to the enum `esp_tcp_transport_err_t` for all the possible return values
+ *
  */
 int esp_transport_read(esp_transport_handle_t t, char *buffer, int len, int timeout_ms);
 
@@ -334,6 +354,15 @@ esp_tls_error_handle_t esp_transport_get_error_handle(esp_transport_handle_t t);
  */
 int esp_transport_get_errno(esp_transport_handle_t t);
 
+/**
+ * @brief Translates the TCP transport error codes to esp_err_t error codes
+ *
+ * @param[in] error TCP Transport specific error code
+ *
+ * @return Corresponding esp_err_t based error code
+ */
+esp_err_t esp_transport_translate_error(enum esp_tcp_transport_err_t error);
+
 #ifdef __cplusplus
 }
 #endif

components/tcp_transport/private_include/esp_transport_internal.h

@@ -45,18 +45,6 @@ struct esp_transport_item_t {
     STAILQ_ENTRY(esp_transport_item_t) next;
 };
 
-/**
- * @brief Internal error types for TCP connection issues not covered in socket's errno
- */
-enum tcp_transport_errors {
-    ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT,
-    ERR_TCP_TRANSPORT_CANNOT_RESOLVE_HOSTNAME,
-    ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN,
-    ERR_TCP_TRANSPORT_CONNECTION_FAILED,
-    ERR_TCP_TRANSPORT_SETOPT_FAILED,
-    ERR_TCP_TRANSPORT_NO_MEM,
-};
-
 /**
  * @brief      Captures internal tcp connection error
  *
@@ -67,7 +55,7 @@ enum tcp_transport_errors {
  * @param[in] error Internal tcp-transport's error
  *
  */
-void capture_tcp_transport_error(esp_transport_handle_t t, enum tcp_transport_errors error);
+void capture_tcp_transport_error(esp_transport_handle_t t, enum esp_tcp_transport_err_t error);
 
 /**
  * @brief Returns underlying socket for the supplied transport handle

components/tcp_transport/transport.c

@@ -318,25 +318,19 @@ int esp_transport_get_errno(esp_transport_handle_t t)
     return -1;
 }
 
-void capture_tcp_transport_error(esp_transport_handle_t t, enum tcp_transport_errors error)
+void capture_tcp_transport_error(esp_transport_handle_t t, enum esp_tcp_transport_err_t error)
 {
     esp_tls_last_error_t *err_handle = esp_transport_get_error_handle(t);
     switch (error) {
-        case ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT:
-            err_handle->last_error = ESP_ERR_ESP_TLS_CONNECTION_TIMEOUT;
-            break;
-        case ERR_TCP_TRANSPORT_CANNOT_RESOLVE_HOSTNAME:
-            err_handle->last_error = ESP_ERR_ESP_TLS_CANNOT_RESOLVE_HOSTNAME;
-            break;
         case ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN:
             err_handle->last_error = ESP_ERR_ESP_TLS_TCP_CLOSED_FIN;
             break;
+        case ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT:
+            err_handle->last_error = ESP_ERR_ESP_TLS_CONNECTION_TIMEOUT;
+            break;
         case ERR_TCP_TRANSPORT_CONNECTION_FAILED:
             err_handle->last_error = ESP_ERR_ESP_TLS_FAILED_CONNECT_TO_HOST;
             break;
-        case ERR_TCP_TRANSPORT_SETOPT_FAILED:
-            err_handle->last_error = ESP_ERR_ESP_TLS_SOCKET_SETOPT_FAILED;
-            break;
         case ERR_TCP_TRANSPORT_NO_MEM:
             err_handle->last_error = ESP_ERR_NO_MEM;
             break;
@@ -368,3 +362,24 @@ int esp_transport_get_socket(esp_transport_handle_t t)
     }
     return -1;
 }
+
+esp_err_t esp_transport_translate_error(enum esp_tcp_transport_err_t error)
+{
+    esp_err_t err = ESP_FAIL;
+    switch (error) {
+        case ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN:
+            err = ESP_ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN;
+            break;
+        case ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT:
+            err = ESP_ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT;
+            break;
+        case ERR_TCP_TRANSPORT_CONNECTION_FAILED:
+            err = ESP_ERR_TCP_TRANSPORT_CONNECTION_FAILED;
+            break;
+        case ERR_TCP_TRANSPORT_NO_MEM:
+            err = ESP_ERR_TCP_TRANSPORT_NO_MEM;
+            break;
+    }
+
+    return err;
+}

components/tcp_transport/transport_ssl.c

@@ -174,6 +174,8 @@ static int base_poll_read(esp_transport_handle_t t, int timeout_ms)
         esp_transport_capture_errno(t, sock_errno);
         ESP_LOGE(TAG, "poll_read select error %d, errno = %s, fd = %d", sock_errno, strerror(sock_errno), ssl->sockfd);
         ret = -1;
+    } else if (ret == 0) {
+        ESP_LOGD(TAG, "poll_read: select - Timeout before any socket was ready!");
     }
     return ret;
 }
@@ -197,6 +199,8 @@ static int base_poll_write(esp_transport_handle_t t, int timeout_ms)
         esp_transport_capture_errno(t, sock_errno);
         ESP_LOGE(TAG, "poll_write select error %d, errno = %s, fd = %d", sock_errno, strerror(sock_errno), ssl->sockfd);
         ret = -1;
+    } else if (ret == 0) {
+        ESP_LOGD(TAG, "poll_write: select - Timeout before any socket was ready!");
     }
     return ret;
 }
@@ -242,51 +246,64 @@ static int tcp_write(esp_transport_handle_t t, const char *buffer, int len, int
 
 static int ssl_read(esp_transport_handle_t t, char *buffer, int len, int timeout_ms)
 {
-    int poll;
     transport_esp_tls_t *ssl = ssl_get_context_data(t);
 
-    if ((poll = esp_transport_poll_read(t, timeout_ms)) <= 0) {
-        return poll;
+    int poll = esp_transport_poll_read(t, timeout_ms);
+    if (poll == -1) {
+        return ERR_TCP_TRANSPORT_CONNECTION_FAILED;
     }
+    if (poll == 0) {
+        return ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT;
+    }
+
     int ret = esp_tls_conn_read(ssl->tls, (unsigned char *)buffer, len);
     if (ret < 0) {
         ESP_LOGE(TAG, "esp_tls_conn_read error, errno=%s", strerror(errno));
+        if (ret == ESP_TLS_ERR_SSL_WANT_READ || ret == ESP_TLS_ERR_SSL_TIMEOUT) {
+            ret = ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT;
+        }
+
         esp_tls_error_handle_t esp_tls_error_handle;
         if (esp_tls_get_error_handle(ssl->tls, &esp_tls_error_handle) == ESP_OK) {
             esp_transport_set_errors(t, esp_tls_error_handle);
         } else {
             ESP_LOGE(TAG, "Error in obtaining the error handle");
         }
-    }
-    if (ret == 0) {
+    } else if (ret == 0) {
         if (poll > 0) {
             // no error, socket reads 0 while previously detected as readable -> connection has been closed cleanly
             capture_tcp_transport_error(t, ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN);
         }
-        ret = -1;
+        ret = ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN;
     }
     return ret;
 }
 
 static int tcp_read(esp_transport_handle_t t, char *buffer, int len, int timeout_ms)
 {
-    int poll;
     transport_esp_tls_t *ssl = ssl_get_context_data(t);
 
-    if ((poll = esp_transport_poll_read(t, timeout_ms)) <= 0) {
-        return poll;
+    int poll = esp_transport_poll_read(t, timeout_ms);
+    if (poll == -1) {
+        return ERR_TCP_TRANSPORT_CONNECTION_FAILED;
     }
+    if (poll == 0) {
+        return ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT;
+    }
+
     int ret = recv(ssl->sockfd, (unsigned char *)buffer, len, 0);
     if (ret < 0) {
         ESP_LOGE(TAG, "tcp_read error, errno=%s", strerror(errno));
         esp_transport_capture_errno(t, errno);
-    }
-    if (ret == 0) {
+        if (errno == EAGAIN) {
+            ret = ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT;
+        }
+    } else if (ret == 0) {
         if (poll > 0) {
             // no error, socket reads 0 while previously detected as readable -> connection has been closed cleanly
             capture_tcp_transport_error(t, ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN);
         }
-        ret = -1;
+        ret = ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN;
     }
     return ret;
 }