From af95517612c209976352201f5f8f302bf9aef876 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Wed, 12 Jul 2023 14:23:07 -0700 Subject: [PATCH 1/5] Retry poll(2) if we are intterupted. This commit adds logic to retry our poll call when waiting for the connection to complete, in the event that we are interrupted by a signal. Additionally we do some simple bookkeeping to keep track of the overall timeout specified by the user. Fixes #1206 --- net.c | 54 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/net.c b/net.c index 1e01638..c7d8271 100644 --- a/net.c +++ b/net.c @@ -41,6 +41,7 @@ #include #include #include +#include #include "net.h" #include "sds.h" @@ -271,37 +272,54 @@ static int redisContextTimeoutMsec(redisContext *c, long *result) return REDIS_OK; } +static long redisPollMillis(void) { +#ifndef _MSC_VER + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + return (now.tv_sec * 1000) + now.tv_nsec / 1000000; +#else + FILETIME ft; + GetSystemTimeAsFileTime(&ft); + return (((long long)ft.dwHighDateTime << 32) | ft.dwLowDateTime) / 10; +#endif +} + static int redisContextWaitReady(redisContext *c, long msec) { - struct pollfd wfd[1]; + struct pollfd wfd; + long end; + int res; - wfd[0].fd = c->fd; - wfd[0].events = POLLOUT; + if (errno != EINPROGRESS) { + __redisSetErrorFromErrno(c,REDIS_ERR_IO,NULL); + redisNetClose(c); + return REDIS_ERR; + } - if (errno == EINPROGRESS) { - int res; + wfd.fd = c->fd; + wfd.events = POLLOUT; + end = msec >= 0 ? redisPollMillis() + msec : 0; - if ((res = poll(wfd, 1, msec)) == -1) { + while ((res = poll(&wfd, 1, msec)) <= 0) { + if (res < 0 && errno != EINTR) { __redisSetErrorFromErrno(c, REDIS_ERR_IO, "poll(2)"); redisNetClose(c); return REDIS_ERR; - } else if (res == 0) { + } else if (res == 0 || (msec >= 0 && redisPollMillis() >= end)) { errno = ETIMEDOUT; - __redisSetErrorFromErrno(c,REDIS_ERR_IO,NULL); + __redisSetErrorFromErrno(c, REDIS_ERR_IO, NULL); redisNetClose(c); return REDIS_ERR; + } else { + /* res < 0 && errno == EINTR, try again */ } - - if (redisCheckConnectDone(c, &res) != REDIS_OK || res == 0) { - redisCheckSocketError(c); - return REDIS_ERR; - } - - return REDIS_OK; } - __redisSetErrorFromErrno(c,REDIS_ERR_IO,NULL); - redisNetClose(c); - return REDIS_ERR; + if (redisCheckConnectDone(c, &res) != REDIS_OK || res == 0) { + redisCheckSocketError(c); + return REDIS_ERR; + } + + return REDIS_OK; } int redisCheckConnectDone(redisContext *c, int *completed) { From bfe45d9f80ae5802fe3547ceccd03e1a66d41062 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Tue, 25 Jul 2023 10:38:00 -0700 Subject: [PATCH 2/5] Document poll(2) logic changes. See #1206, #1213 --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 74364b4..c0fc2a1 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,17 @@ Redis version >= 1.2.0. The library comes with multiple APIs. There is the *synchronous API*, the *asynchronous API* and the *reply parsing API*. +## Upgrading to > 1.2.0 (**PRERELEASE**) + +* After v1.2.0 we modified how we invoke `poll(2)` to wait for connections to complete, such that we will now retry + the call if it is interrupted by a signal until: + + a) The connection succeeds or fails. + b) The overall connection timeout is reached. + + In previous versions, an interrupted `poll(2)` call would cause the connection to fail + with `c->err` set to `REDIS_ERR_IO` and `c->errstr` set to `poll(2): Interrupted system call`. + ## Upgrading to `1.1.0` Almost all users will simply need to recompile their applications against the newer version of hiredis. From e07ae7d3b6248be8be842eca3e1e97595a17aa1a Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Mon, 17 Jul 2023 10:40:55 -0700 Subject: [PATCH 3/5] Add a panic helper for non-assert aborts. We merged a fix for a "maybe uninitialized" warning in #1209, but after merging there could actually have then been a double free. The reason is that when compiling with NDEBUG our assert macro becomes a no-op, meaning that execution would no longer stop after `assert(NULL)`. This commit just adds a simple panic macro which will execute regardless of whether NDEBUG is defined or not. --- test.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test.c b/test.c index 6ac3ea0..45914af 100644 --- a/test.c +++ b/test.c @@ -104,6 +104,13 @@ static long long usec(void) { #define assert(e) (void)(e) #endif +#define redisTestPanic(msg) \ + do { \ + fprintf(stderr, "PANIC: %s (In function \"%s\", file \"%s\", line %d)\n", \ + msg, __func__, __FILE__, __LINE__); \ + exit(1); \ + } while (1) + /* Helper to extract Redis version information. Aborts on any failure. */ #define REDIS_VERSION_FIELD "redis_version:" void get_redis_version(redisContext *c, int *majorptr, int *minorptr) { @@ -232,7 +239,7 @@ static redisContext *do_connect(struct config config) { c = redisConnectFd(fd); } } else { - assert(NULL); + redisTestPanic("Unknown connection type!"); } if (c == NULL) { @@ -1352,7 +1359,7 @@ static void test_invalid_timeout_errors(struct config config) { } else if(config.type == CONN_UNIX) { c = redisConnectUnixWithTimeout(config.unix_sock.path, config.connect_timeout); } else { - assert(NULL); + redisTestPanic("Unknown connection type!"); } test_cond(c != NULL && c->err == REDIS_ERR_IO && strcmp(c->errstr, "Invalid timeout specified") == 0); @@ -1368,7 +1375,7 @@ static void test_invalid_timeout_errors(struct config config) { } else if(config.type == CONN_UNIX) { c = redisConnectUnixWithTimeout(config.unix_sock.path, config.connect_timeout); } else { - assert(NULL); + redisTestPanic("Unknown connection type!"); } test_cond(c != NULL && c->err == REDIS_ERR_IO && strcmp(c->errstr, "Invalid timeout specified") == 0); From 039385bd8b9192a3994d75e75e377933f17d1f47 Mon Sep 17 00:00:00 2001 From: Chayim Date: Sat, 19 Aug 2023 07:06:47 +0300 Subject: [PATCH 4/5] Integrating spellcheck into CI (#1218) * Adding spellcheck testing * words * updating version of spellcheck action --- .github/spellcheck-settings.yml | 29 ++++++++++ .github/wordlist.txt | 99 ++++++++++++++++++++++++++++++++ .github/workflows/spellcheck.yml | 14 +++++ README.md | 8 +-- 4 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 .github/spellcheck-settings.yml create mode 100644 .github/wordlist.txt create mode 100644 .github/workflows/spellcheck.yml diff --git a/.github/spellcheck-settings.yml b/.github/spellcheck-settings.yml new file mode 100644 index 0000000..b8ca6cc --- /dev/null +++ b/.github/spellcheck-settings.yml @@ -0,0 +1,29 @@ +matrix: +- name: Markdown + expect_match: false + apsell: + lang: en + d: en_US + ignore-case: true + dictionary: + wordlists: + - .github/wordlist.txt + output: wordlist.dic + pipeline: + - pyspelling.filters.markdown: + markdown_extensions: + - markdown.extensions.extra: + - pyspelling.filters.html: + comments: false + attributes: + - alt + ignores: + - ':matches(code, pre)' + - code + - pre + - blockquote + - img + sources: + - 'README.md' + - 'FAQ.md' + - 'docs/**' diff --git a/.github/wordlist.txt b/.github/wordlist.txt new file mode 100644 index 0000000..244bf5b --- /dev/null +++ b/.github/wordlist.txt @@ -0,0 +1,99 @@ +ABI +ACLs +alloc +Allocator +allocators +antirez +api +APIs +ASYNC +asyncRedisContext +asyncronous +AUTOFREE +autoload +autoloader +autoloading +Autoloading +backend +backends +behaviour +boolean +CAS +Changelog +customizable +Customizable +CVE +dataset +de +deallocation +ElastiCache +extensibility +FPM +getaddrinfo +gmail +grunder +Grunder +hiredis +Hiredis +HIREDIS +hostname +IANA +IPv +IPV +keepalive +keyspace +keyspaces +KiB +libc +libev +libevent +localhost +Lua +michael +minimalistic +namespace +NOAUTOFREE +NOAUTOFREEREPLIES +NONBLOCK +Noordhuis +OpenSSL +Packagist +pcnoordhuis +PhpRedis +Pieter +pipelined +pipelining +pluggable +Predis +PRERELEASE +printf +PSR +PSUBSCRIBE +rb +Readme +README +rebalanced +rebalancing +redis +Redis +redisAsyncContext +redisContext +redisOptions +redisReader +reusability +REUSEADDR +runtime +Sanfilippo +SHA +sharding +SONAME +SSL +struct +stunnel +subelements +TCP +TLS +unparsed +UNSPEC +URI +variadic diff --git a/.github/workflows/spellcheck.yml b/.github/workflows/spellcheck.yml new file mode 100644 index 0000000..e152841 --- /dev/null +++ b/.github/workflows/spellcheck.yml @@ -0,0 +1,14 @@ +name: spellcheck +on: + pull_request: +jobs: + check-spelling: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v3 + - name: Check Spelling + uses: rojopolis/spellcheck-github-actions@0.33.1 + with: + config_path: .github/spellcheck-settings.yml + task_name: Markdown diff --git a/README.md b/README.md index c0fc2a1..f704641 100644 --- a/README.md +++ b/README.md @@ -28,11 +28,11 @@ The library comes with multiple APIs. There is the * After v1.2.0 we modified how we invoke `poll(2)` to wait for connections to complete, such that we will now retry the call if it is interrupted by a signal until: - a) The connection succeeds or fails. + a) The connection succeeds or fails. b) The overall connection timeout is reached. - In previous versions, an interrupted `poll(2)` call would cause the connection to fail - with `c->err` set to `REDIS_ERR_IO` and `c->errstr` set to `poll(2): Interrupted system call`. + In previous versions, an interrupted `poll(2)` call would cause the connection to fail + with `c->err` set to `REDIS_ERR_IO` and `c->errstr` set to `poll(2): Interrupted system call`. ## Upgrading to `1.1.0` @@ -302,7 +302,7 @@ void redisFree(redisContext *c); This function immediately closes the socket and then frees the allocations done in creating the context. -### Sending commands (cont'd) +### Sending commands (continued) Together with `redisCommand`, the function `redisCommandArgv` can be used to issue commands. It has the following prototype: From 869f3d0ef1513dd0258ad7190c9914df16dcc4a4 Mon Sep 17 00:00:00 2001 From: Michael Grunder Date: Fri, 18 Aug 2023 21:07:25 -0700 Subject: [PATCH 5/5] Make redisEnableKeepAlive a no-op on AF_UNIX connections. (#1215) Fixes #1185 --- .gitignore | 1 + net.c | 4 ++++ test.c | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+) diff --git a/.gitignore b/.gitignore index 056959f..c223f29 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ /*.pc *.dSYM tags +compile_commands.json diff --git a/net.c b/net.c index c7d8271..8b7831e 100644 --- a/net.c +++ b/net.c @@ -173,6 +173,10 @@ int redisKeepAlive(redisContext *c, int interval) { int val = 1; redisFD fd = c->fd; + /* TCP_KEEPALIVE makes no sense with AF_UNIX connections */ + if (c->connection_type == REDIS_CONN_UNIX) + return REDIS_ERR; + #ifndef _WIN32 if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &val, sizeof(val)) == -1){ __redisSetError(c,REDIS_ERR_OTHER,strerror(errno)); diff --git a/test.c b/test.c index 45914af..897daf8 100644 --- a/test.c +++ b/test.c @@ -431,6 +431,24 @@ static void test_tcp_options(struct config cfg) { redisFree(c); } +static void test_unix_keepalive(struct config cfg) { + redisContext *c; + redisReply *r; + + c = do_connect(cfg); + + test("Setting TCP_KEEPALIVE on a unix socket returns an error: "); + test_cond(redisEnableKeepAlive(c) == REDIS_ERR && c->err == 0); + + test("Setting TCP_KEEPALIVE on a unix socket doesn't break the connection: "); + r = redisCommand(c, "PING"); + test_cond(r != NULL && r->type == REDIS_REPLY_STATUS && r->len == 4 && + !memcmp(r->str, "PONG", 4)); + freeReplyObject(r); + + redisFree(c); +} + static void test_reply_reader(void) { redisReader *reader; void *reply, *root; @@ -2363,6 +2381,7 @@ int main(int argc, char **argv) { test_blocking_connection_timeouts(cfg); test_blocking_io_errors(cfg); test_invalid_timeout_errors(cfg); + test_unix_keepalive(cfg); if (throughput) test_throughput(cfg); } else { test_skipped();