Win32 tests and timeout fix (#776)

Unit tests in Windows and a Windows timeout fix

This commit gets our unit tests compiling and running on Windows as well as removes a duplicated `timeval` -> `DWORD` conversion logic in sockcompat.c 

There are minor differences in behavior between Linux and Windows to note:

1.  In Windows, opening a non-existent hangs forever in WSAPoll whereas
    it correctly returns with a "Connection refused" error on Linux.
    For that reason, I simply skip this test in Windows.

    It may be related to this known issue:
    https://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/

2.  Timeouts are handled slightly differently in Windows and Linux.  
    In Linux, we intentionally set REDIS_ERR_IO for connection
    timeouts whereas in Windows we set REDIS_ERR_TIMEOUT.  It may be
    prudent to fix this discrepancy although there are almost certainly
    users relying on the current behavior.

.travis.yml

@@ -1,5 +1,4 @@
 language: c
-sudo: false
 compiler:
   - gcc
   - clang
@@ -62,9 +61,9 @@ script:
   - mkdir build/ && cd build/
   - cmake .. ${EXTRA_CMAKE_OPTS}
   - make VERBOSE=1
-  - ctest -V
+  - SKIPS_AS_FAILS=1 ctest -V
 
-matrix:
+jobs:
   include:
     # Windows MinGW cross compile on Linux
     - os: linux
@@ -90,8 +89,12 @@ matrix:
         - eval "${MATRIX_EVAL}"
       install:
         - choco install ninja
+        - choco install redis-64 -y
+      before_script:
+        - redis-server --service-install --service-name Redis --port 6379
+        - redis-server --service-start
       script:
         - mkdir build && cd build
         - cmd.exe //C 'C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Auxiliary\Build\vcvarsall.bat' amd64 '&&'
           cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release '&&' ninja -v
-        - ctest -V
+        - ./hiredis-test.exe

CMakeLists.txt

@@ -33,17 +33,10 @@ SET(hiredis_sources
     sockcompat.c
     alloc.c)
 
-IF(WIN32)
-    SET(hiredis_sources
-        ${hiredis_sources}
-        hiredis.def
-    )
-ENDIF()
-
 ADD_LIBRARY(hiredis SHARED ${hiredis_sources})
 
 SET_TARGET_PROPERTIES(hiredis
-    PROPERTIES
+    PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS TRUE
     VERSION "${HIREDIS_SONAME}")
 IF(WIN32 OR MINGW)
     TARGET_LINK_LIBRARIES(hiredis PRIVATE ws2_32)
@@ -87,7 +80,7 @@ IF(ENABLE_SSL)
         DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)
 ENDIF()
 
-IF(NOT (DISABLE_TESTS OR (WIN32 OR MINGW)))
+IF(NOT DISABLE_TESTS)
     ENABLE_TESTING()
     ADD_EXECUTABLE(hiredis-test test.c)
     TARGET_LINK_LIBRARIES(hiredis-test hiredis)

hiredis.def

@@ -1,38 +0,0 @@
-EXPORTS
-	redisAppendCommand
-	redisAppendCommandArgv
-	redisAppendFormattedCommand
-	redisBufferRead
-	redisBufferWrite
-	redisCommand
-	redisCommand
-	redisCommandArgv
-	redisConnect
-	redisConnectBindNonBlock
-	redisConnectBindNonBlockWithReuse
-	redisConnectFd
-	redisConnectNonBlock
-	redisConnectUnix
-	redisConnectUnixNonBlock
-	redisConnectUnixWithTimeout
-	redisConnectWithOptions
-	redisConnectWithTimeout
-	redisEnableKeepAlive
-	redisFormatCommand
-	redisFormatCommandArgv
-	redisFormatSdsCommandArgv
-	redisFree
-	redisFreeCommand
-	redisFreeKeepFd
-	redisFreeSdsCommand
-	redisGetReply
-	redisGetReplyFromReader
-	redisReaderCreate
-	redisReconnect
-	redisSetTimeout
-	redisvAppendCommand
-	redisvCommand
-	redisvFormatCommand
-	freeReplyObject
-	hi_calloc
-	hi_malloc

net.c

@@ -316,11 +316,7 @@ int redisCheckSocketError(redisContext *c) {
 int redisContextSetTimeout(redisContext *c, const struct timeval tv) {
     const void *to_ptr = &tv;
     size_t to_sz = sizeof(tv);
-#ifdef _WIN32
+
-    DWORD timeout_msec = tv.tv_sec * 1000 + tv.tv_usec / 1000;
-    to_ptr = &timeout_msec;
-    to_sz = sizeof(timeout_msec);
-#endif
     if (setsockopt(c->fd,SOL_SOCKET,SO_RCVTIMEO,to_ptr,to_sz) == -1) {
         __redisSetErrorFromErrno(c,REDIS_ERR_IO,"setsockopt(SO_RCVTIMEO)");
         return REDIS_ERR;

test.c

@@ -1,13 +1,13 @@
 #include "fmacros.h"
+#include "sockcompat.h"
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#ifndef _WIN32
 #include <strings.h>
-#include <sys/socket.h>
 #include <sys/time.h>
-#include <netdb.h>
+#endif
 #include <assert.h>
-#include <unistd.h>
 #include <signal.h>
 #include <errno.h>
 #include <limits.h>
@@ -17,6 +17,7 @@
 #include "hiredis_ssl.h"
 #endif
 #include "net.h"
+#include "win32.h"
 
 enum connection_type {
     CONN_TCP,
@@ -48,14 +49,21 @@ struct config {
 };
 
 /* The following lines make up our testing "framework" :) */
-static int tests = 0, fails = 0;
+static int tests = 0, fails = 0, skips = 0;
 #define test(_s) { printf("#%02d ", ++tests); printf(_s); }
 #define test_cond(_c) if(_c) printf("\033[0;32mPASSED\033[0;0m\n"); else {printf("\033[0;31mFAILED\033[0;0m\n"); fails++;}
+#define test_skipped() { printf("\033[01;33mSKIPPED\033[0;0m\n"); skips++; }
 
 static long long usec(void) {
+#ifndef _MSC_VER
     struct timeval tv;
     gettimeofday(&tv,NULL);
     return (((long long)tv.tv_sec)*1000000)+tv.tv_usec;
+#else
+    FILETIME ft;
+    GetSystemTimeAsFileTime(&ft);
+    return (((long long)ft.dwHighDateTime << 32) | ft.dwLowDateTime) / 10;
+#endif
 }
 
 /* The assert() calls below have side effects, so we need assert()
@@ -491,19 +499,19 @@ static void test_blocking_connection_errors(void) {
             (strcmp(c->errstr, "Name or service not known") == 0 ||
              strcmp(c->errstr, "Can't resolve: " HIREDIS_BAD_DOMAIN) == 0 ||
              strcmp(c->errstr, "Name does not resolve") == 0 ||
-             strcmp(c->errstr,
+             strcmp(c->errstr, "nodename nor servname provided, or not known") == 0 ||
-                    "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,
+             strcmp(c->errstr, "hostname nor servname provided, or not known") == 0 ||
-                    "hostname nor servname provided, or not known") == 0 ||
+             strcmp(c->errstr, "no address associated with name") == 0 ||
-             strcmp(c->errstr, "no address associated with name") == 0));
+             strcmp(c->errstr, "No such host is known. ") == 0));
         redisFree(c);
     } else {
         printf("Skipping NXDOMAIN test. Found evil ISP!\n");
         freeaddrinfo(ai_tmp);
     }
 
+#ifndef _WIN32
     test("Returns error when the port is not open: ");
     c = redisConnect((char*)"localhost", 1);
     test_cond(c->err == REDIS_ERR_IO &&
@@ -514,6 +522,7 @@ static void test_blocking_connection_errors(void) {
     c = redisConnectUnix((char*)"/tmp/idontexist.sock");
     test_cond(c->err == REDIS_ERR_IO); /* Don't care about the message... */
     redisFree(c);
+#endif
 }
 
 static void test_blocking_connection(struct config config) {
@@ -599,11 +608,28 @@ static void test_blocking_connection(struct config config) {
     disconnect(c, 0);
 }
 
+/* Send DEBUG SLEEP 0 to detect if we have this command */
+static int detect_debug_sleep(redisContext *c) {
+    int detected;
+    redisReply *reply = redisCommand(c, "DEBUG SLEEP 0\r\n");
+
+    if (reply == NULL || c->err) {
+        const char *cause = c->err ? c->errstr : "(none)";
+        fprintf(stderr, "Error testing for DEBUG SLEEP (Redis error: %s), exiting\n", cause);
+        exit(-1);
+    }
+
+    detected = reply->type == REDIS_REPLY_STATUS;
+    freeReplyObject(reply);
+
+    return detected;
+}
+
 static void test_blocking_connection_timeouts(struct config config) {
     redisContext *c;
     redisReply *reply;
     ssize_t s;
-    const char *cmd = "DEBUG SLEEP 3\r\n";
+    const char *sleep_cmd = "DEBUG SLEEP 3\r\n";
     struct timeval tv;
 
     c = do_connect(config);
@@ -620,14 +646,24 @@ static void test_blocking_connection_timeouts(struct config config) {
 
     c = do_connect(config);
     test("Does not return a reply when the command times out: ");
-    redisAppendFormattedCommand(c, cmd, strlen(cmd));
+    if (detect_debug_sleep(c)) {
-    s = c->funcs->write(c);
+        redisAppendFormattedCommand(c, sleep_cmd, strlen(sleep_cmd));
-    tv.tv_sec = 0;
+        s = c->funcs->write(c);
-    tv.tv_usec = 10000;
+        tv.tv_sec = 0;
-    redisSetTimeout(c, tv);
+        tv.tv_usec = 10000;
-    reply = redisCommand(c, "GET foo");
+        redisSetTimeout(c, tv);
-    test_cond(s > 0 && reply == NULL && c->err == REDIS_ERR_IO && strcmp(c->errstr, "Resource temporarily unavailable") == 0);
+        reply = redisCommand(c, "GET foo");
-    freeReplyObject(reply);
+#ifndef _WIN32
+        test_cond(s > 0 && reply == NULL && c->err == REDIS_ERR_IO &&
+                  strcmp(c->errstr, "Resource temporarily unavailable") == 0);
+#else
+        test_cond(s > 0 && reply == NULL && c->err == REDIS_ERR_TIMEOUT &&
+                  strcmp(c->errstr, "recv timeout") == 0);
+#endif
+        freeReplyObject(reply);
+    } else {
+        test_skipped();
+    }
 
     test("Reconnect properly reconnects after a timeout: ");
     do_reconnect(c, config);
@@ -679,6 +715,7 @@ static void test_blocking_io_errors(struct config config) {
         test_cond(reply == NULL);
     }
 
+#ifndef _WIN32
     /* On 2.0, QUIT will cause the connection to be closed immediately and
      * the read(2) for the reply on QUIT will set the error to EOF.
      * On >2.0, QUIT will return with OK and another read(2) needed to be
@@ -686,14 +723,19 @@ static void test_blocking_io_errors(struct config config) {
      * conditions, the error will be set to EOF. */
     assert(c->err == REDIS_ERR_EOF &&
         strcmp(c->errstr,"Server closed the connection") == 0);
+#endif
     redisFree(c);
 
     c = do_connect(config);
     test("Returns I/O error on socket timeout: ");
     struct timeval tv = { 0, 1000 };
     assert(redisSetTimeout(c,tv) == REDIS_OK);
-    test_cond(redisGetReply(c,&_reply) == REDIS_ERR &&
+    int respcode = redisGetReply(c,&_reply);
-        c->err == REDIS_ERR_IO && errno == EAGAIN);
+#ifndef _WIN32
+    test_cond(respcode == REDIS_ERR && c->err == REDIS_ERR_IO && errno == EAGAIN);
+#else
+    test_cond(respcode == REDIS_ERR && c->err == REDIS_ERR_TIMEOUT);
+#endif
     redisFree(c);
 }
 
@@ -921,9 +963,8 @@ int main(int argc, char **argv) {
     };
     int throughput = 1;
     int test_inherit_fd = 1;
-
+    int skips_as_fails = 0;
-    /* Ignore broken pipe signal (for I/O error tests). */
+    int test_unix_socket;
-    signal(SIGPIPE, SIG_IGN);
 
     /* Parse command line options. */
     argv++; argc--;
@@ -941,6 +982,8 @@ int main(int argc, char **argv) {
             throughput = 0;
         } else if (argc >= 1 && !strcmp(argv[0],"--skip-inherit-fd")) {
             test_inherit_fd = 0;
+        } else if (argc >= 1 && !strcmp(argv[0],"--skips-as-fails")) {
+            skips_as_fails = 1;
 #ifdef HIREDIS_TEST_SSL
         } else if (argc >= 2 && !strcmp(argv[0],"--ssl-port")) {
             argv++; argc--;
@@ -965,6 +1008,16 @@ int main(int argc, char **argv) {
         argv++; argc--;
     }
 
+#ifndef _WIN32
+    /* Ignore broken pipe signal (for I/O error tests). */
+    signal(SIGPIPE, SIG_IGN);
+
+    test_unix_socket = access(cfg.unix_sock.path, F_OK) == 0;
+#else
+    /* Unix sockets don't exist in Windows */
+    test_unix_socket = 0;
+#endif
+
     test_format_commands();
     test_reply_reader();
     test_blocking_connection_errors();
@@ -979,12 +1032,17 @@ int main(int argc, char **argv) {
     test_append_formatted_commands(cfg);
     if (throughput) test_throughput(cfg);
 
-    printf("\nTesting against Unix socket connection (%s):\n", cfg.unix_sock.path);
+    printf("\nTesting against Unix socket connection (%s): ", cfg.unix_sock.path);
-    cfg.type = CONN_UNIX;
+    if (test_unix_socket) {
-    test_blocking_connection(cfg);
+        printf("\n");
-    test_blocking_connection_timeouts(cfg);
+        cfg.type = CONN_UNIX;
-    test_blocking_io_errors(cfg);
+        test_blocking_connection(cfg);
-    if (throughput) test_throughput(cfg);
+        test_blocking_connection_timeouts(cfg);
+        test_blocking_io_errors(cfg);
+        if (throughput) test_throughput(cfg);
+    } else {
+        test_skipped();
+    }
 
 #ifdef HIREDIS_TEST_SSL
     if (cfg.ssl.port && cfg.ssl.host) {
@@ -1001,17 +1059,24 @@ int main(int argc, char **argv) {
 #endif
 
     if (test_inherit_fd) {
-        printf("\nTesting against inherited fd (%s):\n", cfg.unix_sock.path);
+        printf("\nTesting against inherited fd (%s): ", cfg.unix_sock.path);
-        cfg.type = CONN_FD;
+        if (test_unix_socket) {
-        test_blocking_connection(cfg);
+            printf("\n");
+            cfg.type = CONN_FD;
+            test_blocking_connection(cfg);
+        } else {
+            test_skipped();
+        }
     }
 
-
+    if (fails || (skips_as_fails && skips)) {
-    if (fails) {
         printf("*** %d TESTS FAILED ***\n", fails);
+        if (skips) {
+            printf("*** %d TESTS SKIPPED ***\n", skips);
+        }
         return 1;
     }
 
-    printf("ALL TESTS PASSED\n");
+    printf("ALL TESTS PASSED (%d skipped)\n", skips);
     return 0;
 }

test.sh

@@ -4,7 +4,9 @@ REDIS_SERVER=${REDIS_SERVER:-redis-server}
 REDIS_PORT=${REDIS_PORT:-56379}
 REDIS_SSL_PORT=${REDIS_SSL_PORT:-56443}
 TEST_SSL=${TEST_SSL:-0}
+SKIPS_AS_FAILS=${SKIPS_AS_FAILS-:0}
 SSL_TEST_ARGS=
+SKIPS_ARG=
 
 tmpdir=$(mktemp -d)
 PID_FILE=${tmpdir}/hiredis-test-redis.pid
@@ -67,4 +69,10 @@ fi
 cat ${tmpdir}/redis.conf
 ${REDIS_SERVER} ${tmpdir}/redis.conf
 
-${TEST_PREFIX:-} ./hiredis-test -h 127.0.0.1 -p ${REDIS_PORT} -s ${SOCK_FILE} ${SSL_TEST_ARGS}
+# Wait until we detect the unix socket
+while [ ! -S "${SOCK_FILE}" ]; do sleep 1; done
+
+# Treat skips as failures if directed
+[ "$SKIPS_AS_FAILS" = 1 ] && SKIPS_ARG="--skips-as-fails"
+
+${TEST_PREFIX:-} ./hiredis-test -h 127.0.0.1 -p ${REDIS_PORT} -s ${SOCK_FILE} ${SSL_TEST_ARGS} ${SKIPS_ARG}