Merge branch 'bugfix/tcp_connect_tout' into 'master'

tcp_transport: Implement connect timeout

Closes IDFGH-2971, IDFGH-2810, and IDFGH-1541

See merge request espressif/esp-idf!8428

components/esp-tls/esp_tls.c

@@ -96,15 +96,23 @@ static ssize_t tcp_write(esp_tls_t *tls, const char *data, size_t datalen)
  * @brief      Close the TLS connection and free any allocated resources.
  */
 void esp_tls_conn_delete(esp_tls_t *tls)
+{
+    esp_tls_conn_destroy(tls);
+}
+
+int esp_tls_conn_destroy(esp_tls_t *tls)
 {
     if (tls != NULL) {
+        int ret = 0;
         _esp_tls_conn_delete(tls);
         if (tls->sockfd >= 0) {
-            close(tls->sockfd);
+            ret = close(tls->sockfd);
         }
-    free(tls->error_handle);
-    free(tls);
+        free(tls->error_handle);
+        free(tls);
+        return ret;
     }
+    return -1; // invalid argument
 }
 
 esp_tls_t *esp_tls_init(void)

components/esp-tls/esp_tls.h

@@ -463,6 +463,15 @@ static inline ssize_t esp_tls_conn_read(esp_tls_t *tls, void  *data, size_t data
     return tls->read(tls, (char *)data, datalen);
 }
 
+/**
+ * @brief      Compatible version of esp_tls_conn_destroy() to close the TLS/SSL connection
+ *
+ * @note This API will be removed in IDFv5.0
+ *
+ * @param[in]  tls  pointer to esp-tls as esp-tls handle.
+ */
+void esp_tls_conn_delete(esp_tls_t *tls);
+
 /**
  * @brief      Close the TLS/SSL connection and free any allocated resources.
  *
@@ -470,8 +479,11 @@ static inline ssize_t esp_tls_conn_read(esp_tls_t *tls, void  *data, size_t data
  * esp_tls_conn_http_new() APIs.
  *
  * @param[in]  tls  pointer to esp-tls as esp-tls handle.
+ *
+ * @return    - 0 on success
+ *            - -1 if socket error or an invalid argument
  */
-void esp_tls_conn_delete(esp_tls_t *tls);
+int esp_tls_conn_destroy(esp_tls_t *tls);
 
 /**
  * @brief      Return the number of application data bytes remaining to be

components/tcp_transport/test/test_transport.c

@@ -4,6 +4,111 @@
 #include "esp_transport_tcp.h"
 #include "esp_transport_ssl.h"
 #include "esp_transport_ws.h"
+#include "test_utils.h"
+#include "esp_log.h"
+#include "lwip/err.h"
+#include "lwip/sockets.h"
+#include "lwip/sys.h"
+#include <lwip/netdb.h>
+#include "freertos/event_groups.h"
+
+#define TCP_CONNECT_DONE (1)
+#define TCP_LISTENER_DONE (2)
+
+struct tcp_connect_task_params {
+    int timeout_ms;
+    int port;
+    EventGroupHandle_t tcp_connect_done;
+    int ret;
+    int listen_sock;
+    int last_connect_sock;
+    bool tcp_listener_failed;
+};
+
+/**
+ * @brief Recursively connects with a new socket to loopback interface until the last one blocks.
+ * The last socket is closed upon test teardown, that initiates recursive cleanup (close) for all
+ * active/connected sockets.
+ */
+static void connect_once(struct tcp_connect_task_params *params)
+{
+    struct sockaddr_in dest_addr_ip4 = {    .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+                                            .sin_family = AF_INET,
+                                            .sin_port = htons(params->port)    };
+    int connect_sock = socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
+    if (connect_sock < 0) {
+        params->tcp_listener_failed = true;
+        return;
+    }
+    params->last_connect_sock = connect_sock;
+    int err = connect(connect_sock,  (struct sockaddr *)&dest_addr_ip4, sizeof(dest_addr_ip4));
+    if (err != 0) {
+        // The last connection is expected to fail here, since the both sockets get closed on test cleanup
+        return;
+    }
+    connect_once(params);
+    close(connect_sock);
+}
+
+/**
+ * @brief creates a listener (without and acceptor) and connect to as many times as possible
+ * to prepare an endpoint which would make the client block but not complete TCP handshake
+ */
+static void localhost_listener(void *pvParameters)
+{
+    const char* TAG = "tcp_transport_test";
+    struct tcp_connect_task_params *params = pvParameters;
+    struct sockaddr_in dest_addr_ip4 = {    .sin_addr.s_addr = htonl(INADDR_ANY),
+                                            .sin_family = AF_INET,
+                                            .sin_port = htons(params->port)    };
+    // Create listener socket and bind it to ANY address
+    params->listen_sock = socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
+    if (params->listen_sock < 0) {
+        ESP_LOGE(TAG, "Unable to create socket");
+        params->tcp_listener_failed = true;
+        goto failed;
+    }
+    int err = bind(params->listen_sock, (struct sockaddr *)&dest_addr_ip4, sizeof(dest_addr_ip4));
+    if (err != 0) {
+        ESP_LOGE(TAG, "Socket unable to bind: errno %d", errno);
+        params->tcp_listener_failed = true;
+        goto failed;
+    }
+
+    // Listen with backlog set to a low number
+    err = listen(params->listen_sock, 4);
+    if (err != 0) {
+        ESP_LOGE(TAG, "Error occurred during listen: errno %d", errno);
+        params->tcp_listener_failed = true;
+        goto failed;
+    }
+
+    // Ideally we would set backlog to 0, but since this is an implementation specific recommendation parameter,
+    // we recursively create sockets and try to connect to this listener in order to consume the backlog. After
+    // the backlog is consumed, the last connection blocks (waiting for accept), but at that point we are sure
+    // that any other connection would also block
+    connect_once(params);
+
+failed:
+    xEventGroupSetBits(params->tcp_connect_done, TCP_LISTENER_DONE);
+    vTaskSuspend(NULL);
+}
+
+static void tcp_connect_task(void *pvParameters)
+{
+    struct tcp_connect_task_params *params = pvParameters;
+
+    esp_transport_list_handle_t transport_list = esp_transport_list_init();
+    esp_transport_handle_t tcp = esp_transport_tcp_init();
+    esp_transport_list_add(transport_list, tcp, "tcp");
+
+    params->ret = esp_transport_connect(tcp, "localhost", params->port, params->timeout_ms);
+    xEventGroupSetBits(params->tcp_connect_done, TCP_CONNECT_DONE);
+    esp_transport_close(tcp);
+    esp_transport_list_destroy(transport_list);
+    vTaskSuspend(NULL);
+}
+
 
 TEST_CASE("tcp_transport: init and deinit transport list", "[tcp_transport][leaks=0]")
 {
@@ -26,3 +131,49 @@ TEST_CASE("tcp_transport: using ws transport separately", "[tcp_transport][leaks
     TEST_ASSERT_EQUAL(ESP_OK, esp_transport_destroy(ws));
     TEST_ASSERT_EQUAL(ESP_OK, esp_transport_destroy(tcp));
 }
+
+TEST_CASE("tcp_transport: connect timeout", "[tcp_transport]")
+{
+    // This case simulates connection timeout running tcp connect asynchronously with other socket connection
+    // consuming entire socket listener backlog.
+    // Important: Both tasks must run on the same core, with listener's prio higher to make sure that
+    // 1) first the localhost_listener() creates and connects all sockets until the last one blocks
+    // 2) before the tcp_connect_task() attempts to connect and thus fails with connection timeout
+
+    struct tcp_connect_task_params params = {   .tcp_connect_done = xEventGroupCreate(),
+                                                .timeout_ms = 200,
+                                                .port = 80 };
+    TickType_t max_wait = pdMS_TO_TICKS(params.timeout_ms * 10);
+    TaskHandle_t localhost_listener_task_handle = NULL;
+    TaskHandle_t tcp_connect_task_handle = NULL;
+
+    test_case_uses_tcpip();
+
+    // Create listener and connect it with as many sockets until the last one blocks
+    xTaskCreatePinnedToCore(localhost_listener, "localhost_listener", 4096, (void*)&params, 5, &localhost_listener_task_handle, 0);
+
+    // Perform tcp-connect in a separate task to check asynchronously for the timeout
+    xTaskCreatePinnedToCore(tcp_connect_task, "tcp_connect_task", 4096, (void*)&params, 4, &tcp_connect_task_handle, 0);
+
+    // Roughly measure tick-time spent while trying to connect
+    TickType_t start = xTaskGetTickCount();
+    EventBits_t bits = xEventGroupWaitBits(params.tcp_connect_done, TCP_CONNECT_DONE, true, true, max_wait);
+    TickType_t end = xTaskGetTickCount();
+
+    TEST_ASSERT_EQUAL(TCP_CONNECT_DONE, bits);          // Connection has finished
+    TEST_ASSERT_EQUAL(-1, params.ret);   // Connection failed with -1
+
+    // Test connection attempt took expected timeout value
+    TEST_ASSERT_INT_WITHIN(pdMS_TO_TICKS(params.timeout_ms/5), pdMS_TO_TICKS(params.timeout_ms), end-start);
+
+    // Closing both parties of the last "blocking" connection to unwind localhost_listener() and let other connected sockets closed
+    close(params.listen_sock);
+    close(params.last_connect_sock);
+
+    // Cleanup
+    xEventGroupWaitBits(params.tcp_connect_done, TCP_LISTENER_DONE, true, true, max_wait);
+    TEST_ASSERT_EQUAL(false, params.tcp_listener_failed);
+    vEventGroupDelete(params.tcp_connect_done);
+    test_utils_task_delete(localhost_listener_task_handle);
+    test_utils_task_delete(tcp_connect_task_handle);
+}

components/tcp_transport/transport_ssl.c

@@ -74,7 +74,7 @@ static int ssl_connect(esp_transport_handle_t t, const char *host, int port, int
     if (esp_tls_conn_new_sync(host, strlen(host), port, &ssl->cfg, ssl->tls) <= 0) {
         ESP_LOGE(TAG, "Failed to open a new connection");
         esp_transport_set_errors(t, ssl->tls->error_handle);
-        esp_tls_conn_delete(ssl->tls);
+        esp_tls_conn_destroy(ssl->tls);
         ssl->tls = NULL;
         return -1;
     }
@@ -170,7 +170,7 @@ static int ssl_close(esp_transport_handle_t t)
     int ret = -1;
     transport_ssl_t *ssl = esp_transport_get_context_data(t);
     if (ssl->ssl_initialized) {
-        esp_tls_conn_delete(ssl->tls);
+        ret = esp_tls_conn_destroy(ssl->tls);
         ssl->conn_state = TRANS_SSL_INIT;
         ssl->ssl_initialized = false;
     }

components/tcp_transport/transport_tcp.c

@@ -81,14 +81,68 @@ static int tcp_connect(esp_transport_handle_t t, const char *host, int port, int
     setsockopt(tcp->sock, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
     setsockopt(tcp->sock, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv));
 
-    ESP_LOGD(TAG, "[sock=%d],connecting to server IP:%s,Port:%d...",
-             tcp->sock, ipaddr_ntoa((const ip_addr_t*)&remote_ip.sin_addr.s_addr), port);
-    if (connect(tcp->sock, (struct sockaddr *)(&remote_ip), sizeof(struct sockaddr)) != 0) {
-        close(tcp->sock);
-        tcp->sock = -1;
-        return -1;
+    // Set socket to non-blocking
+    int flags;
+    if ((flags = fcntl(tcp->sock, F_GETFL, NULL)) < 0) {
+        ESP_LOGE(TAG, "[sock=%d] get file flags error: %s", tcp->sock, strerror(errno));
+        goto error;
+    }
+    if (fcntl(tcp->sock, F_SETFL, flags |= O_NONBLOCK) < 0) {
+        ESP_LOGE(TAG, "[sock=%d] set nonblocking error: %s", tcp->sock, strerror(errno));
+        goto error;
+    }
+
+    ESP_LOGD(TAG, "[sock=%d] Connecting to server. IP: %s, Port: %d",
+            tcp->sock, ipaddr_ntoa((const ip_addr_t*)&remote_ip.sin_addr.s_addr), port);
+
+    if (connect(tcp->sock, (struct sockaddr *)(&remote_ip), sizeof(struct sockaddr)) < 0) {
+        if (errno == EINPROGRESS) {
+            fd_set fdset;
+
+            esp_transport_utils_ms_to_timeval(timeout_ms, &tv);
+            FD_ZERO(&fdset);
+            FD_SET(tcp->sock, &fdset);
+
+            int res = select(tcp->sock+1, NULL, &fdset, NULL, &tv);
+            if (res < 0) {
+                ESP_LOGE(TAG, "[sock=%d] select() error: %s", tcp->sock, strerror(errno));
+                goto error;
+            }
+            else if (res == 0) {
+                ESP_LOGE(TAG, "[sock=%d] select() timeout", tcp->sock);
+                goto error;
+            } else {
+                int sockerr;
+                socklen_t len = (socklen_t)sizeof(int);
+
+                if (getsockopt(tcp->sock, SOL_SOCKET, SO_ERROR, (void*)(&sockerr), &len) < 0) {
+                    ESP_LOGE(TAG, "[sock=%d] getsockopt() error: %s", tcp->sock, strerror(errno));
+                    goto error;
+                }
+                else if (sockerr) {
+                    ESP_LOGE(TAG, "[sock=%d] delayed connect error: %s", tcp->sock, strerror(sockerr));
+                    goto error;
+                }
+            }
+        } else {
+            ESP_LOGE(TAG, "[sock=%d] connect() error: %s", tcp->sock, strerror(errno));
+            goto error;
+        }
+    }
+    // Reset socket to blocking
+    if ((flags = fcntl(tcp->sock, F_GETFL, NULL)) < 0) {
+        ESP_LOGE(TAG, "[sock=%d] get file flags error: %s", tcp->sock, strerror(errno));
+        goto error;
+    }
+    if (fcntl(tcp->sock, F_SETFL, flags & ~O_NONBLOCK) < 0) {
+        ESP_LOGE(TAG, "[sock=%d] reset blocking error: %s", tcp->sock, strerror(errno));
+        goto error;
     }
     return tcp->sock;
+error:
+    close(tcp->sock);
+    tcp->sock = -1;
+    return -1;
 }
 
 static int tcp_write(esp_transport_handle_t t, const char *buffer, int len, int timeout_ms)