Error handling in esp_websocket_client_send_with_opcode() (IDFGH-8053)

[BartVanHofwegen]

Hi, looking at line 1004/1005 of esp_websocket_client.c it strikes me that there might be an error situation not correctly reflected in the the return value of esp_websocket_client_send_with_opcode() (see my comment in the snippet):

        wlen = esp_transport_ws_send_raw(client->transport, current_opcode, (char *)client->tx_buffer, need_write,
                                        (timeout==portMAX_DELAY)? -1 : timeout * portTICK_PERIOD_MS);
        if (wlen < 0 || (wlen == 0 && need_write != 0)) {
            ret = wlen;  /* <--- BvH HERE */
            ESP_LOGE(TAG, "Network error: esp_transport_write() returned %d, errno=%d", ret, errno);
            esp_websocket_free_buf(client, true);
            esp_websocket_client_abort_connection(client);
            goto unlock_and_return;
        }

When 0 is returned from esp_transport_ws_send_raw() while there was data to send (need_write != 0), the return value is set to 0, which is also ESP_OK. If and only if the transport layer can indeed return 0 in this case, I would suggest:

ret = (wlen < 0) ? wlen : ESP_FAIL;

Before filing a pull request, I would like to know if I am right or if there is more to this...

Thanks for the great components BTW !

[david-cermak]

Hi @BartVanHofwegen

Thank you for the report. Yes, this is a bug, but since we return number of bytes rather than esp_err_t type (ESP_OK vs ESP_FAIL):

esp-protocols/components/esp_websocket_client/include/esp_websocket_client.h

Lines 190 to 192 in 1ffc20c

	* @return
	* - Number of data was sent
	* - (-1) if any errors

I'd suggest adding

    ret = widx;

Or better yet, remove those incorrect types and conversions and return ESP_FAIL;'s :

esp-protocols/components/esp_websocket_client/esp_websocket_client.c

Line 967 in 1ffc20c

int ret = ESP_FAIL;

and simply return -1; on invalid params/etc, and return widx; on happy flow.

[BartVanHofwegen]

Hi @david-cermak, thanks for the reply.

Or better yet, remove those incorrect types and conversions and return ESP_FAIL;'s :

Good point. BTW returning ESP_FAIL is exactly what I proposed for this particular line indeed. widx is already returned on happy flow as it is. I was just not sure if the transport layer could return 0 (in stead of an error) in this case. If not, the whole condition for this case could simply be trimmed.