From e1f982921a08022ca4307900fc058ccacccd26d0 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 12 Nov 2019 17:42:51 +0100 Subject: [PATCH] ws_client: fix handling timeouts by websocket client. tcp-transport component did not support wait forever. this update uses value of -1 to request this state. websocket client uses timeouts in RTOS ticks. fixed recalculation to ms (including special value of -1) to use correctly tcp-transport component Closes https://github.com/espressif/esp-idf/issues/4316 --- .../esp_websocket_client/esp_websocket_client.c | 4 ++-- .../include/esp_websocket_client.h | 6 +++--- components/tcp_transport/include/esp_transport.h | 12 ++++++------ .../tcp_transport/include/esp_transport_ws.h | 2 +- .../private_include/esp_transport_utils.h | 13 +++++++++---- components/tcp_transport/transport_ssl.c | 10 ++++------ components/tcp_transport/transport_tcp.c | 16 ++++++++-------- components/tcp_transport/transport_utils.c | 6 +++++- 8 files changed, 38 insertions(+), 31 deletions(-) diff --git a/components/esp_websocket_client/esp_websocket_client.c b/components/esp_websocket_client/esp_websocket_client.c index b509463cf..08d2ba21f 100644 --- a/components/esp_websocket_client/esp_websocket_client.c +++ b/components/esp_websocket_client/esp_websocket_client.c @@ -606,14 +606,14 @@ static int esp_websocket_client_send_with_opcode(esp_websocket_client_handle_t c goto unlock_and_return; } - while (widx < len) { if (need_write > client->buffer_size) { need_write = client->buffer_size; } memcpy(client->tx_buffer, data + widx, need_write); // send with ws specific way and specific opcode - wlen = esp_transport_ws_send_raw(client->transport, opcode, (char *)client->tx_buffer, need_write, timeout); + wlen = esp_transport_ws_send_raw(client->transport, opcode, (char *)client->tx_buffer, need_write, + (timeout==portMAX_DELAY)? -1 : timeout * portTICK_PERIOD_MS); if (wlen <= 0) { ret = wlen; ESP_LOGE(TAG, "Network error: esp_transport_write() returned %d, errno=%d", ret, errno); diff --git a/components/esp_websocket_client/include/esp_websocket_client.h b/components/esp_websocket_client/include/esp_websocket_client.h index 444343c82..3dbe1df9a 100644 --- a/components/esp_websocket_client/include/esp_websocket_client.h +++ b/components/esp_websocket_client/include/esp_websocket_client.h @@ -155,7 +155,7 @@ esp_err_t esp_websocket_client_destroy(esp_websocket_client_handle_t client); * @param[in] client The client * @param[in] data The data * @param[in] len The length - * @param[in] timeout Write data timeout + * @param[in] timeout Write data timeout in RTOS ticks * * @return * - Number of data was sent @@ -169,7 +169,7 @@ int esp_websocket_client_send(esp_websocket_client_handle_t client, const char * * @param[in] client The client * @param[in] data The data * @param[in] len The length - * @param[in] timeout Write data timeout + * @param[in] timeout Write data timeout in RTOS ticks * * @return * - Number of data was sent @@ -183,7 +183,7 @@ int esp_websocket_client_send_bin(esp_websocket_client_handle_t client, const ch * @param[in] client The client * @param[in] data The data * @param[in] len The length - * @param[in] timeout Write data timeout + * @param[in] timeout Write data timeout in RTOS ticks * * @return * - Number of data was sent diff --git a/components/tcp_transport/include/esp_transport.h b/components/tcp_transport/include/esp_transport.h index 39e694f05..4841725d0 100644 --- a/components/tcp_transport/include/esp_transport.h +++ b/components/tcp_transport/include/esp_transport.h @@ -133,7 +133,7 @@ esp_err_t esp_transport_set_default_port(esp_transport_handle_t t, int port); * @param t The transport handle * @param[in] host Hostname * @param[in] port Port - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates wait forever) * * @return * - socket for will use by this transport @@ -147,7 +147,7 @@ int esp_transport_connect(esp_transport_handle_t t, const char *host, int port, * @param t The transport handle * @param[in] host Hostname * @param[in] port Port - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates wait forever) * * @return * - socket for will use by this transport @@ -161,7 +161,7 @@ int esp_transport_connect_async(esp_transport_handle_t t, const char *host, int * @param t The transport handle * @param buffer The buffer * @param[in] len The length - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates wait forever) * * @return * - Number of bytes was read @@ -173,7 +173,7 @@ int esp_transport_read(esp_transport_handle_t t, char *buffer, int len, int time * @brief Poll the transport until readable or timeout * * @param[in] t The transport handle - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates wait forever) * * @return * - 0 Timeout @@ -188,7 +188,7 @@ int esp_transport_poll_read(esp_transport_handle_t t, int timeout_ms); * @param t The transport handle * @param buffer The buffer * @param[in] len The length - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates wait forever) * * @return * - Number of bytes was written @@ -200,7 +200,7 @@ int esp_transport_write(esp_transport_handle_t t, const char *buffer, int len, i * @brief Poll the transport until writeable or timeout * * @param[in] t The transport handle - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates wait forever) * * @return * - 0 Timeout diff --git a/components/tcp_transport/include/esp_transport_ws.h b/components/tcp_transport/include/esp_transport_ws.h index 0876480a4..9920e1bff 100644 --- a/components/tcp_transport/include/esp_transport_ws.h +++ b/components/tcp_transport/include/esp_transport_ws.h @@ -63,7 +63,7 @@ esp_err_t esp_transport_ws_set_subprotocol(esp_transport_handle_t t, const char * @param[in] opcode ws operation code * @param[in] buffer The buffer * @param[in] len The length - * @param[in] timeout_ms The timeout milliseconds + * @param[in] timeout_ms The timeout milliseconds (-1 indicates block forever) * * @return * - Number of bytes was written diff --git a/components/tcp_transport/private_include/esp_transport_utils.h b/components/tcp_transport/private_include/esp_transport_utils.h index 6a9d1d02c..dbc91b119 100644 --- a/components/tcp_transport/private_include/esp_transport_utils.h +++ b/components/tcp_transport/private_include/esp_transport_utils.h @@ -30,12 +30,17 @@ extern "C" { } /** - * @brief Convert milliseconds to timeval struct + * @brief Convert milliseconds to timeval struct for valid timeouts, otherwise + * (if "wait forever" requested by timeout_ms=-1) timeval structure is not updated and NULL returned * - * @param[in] timeout_ms The timeout milliseconds - * @param[out] tv Timeval struct + * @param[in] timeout_ms The timeout value in milliseconds or -1 to waiting forever + * @param[out] tv Pointer to timeval struct + * + * @return + * - NULL if timeout_ms=-1 (wait forever) + * - pointer to the updated timeval structure (provided as "tv" argument) with recalculated timeout value */ -void esp_transport_utils_ms_to_timeval(int timeout_ms, struct timeval *tv); +struct timeval* esp_transport_utils_ms_to_timeval(int timeout_ms, struct timeval *tv); #ifdef __cplusplus diff --git a/components/tcp_transport/transport_ssl.c b/components/tcp_transport/transport_ssl.c index b92c21157..91fb4530b 100644 --- a/components/tcp_transport/transport_ssl.c +++ b/components/tcp_transport/transport_ssl.c @@ -86,16 +86,15 @@ static int ssl_poll_read(esp_transport_handle_t t, int timeout_ms) { transport_ssl_t *ssl = esp_transport_get_context_data(t); int ret = -1; + struct timeval timeout; fd_set readset; fd_set errset; FD_ZERO(&readset); FD_ZERO(&errset); FD_SET(ssl->tls->sockfd, &readset); FD_SET(ssl->tls->sockfd, &errset); - struct timeval timeout; - esp_transport_utils_ms_to_timeval(timeout_ms, &timeout); - ret = select(ssl->tls->sockfd + 1, &readset, NULL, &errset, &timeout); + ret = select(ssl->tls->sockfd + 1, &readset, NULL, &errset, esp_transport_utils_ms_to_timeval(timeout_ms, &timeout)); if (ret > 0 && FD_ISSET(ssl->tls->sockfd, &errset)) { int sock_errno = 0; uint32_t optlen = sizeof(sock_errno); @@ -110,15 +109,14 @@ static int ssl_poll_write(esp_transport_handle_t t, int timeout_ms) { transport_ssl_t *ssl = esp_transport_get_context_data(t); int ret = -1; + struct timeval timeout; fd_set writeset; fd_set errset; FD_ZERO(&writeset); FD_ZERO(&errset); FD_SET(ssl->tls->sockfd, &writeset); FD_SET(ssl->tls->sockfd, &errset); - struct timeval timeout; - esp_transport_utils_ms_to_timeval(timeout_ms, &timeout); - ret = select(ssl->tls->sockfd + 1, NULL, &writeset, &errset, &timeout); + ret = select(ssl->tls->sockfd + 1, NULL, &writeset, &errset, esp_transport_utils_ms_to_timeval(timeout_ms, &timeout)); if (ret > 0 && FD_ISSET(ssl->tls->sockfd, &errset)) { int sock_errno = 0; uint32_t optlen = sizeof(sock_errno); diff --git a/components/tcp_transport/transport_tcp.c b/components/tcp_transport/transport_tcp.c index 3fba399a0..5bfb99ddf 100644 --- a/components/tcp_transport/transport_tcp.c +++ b/components/tcp_transport/transport_tcp.c @@ -52,7 +52,7 @@ static int resolve_dns(const char *host, struct sockaddr_in *ip) { static int tcp_connect(esp_transport_handle_t t, const char *host, int port, int timeout_ms) { struct sockaddr_in remote_ip; - struct timeval tv; + struct timeval tv = { 0 }; transport_tcp_t *tcp = esp_transport_get_context_data(t); bzero(&remote_ip, sizeof(struct sockaddr_in)); @@ -74,7 +74,7 @@ static int tcp_connect(esp_transport_handle_t t, const char *host, int port, int remote_ip.sin_family = AF_INET; remote_ip.sin_port = htons(port); - esp_transport_utils_ms_to_timeval(timeout_ms, &tv); + esp_transport_utils_ms_to_timeval(timeout_ms, &tv); // if timeout=-1, tv is unchanged, 0, i.e. waits forever setsockopt(tcp->sock, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); setsockopt(tcp->sock, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)); @@ -117,15 +117,15 @@ static int tcp_poll_read(esp_transport_handle_t t, int timeout_ms) { transport_tcp_t *tcp = esp_transport_get_context_data(t); int ret = -1; + struct timeval timeout; fd_set readset; fd_set errset; FD_ZERO(&readset); FD_ZERO(&errset); FD_SET(tcp->sock, &readset); FD_SET(tcp->sock, &errset); - struct timeval timeout; - esp_transport_utils_ms_to_timeval(timeout_ms, &timeout); - ret = select(tcp->sock + 1, &readset, NULL, &errset, &timeout); + + ret = select(tcp->sock + 1, &readset, NULL, &errset, esp_transport_utils_ms_to_timeval(timeout_ms, &timeout)); if (ret > 0 && FD_ISSET(tcp->sock, &errset)) { int sock_errno = 0; uint32_t optlen = sizeof(sock_errno); @@ -140,15 +140,15 @@ static int tcp_poll_write(esp_transport_handle_t t, int timeout_ms) { transport_tcp_t *tcp = esp_transport_get_context_data(t); int ret = -1; + struct timeval timeout; fd_set writeset; fd_set errset; FD_ZERO(&writeset); FD_ZERO(&errset); FD_SET(tcp->sock, &writeset); FD_SET(tcp->sock, &errset); - struct timeval timeout; - esp_transport_utils_ms_to_timeval(timeout_ms, &timeout); - ret = select(tcp->sock + 1, NULL, &writeset, &errset, &timeout); + + ret = select(tcp->sock + 1, NULL, &writeset, &errset, esp_transport_utils_ms_to_timeval(timeout_ms, &timeout)); if (ret > 0 && FD_ISSET(tcp->sock, &errset)) { int sock_errno = 0; uint32_t optlen = sizeof(sock_errno); diff --git a/components/tcp_transport/transport_utils.c b/components/tcp_transport/transport_utils.c index 9ef56dc24..5e0a02162 100644 --- a/components/tcp_transport/transport_utils.c +++ b/components/tcp_transport/transport_utils.c @@ -6,8 +6,12 @@ #include "esp_transport_utils.h" -void esp_transport_utils_ms_to_timeval(int timeout_ms, struct timeval *tv) +struct timeval* esp_transport_utils_ms_to_timeval(int timeout_ms, struct timeval *tv) { + if (timeout_ms == -1) { + return NULL; + } tv->tv_sec = timeout_ms / 1000; tv->tv_usec = (timeout_ms - (tv->tv_sec * 1000)) * 1000; + return tv; } \ No newline at end of file