connect: improve happy eyeballs handling

For QUIC but also for regular TCP when the second family runs out of IPs
with a failure while the first family is still trying to connect.

Separated the timeout handling for IPv4 and IPv6 connections when they
both have a number of addresses to iterate over.

5 files changed, 84 insertions, 58 deletions

diff --git a/lib/connect.c b/lib/connect.c
index 46c285537..29293f087 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -560,7 +560,7 @@ static bool verifyconnect(curl_socket_t sockfd, int *error)
    to the correct family */
 static struct Curl_addrinfo *ainext(struct connectdata *conn,
                                     int tempindex,
-                                    bool next) /* use current or next entry */
+                                    bool next) /* use next entry? */
 {
   struct Curl_addrinfo *ai = conn->tempaddr[tempindex];
   if(ai && next)
@@ -571,7 +571,7 @@ static struct Curl_addrinfo *ainext(struct connectdata *conn,
   return ai;
 }
 
-/* Used within the multi interface. Try next IP address, return TRUE if no
+/* Used within the multi interface. Try next IP address, returns error if no
    more address exists or error */
 static CURLcode trynextip(struct connectdata *conn,
                           int sockindex,
@@ -823,8 +823,8 @@ CURLcode Curl_is_connected(struct connectdata *conn,
   timediff_t allow;
   int error = 0;
   struct curltime now;
-  int rc;
-  int i;
+  int rc = 0;
+  unsigned int i;
 
   DEBUGASSERT(sockindex >= FIRSTSOCKET && sockindex <= SECONDARYSOCKET);
 
@@ -859,49 +859,42 @@ CURLcode Curl_is_connected(struct connectdata *conn,
     const int other = i ^ 1;
     if(conn->tempsock[i] == CURL_SOCKET_BAD)
       continue;
-
+    error = 0;
 #ifdef ENABLE_QUIC
     if(conn->transport == TRNSPRT_QUIC) {
       result = Curl_quic_is_connected(conn, i, connected);
-      if(result) {
-        error = SOCKERRNO;
-        goto error;
-      }
-      if(*connected) {
+      if(!result && *connected) {
         /* use this socket from now on */
         conn->sock[sockindex] = conn->tempsock[i];
         conn->ip_addr = conn->tempaddr[i];
         conn->tempsock[i] = CURL_SOCKET_BAD;
         post_SOCKS(conn, sockindex, connected);
         connkeep(conn, "HTTP/3 default");
-        return result;
-      }
-      /* should we try another protocol family? */
-      if(i == 0 && !conn->bits.parallel_connect &&
-         (Curl_timediff(now, conn->connecttime) >=
-          data->set.happy_eyeballs_timeout)) {
-        conn->bits.parallel_connect = TRUE; /* starting now */
-        trynextip(conn, sockindex, 1);
+        return CURLE_OK;
       }
-      continue;
+      if(result)
+        error = SOCKERRNO;
     }
+    else
 #endif
-
+    {
 #ifdef mpeix
-    /* Call this function once now, and ignore the results. We do this to
-       "clear" the error state on the socket so that we can later read it
-       reliably. This is reported necessary on the MPE/iX operating system. */
-    (void)verifyconnect(conn->tempsock[i], NULL);
+      /* Call this function once now, and ignore the results. We do this to
+         "clear" the error state on the socket so that we can later read it
+         reliably. This is reported necessary on the MPE/iX operating
+         system. */
+      (void)verifyconnect(conn->tempsock[i], NULL);
 #endif
 
-    /* check socket for connect */
-    rc = SOCKET_WRITABLE(conn->tempsock[i], 0);
+      /* check socket for connect */
+      rc = SOCKET_WRITABLE(conn->tempsock[i], 0);
+    }
 
     if(rc == 0) { /* no connection yet */
-      error = 0;
-      if(Curl_timediff(now, conn->connecttime) >= conn->timeoutms_per_addr) {
+      if(Curl_timediff(now, conn->connecttime) >=
+         conn->timeoutms_per_addr[i]) {
         infof(data, "After %" CURL_FORMAT_TIMEDIFF_T
-              "ms connect time, move on!\n", conn->timeoutms_per_addr);
+              "ms connect time, move on!\n", conn->timeoutms_per_addr[i]);
         error = ETIMEDOUT;
       }
 
@@ -946,9 +939,6 @@ CURLcode Curl_is_connected(struct connectdata *conn,
     else if(rc & CURL_CSELECT_ERR)
       (void)verifyconnect(conn->tempsock[i], &error);
 
-#ifdef ENABLE_QUIC
-    error:
-#endif
     /*
      * The connection failed here, we should attempt to connect to the "next
      * address" for the given host. But first remember the latest error.
@@ -968,7 +958,7 @@ CURLcode Curl_is_connected(struct connectdata *conn,
               Curl_strerror(error, buffer, sizeof(buffer)));
 #endif
 
-        conn->timeoutms_per_addr = conn->tempaddr[i]->ai_next == NULL ?
+        conn->timeoutms_per_addr[i] = conn->tempaddr[i]->ai_next == NULL ?
           allow : allow / 2;
         ainext(conn, i, TRUE);
         status = trynextip(conn, sockindex, i);
@@ -980,13 +970,15 @@ CURLcode Curl_is_connected(struct connectdata *conn,
     }
   }
 
-  if(result) {
+  if(result &&
+     (conn->tempsock[0] == CURL_SOCKET_BAD) &&
+     (conn->tempsock[1] == CURL_SOCKET_BAD)) {
     /* no more addresses to try */
     const char *hostname;
     char buffer[STRERROR_LEN];
 
-    /* if the first address family runs out of addresses to try before
-       the happy eyeball timeout, go ahead and try the next family now */
+    /* if the first address family runs out of addresses to try before the
+       happy eyeball timeout, go ahead and try the next family now */
     result = trynextip(conn, sockindex, 1);
     if(!result)
       return result;
@@ -1007,6 +999,9 @@ CURLcode Curl_is_connected(struct connectdata *conn,
           hostname, conn->port,
           Curl_strerror(error, buffer, sizeof(buffer)));
 
+    Curl_quic_disconnect(conn, 0);
+    Curl_quic_disconnect(conn, 1);
+
 #ifdef WSAETIMEDOUT
     if(WSAETIMEDOUT == data->state.os_errno)
       result = CURLE_OPERATION_TIMEDOUT;
@@ -1015,6 +1010,8 @@ CURLcode Curl_is_connected(struct connectdata *conn,
       result = CURLE_OPERATION_TIMEDOUT;
 #endif
   }
+  else
+    result = CURLE_OK; /* still trying */
 
   return result;
 }
@@ -1206,8 +1203,10 @@ static CURLcode singleipconnect(struct connectdata *conn,
   (void)curlx_nonblock(sockfd, TRUE);
 
   conn->connecttime = Curl_now();
-  if(conn->num_addr > 1)
-    Curl_expire(data, conn->timeoutms_per_addr, EXPIRE_DNS_PER_NAME);
+  if(conn->num_addr > 1) {
+    Curl_expire(data, conn->timeoutms_per_addr[0], EXPIRE_DNS_PER_NAME);
+    Curl_expire(data, conn->timeoutms_per_addr[1], EXPIRE_DNS_PER_NAME2);
+  }
 
   /* Connect TCP and QUIC sockets */
   if(!isconnected && (conn->transport != TRNSPRT_UDP)) {
@@ -1330,8 +1329,10 @@ CURLcode Curl_connecthost(struct connectdata *conn,  /* context */
   conn->tempsock[0] = conn->tempsock[1] = CURL_SOCKET_BAD;
 
   /* Max time for the next connection attempt */
-  conn->timeoutms_per_addr =
+  conn->timeoutms_per_addr[0] =
     conn->tempaddr[0]->ai_next == NULL ? timeout_ms : timeout_ms / 2;
+  conn->timeoutms_per_addr[1] =
+    conn->tempaddr[1]->ai_next == NULL ? timeout_ms : timeout_ms / 2;
 
   conn->tempfamily[0] = conn->tempaddr[0]?
     conn->tempaddr[0]->ai_family:0;
diff --git a/lib/quic.h b/lib/quic.h
index 1eb23e976..8e7df90ea 100644
--- a/lib/quic.h
+++ b/lib/quic.h
@@ -7,7 +7,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2020, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -47,11 +47,13 @@ int Curl_quic_ver(char *p, size_t len);
 CURLcode Curl_quic_done_sending(struct connectdata *conn);
 void Curl_quic_done(struct Curl_easy *data, bool premature);
 bool Curl_quic_data_pending(const struct Curl_easy *data);
+void Curl_quic_disconnect(struct connectdata *conn, int tempindex);
 
 #else /* ENABLE_QUIC */
 #define Curl_quic_done_sending(x)
 #define Curl_quic_done(x,y)
 #define Curl_quic_data_pending(x)
+#define Curl_quic_disconnect(x,y)
 #endif /* !ENABLE_QUIC */
 
 #endif /* HEADER_CURL_QUIC_H */
diff --git a/lib/urldata.h b/lib/urldata.h
index 33ec160ac..74b43abaa 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -981,8 +981,10 @@ struct connectdata {
   struct curltime connecttime;
   /* The two fields below get set in Curl_connecthost */
   int num_addr; /* number of addresses to try to connect to */
-  timediff_t timeoutms_per_addr; /* how long time in milliseconds to spend on
-                                    trying to connect to each IP address */
+
+  /* how long time in milliseconds to spend on trying to connect to each IP
+     address, per family */
+  timediff_t timeoutms_per_addr[2];
 
   const struct Curl_handler *handler; /* Connection's protocol handler */
   const struct Curl_handler *given;   /* The protocol first given */
@@ -1245,7 +1247,8 @@ typedef enum {
   EXPIRE_100_TIMEOUT,
   EXPIRE_ASYNC_NAME,
   EXPIRE_CONNECTTIMEOUT,
-  EXPIRE_DNS_PER_NAME,
+  EXPIRE_DNS_PER_NAME, /* family1 */
+  EXPIRE_DNS_PER_NAME2, /* family2 */
   EXPIRE_HAPPY_EYEBALLS_DNS, /* See asyn-ares.c */
   EXPIRE_HAPPY_EYEBALLS,
   EXPIRE_MULTI_PENDING,
diff --git a/lib/vquic/ngtcp2.c b/lib/vquic/ngtcp2.c
index b973a7333..e5528231c 100644
--- a/lib/vquic/ngtcp2.c
+++ b/lib/vquic/ngtcp2.c
@@ -256,11 +256,11 @@ static int quic_set_encryption_secrets(SSL *ssl,
   int level = quic_from_ossl_level(ossl_level);
 
   if(ngtcp2_crypto_derive_and_install_rx_key(
-         qs->qconn, NULL, NULL, NULL, level, rx_secret, secretlen) != 0)
+       qs->qconn, NULL, NULL, NULL, level, rx_secret, secretlen) != 0)
     return 0;
 
   if(ngtcp2_crypto_derive_and_install_tx_key(
-         qs->qconn, NULL, NULL, NULL, level, tx_secret, secretlen) != 0)
+       qs->qconn, NULL, NULL, NULL, level, tx_secret, secretlen) != 0)
     return 0;
 
   if(level == NGTCP2_CRYPTO_LEVEL_APP) {
@@ -341,9 +341,7 @@ static int quic_init_ssl(struct quicsocket *qs)
   /* this will need some attention when HTTPS proxy over QUIC get fixed */
   const char * const hostname = qs->conn->host.name;
 
-  if(qs->ssl)
-    SSL_free(qs->ssl);
-
+  DEBUGASSERT(!qs->ssl);
   qs->ssl = SSL_new(qs->sslctx);
 
   SSL_set_app_data(qs->ssl, qs);
@@ -375,11 +373,11 @@ static int secret_func(gnutls_session_t ssl,
 
   if(level != NGTCP2_CRYPTO_LEVEL_EARLY &&
      ngtcp2_crypto_derive_and_install_rx_key(
-         qs->qconn, NULL, NULL, NULL, level, rx_secret, secretlen) != 0)
+       qs->qconn, NULL, NULL, NULL, level, rx_secret, secretlen) != 0)
     return 0;
 
   if(ngtcp2_crypto_derive_and_install_tx_key(
-         qs->qconn, NULL, NULL, NULL, level, tx_secret, secretlen) != 0)
+       qs->qconn, NULL, NULL, NULL, level, tx_secret, secretlen) != 0)
     return 0;
 
   if(level == NGTCP2_CRYPTO_LEVEL_APP) {
@@ -429,8 +427,8 @@ static int tp_recv_func(gnutls_session_t ssl, const uint8_t *data,
   ngtcp2_transport_params params;
 
   if(ngtcp2_decode_transport_params(
-         &params, NGTCP2_TRANSPORT_PARAMS_TYPE_ENCRYPTED_EXTENSIONS,
-         data, data_size) != 0)
+       &params, NGTCP2_TRANSPORT_PARAMS_TYPE_ENCRYPTED_EXTENSIONS,
+       data, data_size) != 0)
     return -1;
 
   if(ngtcp2_conn_set_remote_transport_params(qs->qconn, &params) != 0)
@@ -471,8 +469,7 @@ static int quic_init_ssl(struct quicsocket *qs)
   const char * const hostname = qs->conn->host.name;
   int rc;
 
-  if(qs->ssl)
-    gnutls_deinit(qs->ssl);
+  DEBUGASSERT(!qs->ssl);
 
   gnutls_init(&qs->ssl, GNUTLS_CLIENT);
   gnutls_session_set_ptr(qs->ssl, qs);
@@ -782,6 +779,8 @@ CURLcode Curl_quic_connect(struct connectdata *conn,
   long port;
   int qfd;
 
+  if(qs->conn)
+    Curl_quic_disconnect(conn, sockindex);
   qs->conn = conn;
 
   /* extract the used address as a string */
@@ -880,11 +879,16 @@ static int ng_perform_getsock(const struct connectdata *conn,
   return ng_getsock((struct connectdata *)conn, socks);
 }
 
-static CURLcode qs_disconnect(struct quicsocket *qs)
+static void qs_disconnect(struct quicsocket *qs)
 {
   int i;
-  if(qs->qlogfd != -1)
+  if(!qs->conn) /* already closed */
+    return;
+  qs->conn = NULL;
+  if(qs->qlogfd != -1) {
     close(qs->qlogfd);
+    qs->qlogfd = -1;
+  }
   if(qs->ssl)
 #ifdef USE_OPENSSL
     SSL_free(qs->ssl);
@@ -903,14 +907,22 @@ static CURLcode qs_disconnect(struct quicsocket *qs)
 #ifdef USE_OPENSSL
   SSL_CTX_free(qs->sslctx);
 #endif
-  return CURLE_OK;
+}
+
+void Curl_quic_disconnect(struct connectdata *conn,
+                          int tempindex)
+{
+  if(conn->transport == TRNSPRT_QUIC)
+    qs_disconnect(&conn->hequic[tempindex]);
 }
 
 static CURLcode ng_disconnect(struct connectdata *conn,
                               bool dead_connection)
 {
   (void)dead_connection;
-  return qs_disconnect(&conn->hequic[0]);
+  Curl_quic_disconnect(conn, 0);
+  Curl_quic_disconnect(conn, 1);
+  return CURLE_OK;
 }
 
 static unsigned int ng_conncheck(struct connectdata *conn,
diff --git a/lib/vquic/quiche.c b/lib/vquic/quiche.c
index 9af2d894f..be6f15c19 100644
--- a/lib/vquic/quiche.c
+++ b/lib/vquic/quiche.c
@@ -107,6 +107,14 @@ static CURLcode quiche_disconnect(struct connectdata *conn,
   (void)dead_connection;
   return qs_disconnect(qs);
 }
+
+void Curl_quic_disconnect(struct connectdata *conn,
+                          int tempindex)
+{
+  if(conn->transport == TRNSPRT_QUIC)
+    qs_disconnect(&conn->hequic[tempindex]);
+}
+
 static unsigned int quiche_conncheck(struct connectdata *conn,
                                      unsigned int checks_to_perform)
 {