Merge pull request #578 from mnunberg/connfix

Proper error reporting for connect failures

7 files changed, 103 insertions, 36 deletions

diff --git a/async.c b/async.c
index cb5b841..0cecd30 100644
--- a/async.c
+++ b/async.c
@@ -506,22 +506,22 @@ void redisProcessCallbacks(redisAsyncContext *ac) {
  * write event fires. When connecting was not successful, the connect callback
  * is called with a REDIS_ERR status and the context is free'd. */
 static int __redisAsyncHandleConnect(redisAsyncContext *ac) {
+    int completed = 0;
     redisContext *c = &(ac->c);
-
-    if (redisCheckSocketError(c) == REDIS_ERR) {
-        /* Try again later when connect(2) is still in progress. */
-        if (errno == EINPROGRESS)
-            return REDIS_OK;
-
-        if (ac->onConnect) ac->onConnect(ac,REDIS_ERR);
+    if (redisCheckConnectDone(c, &completed) == REDIS_ERR) {
+        /* Error! */
+        redisCheckSocketError(c);
+        if (ac->onConnect) ac->onConnect(ac, REDIS_ERR);
         __redisAsyncDisconnect(ac);
         return REDIS_ERR;
+    } else if (completed == 1) {
+        /* connected! */
+        if (ac->onConnect) ac->onConnect(ac, REDIS_OK);
+        c->flags |= REDIS_CONNECTED;
+        return REDIS_OK;
+    } else {
+        return REDIS_OK;
     }
-
-    /* Mark context as connected. */
-    c->flags |= REDIS_CONNECTED;
-    if (ac->onConnect) ac->onConnect(ac,REDIS_OK);
-    return REDIS_OK;
 }
 
 /* This function should be called when the socket is readable.
diff --git a/async.h b/async.h
index e69d840..740555c 100644
--- a/async.h
+++ b/async.h
@@ -93,6 +93,10 @@ typedef struct redisAsyncContext {
     /* Regular command callbacks */
     redisCallbackList replies;
 
+    /* Address used for connect() */
+    struct sockaddr *saddr;
+    size_t addrlen;
+
     /* Subscription callbacks */
     struct {
         redisCallbackList invalid;
diff --git a/hiredis.c b/hiredis.c
index 98f43c9..351dfbc 100644
--- a/hiredis.c
+++ b/hiredis.c
@@ -606,12 +606,14 @@ void redisFree(redisContext *c) {
         return;
     if (c->fd > 0)
         close(c->fd);
+
     sdsfree(c->obuf);
     redisReaderFree(c->reader);
     free(c->tcp.host);
     free(c->tcp.source_addr);
     free(c->unix_sock.path);
     free(c->timeout);
+    free(c->saddr);
     free(c);
 }
 
diff --git a/hiredis.h b/hiredis.h
index bd53e7e..1b0d5e6 100644
--- a/hiredis.h
+++ b/hiredis.h
@@ -134,6 +134,9 @@ typedef struct redisContext {
         char *path;
     } unix_sock;
 
+    /* For non-blocking connect */
+    struct sockadr *saddr;
+    size_t addrlen;
 } redisContext;
 
 redisContext *redisConnect(const char *ip, int port);
diff --git a/net.c b/net.c
index 5d50f7c..a4b3abc 100644
--- a/net.c
+++ b/net.c
@@ -221,8 +221,10 @@ static int redisContextWaitReady(redisContext *c, long msec) {
             return REDIS_ERR;
         }
 
-        if (redisCheckSocketError(c) != REDIS_OK)
+        if (redisCheckConnectDone(c, &res) != REDIS_OK || res == 0) {
+            redisCheckSocketError(c);
             return REDIS_ERR;
+        }
 
         return REDIS_OK;
     }
@@ -232,8 +234,28 @@ static int redisContextWaitReady(redisContext *c, long msec) {
     return REDIS_ERR;
 }
 
+int redisCheckConnectDone(redisContext *c, int *completed) {
+    int rc = connect(c->fd, (const struct sockaddr *)c->saddr, c->addrlen);
+    if (rc == 0) {
+        *completed = 1;
+        return REDIS_OK;
+    }
+    switch (errno) {
+    case EISCONN:
+        *completed = 1;
+        return REDIS_OK;
+    case EALREADY:
+    case EINPROGRESS:
+    case EWOULDBLOCK:
+        *completed = 0;
+        return REDIS_OK;
+    default:
+        return REDIS_ERR;
+    }
+}
+
 int redisCheckSocketError(redisContext *c) {
-    int err = 0;
+    int err = 0, errno_saved = errno;
     socklen_t errlen = sizeof(err);
 
     if (getsockopt(c->fd, SOL_SOCKET, SO_ERROR, &err, &errlen) == -1) {
@@ -241,6 +263,10 @@ int redisCheckSocketError(redisContext *c) {
         return REDIS_ERR;
     }
 
+    if (err == 0) {
+        err = errno_saved;
+    }
+
     if (err) {
         errno = err;
         __redisSetErrorFromErrno(c,REDIS_ERR_IO,NULL);
@@ -373,12 +399,27 @@ addrretry:
                 goto error;
             }
         }
+
+        /* For repeat connection */
+        if (c->saddr) {
+            free(c->saddr);
+        }
+        c->saddr = malloc(p->ai_addrlen);
+        memcpy(c->saddr, p->ai_addr, p->ai_addrlen);
+        c->addrlen = p->ai_addrlen;
+
         if (connect(s,p->ai_addr,p->ai_addrlen) == -1) {
             if (errno == EHOSTUNREACH) {
                 redisContextCloseFd(c);
                 continue;
-            } else if (errno == EINPROGRESS && !blocking) {
-                /* This is ok. */
+            } else if (errno == EINPROGRESS) {
+                if (blocking) {
+                    goto wait_for_ready;
+                }
+                /* This is ok.
+                 * Note that even when it's in blocking mode, we unset blocking
+                 * for `connect()`
+                 */
             } else if (errno == EADDRNOTAVAIL && reuseaddr) {
                 if (++reuses >= REDIS_CONNECT_RETRIES) {
                     goto error;
@@ -387,6 +428,7 @@ addrretry:
                     goto addrretry;
                 }
             } else {
+                wait_for_ready:
                 if (redisContextWaitReady(c,timeout_msec) != REDIS_OK)
                     goto error;
             }
diff --git a/net.h b/net.h
index d9dc362..a11594e 100644
--- a/net.h
+++ b/net.h
@@ -45,5 +45,6 @@ int redisContextConnectBindTcp(redisContext *c, const char *addr, int port,
                                const char *source_addr);
 int redisContextConnectUnix(redisContext *c, const char *path, const struct timeval *timeout);
 int redisKeepAlive(redisContext *c, int interval);
+int redisCheckConnectDone(redisContext *c, int *completed);
 
 #endif
diff --git a/test.c b/test.c
index 0f5bfe5..a5eb9c7 100644
--- a/test.c
+++ b/test.c
@@ -4,6 +4,7 @@
 #include <string.h>
 #include <strings.h>
 #include <sys/time.h>
+#include <netdb.h>
 #include <assert.h>
 #include <unistd.h>
 #include <signal.h>
@@ -91,7 +92,7 @@ static int disconnect(redisContext *c, int keep_fd) {
     return -1;
 }
 
-static redisContext *connect(struct config config) {
+static redisContext *do_connect(struct config config) {
     redisContext *c = NULL;
 
     if (config.type == CONN_TCP) {
@@ -248,7 +249,7 @@ static void test_append_formatted_commands(struct config config) {
     char *cmd;
     int len;
 
-    c = connect(config);
+    c = do_connect(config);
 
     test("Append format command: ");
 
@@ -434,18 +435,32 @@ static void test_free_null(void) {
 
 static void test_blocking_connection_errors(void) {
     redisContext *c;
-
-    test("Returns error when host cannot be resolved: ");
-    c = redisConnect((char*)"idontexist.test", 6379);
-    test_cond(c->err == REDIS_ERR_OTHER &&
-        (strcmp(c->errstr,"Name or service not known") == 0 ||
-         strcmp(c->errstr,"Can't resolve: idontexist.test") == 0 ||
-         strcmp(c->errstr,"nodename nor servname provided, or not known") == 0 ||
-         strcmp(c->errstr,"No address associated with hostname") == 0 ||
-         strcmp(c->errstr,"Temporary failure in name resolution") == 0 ||
-         strcmp(c->errstr,"hostname nor servname provided, or not known") == 0 ||
-         strcmp(c->errstr,"no address associated with name") == 0));
-    redisFree(c);
+    struct addrinfo hints = {.ai_family = AF_INET};
+    struct addrinfo *ai_tmp = NULL;
+    const char *bad_domain = "idontexist.com";
+
+    int rv = getaddrinfo(bad_domain, "6379", &hints, &ai_tmp);
+    if (rv != 0) {
+        // Address does *not* exist
+        test("Returns error when host cannot be resolved: ");
+        // First see if this domain name *actually* resolves to NXDOMAIN
+        c = redisConnect("dontexist.com", 6379);
+        test_cond(
+            c->err == REDIS_ERR_OTHER &&
+            (strcmp(c->errstr, "Name or service not known") == 0 ||
+             strcmp(c->errstr, "Can't resolve: sadkfjaskfjsa.com") == 0 ||
+             strcmp(c->errstr,
+                    "nodename nor servname provided, or not known") == 0 ||
+             strcmp(c->errstr, "No address associated with hostname") == 0 ||
+             strcmp(c->errstr, "Temporary failure in name resolution") == 0 ||
+             strcmp(c->errstr,
+                    "hostname nor servname provided, or not known") == 0 ||
+             strcmp(c->errstr, "no address associated with name") == 0));
+        redisFree(c);
+    } else {
+        printf("Skipping NXDOMAIN test. Found evil ISP!\n");
+        freeaddrinfo(ai_tmp);
+    }
 
     test("Returns error when the port is not open: ");
     c = redisConnect((char*)"localhost", 1);
@@ -463,7 +478,7 @@ static void test_blocking_connection(struct config config) {
     redisContext *c;
     redisReply *reply;
 
-    c = connect(config);
+    c = do_connect(config);
 
     test("Is able to deliver commands: ");
     reply = redisCommand(c,"PING");
@@ -544,7 +559,7 @@ static void test_blocking_connection_timeouts(struct config config) {
     const char *cmd = "DEBUG SLEEP 3\r\n";
     struct timeval tv;
 
-    c = connect(config);
+    c = do_connect(config);
     test("Successfully completes a command when the timeout is not exceeded: ");
     reply = redisCommand(c,"SET foo fast");
     freeReplyObject(reply);
@@ -556,7 +571,7 @@ static void test_blocking_connection_timeouts(struct config config) {
     freeReplyObject(reply);
     disconnect(c, 0);
 
-    c = connect(config);
+    c = do_connect(config);
     test("Does not return a reply when the command times out: ");
     s = write(c->fd, cmd, strlen(cmd));
     tv.tv_sec = 0;
@@ -590,7 +605,7 @@ static void test_blocking_io_errors(struct config config) {
     int major, minor;
 
     /* Connect to target given by config. */
-    c = connect(config);
+    c = do_connect(config);
     {
         /* Find out Redis version to determine the path for the next test */
         const char *field = "redis_version:";
@@ -625,7 +640,7 @@ static void test_blocking_io_errors(struct config config) {
         strcmp(c->errstr,"Server closed the connection") == 0);
     redisFree(c);
 
-    c = connect(config);
+    c = do_connect(config);
     test("Returns I/O error on socket timeout: ");
     struct timeval tv = { 0, 1000 };
     assert(redisSetTimeout(c,tv) == REDIS_OK);
@@ -659,7 +674,7 @@ static void test_invalid_timeout_errors(struct config config) {
 }
 
 static void test_throughput(struct config config) {
-    redisContext *c = connect(config);
+    redisContext *c = do_connect(config);
     redisReply **replies;
     int i, num;
     long long t1, t2;