WiFiClient.cpp - Fix connect() behavior

[weinrank]

This PR fixes an issue regarding WiFiClient::connect()
We observed that connect() returned 1 even if the connect call wasn't successful.

We fixed this behaviour and improved the error handling.
If you need further explanation, let me know.

Thanks to @tuexen for help!

[boarchuz]

Looks good to me. Turns out the value returned by select() is not enough to check that the socket is ready to be written to, according to here: http://man7.org/linux/man-pages/man2/connect.2.html (under EINPROGRESS)

While we're at it, it might be worth comparing lwip_connect_r with EINPROGRESS as well. ie. errno should be set to EINPROGRESS if a non-blocking connect has successfully begun, so it might look something like this:

lwip_connect_r(sockfd, (struct sockaddr*)&serveraddr, sizeof(serveraddr));
if(errno != EINPROGRESS)
{
    log_e("connect on fd %d, errno: %d, \"%s\"", sockfd, errno, strerror(errno));
    close(sockfd);
    return 0;
}

[tuexen]

I think you should only check for errno if lwip_connect_r wasn't successful. At least in kernel implementations this can happen. In case lwip_connect_r fails, only EINPROGRESS is expected.
So the code should be something like:

res = lwip_connect_r(sockfd, (struct sockaddr*)&serveraddr, sizeof(serveraddr));
if(res < 0 && errno != EINPROGRESS)
{
    log_e("connect on fd %d, errno: %d, \"%s\"", sockfd, errno, strerror(errno));
    close(sockfd);
    return 0;
}

[boarchuz]

Same either way, it's non-blocking so it always returns -1. errno is the important thing.

[tuexen]

Same either way, it's non-blocking so it always returns -1. errno is the important thing.

Is that true the LWIP stack that connect() for non-blocking socket always fails? This is not true for kernel stacks. If the TCP handshake completes before control is given back to the caller of connect(), the call is successful. For example, this happens (not always) on the loopback interface...
Since one should only read errno after a system call failed, errno might have an arbitrary value if the connect call was successful.

[boarchuz]

I’m happy to defer to your expertise on that one. I agree that checking it is good practice either way.

[tuexen]

Great. I'll talk to @weinrank tomorrow such that he updates the PR after testing.

[weinrank]

I've updated the PR with an additional check of the lwip_connect_r() return code.

[weinrank]

Further information to reproduce the issue

Sample program

I've compiled the sample program with extended debug output: -DCORE_DEBUG_LEVEL=5
Adjust WIFI credentials and IP addresses accordingly.

#ifdef TARGET_ESP8266
#include <ESP8266WiFi.h>
#else
#include <WiFi.h>
#endif

static bool wifi_connect(void);

// WiFi / MQTT
WiFiClient espClient;

void setup(void) {
  Serial.begin(115200);
  Serial.println(F("Hello! Let's go..."));

  // We're using the pin for activity indication
  pinMode(LED_BUILTIN, OUTPUT);

  randomSeed(micros());

  Serial.println(F("Initialized"));
  delay(500);
}

void loop(void) {
  static long last_led_toggle;

  int rc = 0;

  // Toggle status LED
  if (last_led_toggle == 0 || (millis() - last_led_toggle) >= 250) {
    last_led_toggle = millis();
    digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));
  }

  // Maintain WiFi connection
  if (!wifi_connect()) {
    delay(5000);
    return;
  }

  char buffer[200];

  // Unused IP - expecting timeout
  #define IP "10.11.12.3"
  #define PORT 23
  rc = espClient.connect(IP, PORT);
  snprintf(buffer, sizeof(buffer), "espClient.connect(%s, %d) - RC=%d\n", IP, PORT, rc);
  Serial.print(buffer);

  // Used IP - unused port - expecting refused connection
  #define IP "10.11.12.2"
  #define PORT 23
  rc = espClient.connect(IP, PORT);
  snprintf(buffer, sizeof(buffer), "espClient.connect(%s, %d) - RC=%d\n", IP, PORT, rc);
  Serial.print(buffer);

  // Used IP - used port - expecting successful connection
  #define IP "10.11.12.2"
  #define PORT 22
  rc = espClient.connect(IP, PORT);
  snprintf(buffer, sizeof(buffer), "espClient.connect(%s, %d) - RC=%d\n", IP, PORT, rc);
  Serial.print(buffer);

  delay(5000);
}


static bool wifi_connect(void) {
  int connection_tries = 0;

  if (WiFi.status() == WL_CONNECTED) {
    return true;
  }
  
  delay(10);
  // We start by connecting to a WiFi network
  Serial.print("Connecting to WIFI ");
  Serial.print(CONFIG_WIFI_NAME);
  Serial.print(" ");

  WiFi.disconnect();

  WiFi.mode(WIFI_STA);
  WiFi.begin(CONFIG_WIFI_NAME, CONFIG_WIFI_PSK);

  while (WiFi.status() != WL_CONNECTED) {
    delay(1000);
    Serial.print(".");

    if (++connection_tries >= 10) {
      break;
    }
  }

  if (WiFi.status() != WL_CONNECTED) {
    Serial.println(" failed!");
    return false;
  }

  Serial.println(" connected!");
  Serial.print("MAC: ");
  Serial.println(String(WiFi.macAddress()));
  Serial.print("IP: ");
  Serial.println(WiFi.localIP());
  Serial.print("SM: ");
  Serial.println(WiFi.subnetMask());
  Serial.print("GW: ");
  Serial.println(WiFi.gatewayIP());
  Serial.print("BSSID: ");
  Serial.println(WiFi.BSSIDstr());
  Serial.println("");

  return true;
}

Output without patch

espClient.connect(10.11.12.3, 23) - RC=1
espClient.connect(10.11.12.2, 23) - RC=1
espClient.connect(10.11.12.2, 22) - RC=1

Output after patch

[E][WiFiClient.cpp:258] connect(): socket error on fd 63, errno: 113, "Software caused connection abort"
espClient.connect(10.11.12.3, 23) - RC=0
[E][WiFiClient.cpp:258] connect(): socket error on fd 54, errno: 104, "Connection reset by peer"
espClient.connect(10.11.12.2, 23) - RC=0
espClient.connect(10.11.12.2, 22) - RC=1

[weinrank]

Is this PR ready to be merged or is anything missing?

[me-no-dev]

Somehow I have missed this PR. :) Sorry for the delay.