From a5f0fa7f7aa2cbd506dbf0ae2101dc540d4e8faf Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 29 May 2020 11:07:13 +0200 Subject: [PATCH 01/29] Fix handling of special chars in ACL LOAD. Now it is also possible for ACL SETUSER to accept empty strings as valid operations (doing nothing), so for instance ACL SETUSER myuser "" Will have just the effect of creating a user in the default state. This should fix #7329. --- src/acl.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/acl.c b/src/acl.c index c3a724c33..6dd0f70ac 100644 --- a/src/acl.c +++ b/src/acl.c @@ -732,10 +732,11 @@ void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub) { * EEXIST: You are adding a key pattern after "*" was already added. This is * almost surely an error on the user side. * ENODEV: The password you are trying to remove from the user does not exist. - * EBADMSG: The hash you are trying to add is not a valid hash. + * EBADMSG: The hash you are trying to add is not a valid hash. */ int ACLSetUser(user *u, const char *op, ssize_t oplen) { if (oplen == -1) oplen = strlen(op); + if (oplen == 0) return C_OK; /* Empty string is a no-operation. */ if (!strcasecmp(op,"on")) { u->flags |= USER_FLAG_ENABLED; u->flags &= ~USER_FLAG_DISABLED; @@ -1297,7 +1298,7 @@ sds ACLLoadFromFile(const char *filename) { if (lines[i][0] == '\0') continue; /* Split into arguments */ - argv = sdssplitargs(lines[i],&argc); + argv = sdssplitlen(lines[i],sdslen(lines[i])," ",1,&argc); if (argv == NULL) { errors = sdscatprintf(errors, "%s:%d: unbalanced quotes in acl line. ", @@ -1329,11 +1330,14 @@ sds ACLLoadFromFile(const char *filename) { continue; } - /* Try to process the line using the fake user to validate iif - * the rules are able to apply cleanly. */ + /* Try to process the line using the fake user to validate if + * the rules are able to apply cleanly. At this stage we also + * trim trailing spaces, so that we don't have to handle that + * in ACLSetUser(). */ ACLSetUser(fakeuser,"reset",-1); int j; for (j = 2; j < argc; j++) { + argv[j] = sdstrim(argv[j],"\t\r\n"); if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) != C_OK) { char *errmsg = ACLSetUserStringError(); errors = sdscatprintf(errors, From 123d94ffc09bb3f42eaa54d2fe092c5ee91eb88f Mon Sep 17 00:00:00 2001 From: Liu Zhen Date: Wed, 27 May 2020 11:55:23 +0800 Subject: [PATCH 02/29] fix clusters mixing accidentally by gossip `clusterStartHandshake` will start hand handshake and eventually send CLUSTER MEET message, which is strictly prohibited in the REDIS CLUSTER SPEC. Only system administrator can initiate CLUSTER MEET message. Futher, according to the SPEC, rather than IP/PORT pairs, only nodeid can be trusted. --- src/cluster.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index a2fab323a..24b14d1dc 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -1463,7 +1463,10 @@ void clusterProcessGossipSection(clusterMsg *hdr, clusterLink *link) { } } else { /* If it's not in NOADDR state and we don't have it, we - * start a handshake process against this IP/PORT pairs. + * add it to our trusted dict with exact nodeid and flag. + * Note that we cannot simply start a handshake against + * this IP/PORT pairs, since IP/PORT can be reused already, + * otherwise we risk joining another cluster. * * Note that we require that the sender of this gossip message * is a well known node in our cluster, otherwise we risk @@ -1472,7 +1475,12 @@ void clusterProcessGossipSection(clusterMsg *hdr, clusterLink *link) { !(flags & CLUSTER_NODE_NOADDR) && !clusterBlacklistExists(g->nodename)) { - clusterStartHandshake(g->ip,ntohs(g->port),ntohs(g->cport)); + clusterNode *node; + node = createClusterNode(g->nodename, flags); + memcpy(node->ip,g->ip,NET_IP_STR_LEN); + node->port = ntohs(g->port); + node->cport = ntohs(g->cport); + clusterAddNode(node); } } From 8bfdba1e083899a4cec6351db6900f3848fbda27 Mon Sep 17 00:00:00 2001 From: antirez Date: Sat, 6 Jun 2020 11:42:41 +0200 Subject: [PATCH 03/29] Revert "avoid using sendfile if tls-replication is enabled" This reverts commit 13bbd165e87923558952203d310e9ad053d4d7c0. --- src/replication.c | 65 +++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 36 deletions(-) diff --git a/src/replication.c b/src/replication.c index 063a8705e..7b15944a0 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1067,46 +1067,39 @@ void sendBulkToSlave(connection *conn) { } } - /* If the preamble was already transferred, send the RDB bulk data. - * try to use sendfile system call if supported, unless tls is enabled. - * fallback to normal read+write otherwise. */ - nwritten = 0; + /* If the preamble was already transferred, send the RDB bulk data. */ #if HAVE_SENDFILE - if (!server.tls_replication) { - if ((nwritten = redis_sendfile(conn->fd,slave->repldbfd, - slave->repldboff,PROTO_IOBUF_LEN)) == -1) - { - if (errno != EAGAIN) { - serverLog(LL_WARNING,"Sendfile error sending DB to replica: %s", - strerror(errno)); - freeClient(slave); - } - return; + if ((nwritten = redis_sendfile(conn->fd,slave->repldbfd, + slave->repldboff,PROTO_IOBUF_LEN)) == -1) + { + if (errno != EAGAIN) { + serverLog(LL_WARNING,"Sendfile error sending DB to replica: %s", + strerror(errno)); + freeClient(slave); } + return; + } +#else + ssize_t buflen; + char buf[PROTO_IOBUF_LEN]; + + lseek(slave->repldbfd,slave->repldboff,SEEK_SET); + buflen = read(slave->repldbfd,buf,PROTO_IOBUF_LEN); + if (buflen <= 0) { + serverLog(LL_WARNING,"Read error sending DB to replica: %s", + (buflen == 0) ? "premature EOF" : strerror(errno)); + freeClient(slave); + return; + } + if ((nwritten = connWrite(conn,buf,buflen)) == -1) { + if (connGetState(conn) != CONN_STATE_CONNECTED) { + serverLog(LL_WARNING,"Write error sending DB to replica: %s", + connGetLastError(conn)); + freeClient(slave); + } + return; } #endif - if (!nwritten) { - ssize_t buflen; - char buf[PROTO_IOBUF_LEN]; - - lseek(slave->repldbfd,slave->repldboff,SEEK_SET); - buflen = read(slave->repldbfd,buf,PROTO_IOBUF_LEN); - if (buflen <= 0) { - serverLog(LL_WARNING,"Read error sending DB to replica: %s", - (buflen == 0) ? "premature EOF" : strerror(errno)); - freeClient(slave); - return; - } - if ((nwritten = connWrite(conn,buf,buflen)) == -1) { - if (connGetState(conn) != CONN_STATE_CONNECTED) { - serverLog(LL_WARNING,"Write error sending DB to replica: %s", - connGetLastError(conn)); - freeClient(slave); - } - return; - } - } - slave->repldboff += nwritten; server.stat_net_output_bytes += nwritten; if (slave->repldboff == slave->repldbsize) { From bec1ac9899902a68592c0e7d018002275d6053c6 Mon Sep 17 00:00:00 2001 From: antirez Date: Sat, 6 Jun 2020 11:42:45 +0200 Subject: [PATCH 04/29] Revert "Implements sendfile for redis." This reverts commit 5675053269b0cbc2cf525c99321c96b7c2b39abe. --- src/config.h | 6 ------ src/replication.c | 51 ++--------------------------------------------- 2 files changed, 2 insertions(+), 55 deletions(-) diff --git a/src/config.h b/src/config.h index 23de5d3ad..0fcc42972 100644 --- a/src/config.h +++ b/src/config.h @@ -133,12 +133,6 @@ void setproctitle(const char *fmt, ...); /* Byte ordering detection */ #include /* This will likely define BYTE_ORDER */ -/* Define redis_sendfile. */ -#if defined(__linux__) || (defined(__APPLE__) && defined(MAC_OS_X_VERSION_10_5)) -#define HAVE_SENDFILE 1 -ssize_t redis_sendfile(int out_fd, int in_fd, off_t offset, size_t count); -#endif - #ifndef BYTE_ORDER #if (BSD >= 199103) # include diff --git a/src/replication.c b/src/replication.c index 7b15944a0..d9bff79ad 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1008,41 +1008,10 @@ void removeRDBUsedToSyncReplicas(void) { } } -#if HAVE_SENDFILE -/* Implements redis_sendfile to transfer data between file descriptors and - * avoid transferring data to and from user space. - * - * The function prototype is just like sendfile(2) on Linux. in_fd is a file - * descriptor opened for reading and out_fd is a descriptor opened for writing. - * offset specifies where to start reading data from in_fd. count is the number - * of bytes to copy between the file descriptors. - * - * The return value is the number of bytes written to out_fd, if the transfer - * was successful. On error, -1 is returned, and errno is set appropriately. */ -ssize_t redis_sendfile(int out_fd, int in_fd, off_t offset, size_t count) { -#if defined(__linux__) - #include - return sendfile(out_fd, in_fd, &offset, count); - -#elif defined(__APPLE__) - off_t len = count; - /* Notice that it may return -1 and errno is set to EAGAIN even if some - * bytes have been sent successfully and the len argument is set correctly - * when using a socket marked for non-blocking I/O. */ - if (sendfile(in_fd, out_fd, offset, &len, NULL, 0) == -1 && - errno != EAGAIN) return -1; - else - return (ssize_t)len; - -#endif - errno = ENOSYS; - return -1; -} -#endif - void sendBulkToSlave(connection *conn) { client *slave = connGetPrivateData(conn); - ssize_t nwritten; + char buf[PROTO_IOBUF_LEN]; + ssize_t nwritten, buflen; /* Before sending the RDB file, we send the preamble as configured by the * replication process. Currently the preamble is just the bulk count of @@ -1068,21 +1037,6 @@ void sendBulkToSlave(connection *conn) { } /* If the preamble was already transferred, send the RDB bulk data. */ -#if HAVE_SENDFILE - if ((nwritten = redis_sendfile(conn->fd,slave->repldbfd, - slave->repldboff,PROTO_IOBUF_LEN)) == -1) - { - if (errno != EAGAIN) { - serverLog(LL_WARNING,"Sendfile error sending DB to replica: %s", - strerror(errno)); - freeClient(slave); - } - return; - } -#else - ssize_t buflen; - char buf[PROTO_IOBUF_LEN]; - lseek(slave->repldbfd,slave->repldboff,SEEK_SET); buflen = read(slave->repldbfd,buf,PROTO_IOBUF_LEN); if (buflen <= 0) { @@ -1099,7 +1053,6 @@ void sendBulkToSlave(connection *conn) { } return; } -#endif slave->repldboff += nwritten; server.stat_net_output_bytes += nwritten; if (slave->repldboff == slave->repldbsize) { From ce014c2a3b9c9c8e60659b4efda12f0a4c230b32 Mon Sep 17 00:00:00 2001 From: Kevin Fwu Date: Wed, 27 May 2020 08:53:29 -0400 Subject: [PATCH 05/29] Fix TLS certificate loading for chained certificates. This impacts client verification for chained certificates (such as Lets Encrypt certificates). Client Verify requires the full chain in order to properly verify the certificate. --- src/tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tls.c b/src/tls.c index 28a74df9a..a62f2284e 100644 --- a/src/tls.c +++ b/src/tls.c @@ -217,7 +217,7 @@ int tlsConfigure(redisTLSContextConfig *ctx_config) { SSL_CTX_set_ecdh_auto(ctx, 1); #endif - if (SSL_CTX_use_certificate_file(ctx, ctx_config->cert_file, SSL_FILETYPE_PEM) <= 0) { + if (SSL_CTX_use_certificate_chain_file(ctx, ctx_config->cert_file) <= 0) { ERR_error_string_n(ERR_get_error(), errbuf, sizeof(errbuf)); serverLog(LL_WARNING, "Failed to load certificate: %s: %s", ctx_config->cert_file, errbuf); goto error; From fed743b2e1b2a963d5819a25a88111342bac0829 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 31 May 2020 15:51:52 +0300 Subject: [PATCH 06/29] fix pingoff test race --- tests/integration/psync2-pingoff.tcl | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/psync2-pingoff.tcl b/tests/integration/psync2-pingoff.tcl index 5a9a46d16..cdecfc5c6 100644 --- a/tests/integration/psync2-pingoff.tcl +++ b/tests/integration/psync2-pingoff.tcl @@ -64,6 +64,7 @@ start_server {} { # make sure replication is still alive and kicking $R(1) incr x wait_for_condition 50 1000 { + [status $R(0) loading] == 0 && [$R(0) get x] == 1 } else { fail "replica didn't get incr" From 2a8ee551761cce326ef3bdb75af214beeed77958 Mon Sep 17 00:00:00 2001 From: "zhaozhao.zz" Date: Tue, 2 Jun 2020 11:34:28 +0800 Subject: [PATCH 07/29] donot free protected client in freeClientsInAsyncFreeQueue related #7234 --- src/networking.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/networking.c b/src/networking.c index 8d3e057b7..cc28732d1 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1239,14 +1239,20 @@ void freeClientAsync(client *c) { /* Free the clietns marked as CLOSE_ASAP, return the number of clients * freed. */ int freeClientsInAsyncFreeQueue(void) { - int freed = listLength(server.clients_to_close); - while (listLength(server.clients_to_close)) { - listNode *ln = listFirst(server.clients_to_close); + int freed = 0; + listIter li; + listNode *ln; + + listRewind(server.clients_to_close,&li); + while ((ln = listNext(&li)) != NULL) { client *c = listNodeValue(ln); + if (c->flags & CLIENT_PROTECTED) continue; + c->flags &= ~CLIENT_CLOSE_ASAP; freeClient(c); listDelNode(server.clients_to_close,ln); + freed++; } return freed; } From 216b5a1aae775086c6072d11dce1e8208c6858c8 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 8 Jun 2020 09:50:06 +0300 Subject: [PATCH 08/29] fix disconnectSlaves, to try to free each slave. the recent change in that loop (iteration rather than waiting for it to be empty) was intended to avoid an endless loop in case some slave would refuse to be freed. but the lookup of the first client remained, which would have caused it to try the first one again and again instead of moving on. --- src/networking.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index cc28732d1..80973109c 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1019,7 +1019,6 @@ void disconnectSlaves(void) { listNode *ln; listRewind(server.slaves,&li); while((ln = listNext(&li))) { - listNode *ln = listFirst(server.slaves); freeClient((client*)ln->value); } } From d496ce727125c3fe348b7c3fd02f9042d155a1ed Mon Sep 17 00:00:00 2001 From: "zhaozhao.zz" Date: Wed, 3 Jun 2020 17:55:18 +0800 Subject: [PATCH 09/29] AOF: append origin SET if no expire option --- src/aof.c | 21 +++++++++++++-------- tests/unit/expire.tcl | 10 ++++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/aof.c b/src/aof.c index 02409abe6..6f8e53712 100644 --- a/src/aof.c +++ b/src/aof.c @@ -611,19 +611,24 @@ void feedAppendOnlyFile(struct redisCommand *cmd, int dictid, robj **argv, int a } else if (cmd->proc == setCommand && argc > 3) { int i; robj *exarg = NULL, *pxarg = NULL; - /* Translate SET [EX seconds][PX milliseconds] to SET and PEXPIREAT */ - buf = catAppendOnlyGenericCommand(buf,3,argv); for (i = 3; i < argc; i ++) { if (!strcasecmp(argv[i]->ptr, "ex")) exarg = argv[i+1]; if (!strcasecmp(argv[i]->ptr, "px")) pxarg = argv[i+1]; } serverAssert(!(exarg && pxarg)); - if (exarg) - buf = catAppendOnlyExpireAtCommand(buf,server.expireCommand,argv[1], - exarg); - if (pxarg) - buf = catAppendOnlyExpireAtCommand(buf,server.pexpireCommand,argv[1], - pxarg); + + if (exarg || pxarg) { + /* Translate SET [EX seconds][PX milliseconds] to SET and PEXPIREAT */ + buf = catAppendOnlyGenericCommand(buf,3,argv); + if (exarg) + buf = catAppendOnlyExpireAtCommand(buf,server.expireCommand,argv[1], + exarg); + if (pxarg) + buf = catAppendOnlyExpireAtCommand(buf,server.pexpireCommand,argv[1], + pxarg); + } else { + buf = catAppendOnlyGenericCommand(buf,argc,argv); + } } else { /* All the other commands don't need translation or need the * same translation already operated in the command vector diff --git a/tests/unit/expire.tcl b/tests/unit/expire.tcl index 11fb82ef0..52d174d75 100644 --- a/tests/unit/expire.tcl +++ b/tests/unit/expire.tcl @@ -232,4 +232,14 @@ start_server {tags {"expire"}} { set ttl [r ttl foo] assert {$ttl <= 100 && $ttl > 90} } + + test {SET - use KEEPTTL option, TTL should not be removed after loadaof} { + r config set appendonly yes + r set foo bar EX 100 + r set foo bar2 KEEPTTL + after 2000 + r debug loadaof + set ttl [r ttl foo] + assert {$ttl <= 98 && $ttl > 90} + } } From 881d2c466300837b5a11d0b450478377906176cc Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 8 Jun 2020 09:16:32 +0300 Subject: [PATCH 10/29] Avoid rejecting WATCH / UNWATCH, like MULTI/EXEC/DISCARD Much like MULTI/EXEC/DISCARD, the WATCH and UNWATCH are not actually operating on the database or server state, but instead operate on the client state. the client may send them all in one long pipeline and check all the responses only at the end, so failing them may lead to a mismatch between the client state on the server and the one on the client end, and execute the wrong commands (ones that were meant to be discarded) the watched keys are not actually stored in the client struct, but they are in fact part of the client state. for instance, they're not cleared or moved in SWAPDB or FLUSHDB. --- src/server.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index b7a6a928f..e8e711240 100644 --- a/src/server.c +++ b/src/server.c @@ -776,11 +776,11 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"watch",watchCommand,-2, - "no-script fast @transaction", + "no-script fast ok-loading ok-stale @transaction", 0,NULL,1,-1,1,0,0,0}, {"unwatch",unwatchCommand,1, - "no-script fast @transaction", + "no-script fast ok-loading ok-stale @transaction", 0,NULL,0,0,0,0,0,0}, {"cluster",clusterCommand,-2, @@ -3627,6 +3627,8 @@ int processCommand(client *c) { c->cmd->proc != multiCommand && c->cmd->proc != execCommand && c->cmd->proc != discardCommand && + c->cmd->proc != watchCommand && + c->cmd->proc != unwatchCommand && !(c->cmd->proc == shutdownCommand && c->argc == 2 && tolower(((char*)c->argv[1]->ptr)[0]) == 'n') && From 640a4479b02323ad445de68c5ffa0b66b56e2c36 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 8 Jun 2020 09:43:10 +0300 Subject: [PATCH 11/29] Don't queue commands in an already aborted MULTI state --- src/multi.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/multi.c b/src/multi.c index a331a6240..3e606fcec 100644 --- a/src/multi.c +++ b/src/multi.c @@ -58,6 +58,13 @@ void queueMultiCommand(client *c) { multiCmd *mc; int j; + /* No sense to waste memory if the transaction is already aborted. + * this is useful in case client sends these in a pipeline, or doesn't + * bother to read previous responses and didn't notice the multi was already + * aborted. */ + if (c->flags & CLIENT_DIRTY_EXEC) + return; + c->mstate.commands = zrealloc(c->mstate.commands, sizeof(multiCmd)*(c->mstate.count+1)); mc = c->mstate.commands+c->mstate.count; From 5fae6590a15475f3a4a5d284f116781eef93744e Mon Sep 17 00:00:00 2001 From: xhe Date: Sun, 7 Jun 2020 13:34:55 +0800 Subject: [PATCH 12/29] return the correct proto version HELLO should return the current proto version, while the code hardcoded 3 --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 80973109c..77b9a6fcf 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2527,7 +2527,7 @@ void helloCommand(client *c) { addReplyBulkCString(c,REDIS_VERSION); addReplyBulkCString(c,"proto"); - addReplyLongLong(c,3); + addReplyLongLong(c,ver); addReplyBulkCString(c,"id"); addReplyLongLong(c,c->id); From e819e3ee26fb1e71b6e13fc73bed9449e49d5d4a Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 9 Jun 2020 11:52:33 +0200 Subject: [PATCH 13/29] Temporary fix for #7353 issue about EVAL during -BUSY. --- src/multi.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/multi.c b/src/multi.c index 3e606fcec..60a07dfc7 100644 --- a/src/multi.c +++ b/src/multi.c @@ -135,6 +135,15 @@ void execCommand(client *c) { return; } + /* If we are in -BUSY state, flag the transaction and return the + * -BUSY error, like Redis <= 5. This is a temporary fix, may be changed + * ASAP, see issue #7353 on Github. */ + if (server.lua_timedout) { + flagTransaction(c); + addReply(c, shared.slowscripterr); + return; + } + /* Check if we need to abort the EXEC because: * 1) Some WATCHed key was touched. * 2) There was a previous error while queueing commands. From 93e76ba45629c7a3b123626e991b60c019ea251e Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 9 Jun 2020 12:19:14 +0200 Subject: [PATCH 14/29] Adapt EVAL+busy script test to new behavior. --- tests/unit/multi.tcl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/multi.tcl b/tests/unit/multi.tcl index 55f18bec8..0c70fbde7 100644 --- a/tests/unit/multi.tcl +++ b/tests/unit/multi.tcl @@ -363,6 +363,9 @@ start_server {tags {"multi"}} { set xx [r get xx] # make sure that either the whole transcation passed or none of it (we actually expect none) assert { $xx == 1 || $xx == 3} + # Discard the transaction since EXEC likely got -BUSY error + # so the client is still in MULTI state. + catch { $rd2 discard ;$rd2 read } e # check that the connection is no longer in multi state $rd2 ping asdf set pong [$rd2 read] From ce8f107a5ebc1631cd950a0381a9cee3eccb015d Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 9 Jun 2020 12:00:15 +0200 Subject: [PATCH 15/29] Redis 6.0.5. --- 00-RELEASENOTES | 70 +++++++++++++++++++++++++++++++++++++++++++++++++ src/version.h | 2 +- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/00-RELEASENOTES b/00-RELEASENOTES index 7ce88c556..c6ee44246 100644 --- a/00-RELEASENOTES +++ b/00-RELEASENOTES @@ -11,6 +11,76 @@ CRITICAL: There is a critical bug affecting MOST USERS. Upgrade ASAP. SECURITY: There are security fixes in the release. -------------------------------------------------------------------------------- +================================================================================ +Redis 6.0.5 Released Tue Jun 09 11:56:08 CEST 2020 +================================================================================ + +Upgrade urgency MODERATE: several bugs with moderate impact are fixed here. + +The most important issues are listed here: + +* Fix handling of speical chars in ACL LOAD. +* Make Redis Cluster more robust about operation errors that may lead + to two clusters to mix together. +* Revert the sendfile() implementation of RDB transfer. It causes some delay. +* Fix TLS certificate loading for chained certificates. +* Fix AOF rewirting of KEEPTTL SET option. +* Fix MULTI/EXEC behavior during -BUSY script errors. + +And this is the full list of commits: + +antirez in commit ee8dd01bb: + Temporary fix for #7353 issue about EVAL during -BUSY. + 1 file changed, 9 insertions(+) + +xhe in commit a4a856d53: + return the correct proto version HELLO should return the current proto version, while the code hardcoded 3 + 1 file changed, 1 insertion(+), 1 deletion(-) + +Oran Agra in commit e2046b300: + Don't queue commands in an already aborted MULTI state + 1 file changed, 7 insertions(+) + +Oran Agra in commit b35fdf1de: + Avoid rejecting WATCH / UNWATCH, like MULTI/EXEC/DISCARD + 1 file changed, 4 insertions(+), 2 deletions(-) + +zhaozhao.zz in commit 1d7bf208c: + AOF: append origin SET if no expire option + 2 files changed, 23 insertions(+), 8 deletions(-) + +Oran Agra in commit 676445ad9: + fix disconnectSlaves, to try to free each slave. + 1 file changed, 1 deletion(-) + +zhaozhao.zz in commit 4846c0c8a: + donot free protected client in freeClientsInAsyncFreeQueue + 1 file changed, 9 insertions(+), 3 deletions(-) + +Oran Agra in commit f33de403e: + fix pingoff test race + 1 file changed, 1 insertion(+) + +Kevin Fwu in commit 49af4d07e: + Fix TLS certificate loading for chained certificates. + 1 file changed, 1 insertion(+), 1 deletion(-) + +antirez in commit 329fddbda: + Revert "Implements sendfile for redis." + 2 files changed, 2 insertions(+), 55 deletions(-) + +antirez in commit 925a2cd5a: + Revert "avoid using sendfile if tls-replication is enabled" + 1 file changed, 27 insertions(+), 34 deletions(-) + +Liu Zhen in commit 84a7a9058: + fix clusters mixing accidentally by gossip + 1 file changed, 10 insertions(+), 2 deletions(-) + +antirez in commit cd63359a1: + Fix handling of special chars in ACL LOAD. + 1 file changed, 8 insertions(+), 4 deletions(-) + ================================================================================ Redis 6.0.4 Released Thu May 28 11:36:45 CEST 2020 ================================================================================ diff --git a/src/version.h b/src/version.h index 8efc3c5aa..e1eb096f3 100644 --- a/src/version.h +++ b/src/version.h @@ -1 +1 @@ -#define REDIS_VERSION "6.0.4" +#define REDIS_VERSION "6.0.5" From 49fecbe1d40e19353756fd5a8e2a0a8bc8e37092 Mon Sep 17 00:00:00 2001 From: John Sully Date: Tue, 9 Jun 2020 12:43:58 -0400 Subject: [PATCH 16/29] Implement replicaof remove as requested in issue #192 Former-commit-id: 70b80aa5fad6c2191c2142ce49626b81d0950fa8 --- src/replication.cpp | 37 ++++++++++++++++++++++++++++++++++--- src/server.cpp | 2 +- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/replication.cpp b/src/replication.cpp index dae37fc72..768a53f0c 100644 --- a/src/replication.cpp +++ b/src/replication.cpp @@ -3151,15 +3151,46 @@ void replicaofCommand(client *c) { return; } - /* The special host/port combination "NO" "ONE" turns the instance - * into a master. Otherwise the new master address is set. */ - if (!strcasecmp((const char*)ptrFromObj(c->argv[1]),"no") && + if (c->argc > 3) { + if (c->argc != 4) { + addReplyError(c, "Invalid arguments"); + return; + } + if (!strcasecmp((const char*)ptrFromObj(c->argv[1]),"remove")) { + listIter li; + listNode *ln; + bool fRemoved = false; + long port; + string2l(szFromObj(c->argv[3]), sdslen(szFromObj(c->argv[3])), &port); + LRestart: + listRewind(g_pserver->masters, &li); + while ((ln = listNext(&li))) { + redisMaster *mi = (redisMaster*)listNodeValue(ln); + if (mi->masterport != port) + continue; + if (sdscmp(szFromObj(c->argv[2]), mi->masterhost) == 0) { + replicationUnsetMaster(mi); + fRemoved = true; + goto LRestart; + } + } + if (!fRemoved) { + addReplyError(c, "Master not found"); + return; + } else if (listLength(g_pserver->masters) == 0) { + goto LLogNoMaster; + } + } + } else if (!strcasecmp((const char*)ptrFromObj(c->argv[1]),"no") && !strcasecmp((const char*)ptrFromObj(c->argv[2]),"one")) { + /* The special host/port combination "NO" "ONE" turns the instance + * into a master. Otherwise the new master address is set. */ if (listLength(g_pserver->masters)) { while (listLength(g_pserver->masters)) { replicationUnsetMaster((redisMaster*)listNodeValue(listFirst(g_pserver->masters))); } + LLogNoMaster: sds client = catClientInfoString(sdsempty(),c); serverLog(LL_NOTICE,"MASTER MODE enabled (user request from '%s')", client); diff --git a/src/server.cpp b/src/server.cpp index 4fa6ce725..c197df741 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -768,7 +768,7 @@ struct redisCommand redisCommandTable[] = { "admin no-script ok-stale", 0,NULL,0,0,0,0,0,0}, - {"replicaof",replicaofCommand,3, + {"replicaof",replicaofCommand,-3, "admin no-script ok-stale", 0,NULL,0,0,0,0,0,0}, From c4cd846388939d5bce2f6bbc9f82f104494bb8b7 Mon Sep 17 00:00:00 2001 From: Muhammad Zahalqa Date: Sat, 13 Jun 2020 19:13:38 +0300 Subject: [PATCH 17/29] aeCommand objects no need for memset and missing init of some memebers. Former-commit-id: aeed09c34620e465b57f1d83553d259c4f6d892f --- src/ae.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ae.cpp b/src/ae.cpp index 1a323a7dd..5d306d980 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -234,7 +234,7 @@ int aeCreateRemoteFileEvent(aeEventLoop *eventLoop, int fd, int mask, int ret = AE_OK; - aeCommand cmd = {}; + aeCommand cmd; cmd.op = AE_ASYNC_OP::CreateFileEvent; cmd.fd = fd; cmd.mask = mask; @@ -294,7 +294,6 @@ int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynch } aeCommand cmd = {}; - memset(&cmd, 0, sizeof(aeCommand)); cmd.op = AE_ASYNC_OP::PostCppFunction; cmd.pfn = new (MALLOC_LOCAL) std::function(fn); cmd.pctl = nullptr; @@ -454,7 +453,7 @@ void aeDeleteFileEventAsync(aeEventLoop *eventLoop, int fd, int mask) { if (eventLoop == g_eventLoopThisThread) return aeDeleteFileEvent(eventLoop, fd, mask); - aeCommand cmd; + aeCommand cmd = {}; cmd.op = AE_ASYNC_OP::DeleteFileEvent; cmd.fd = fd; cmd.mask = mask; From e25ec374840e2215c7b03afc54ee829f16c1fa99 Mon Sep 17 00:00:00 2001 From: Muhammad Zahalqa Date: Sat, 13 Jun 2020 17:32:48 +0300 Subject: [PATCH 18/29] fixes for robj_sharedptr 1. fix cases where null pointer might be accessed 2. make assignmnet op safe 3. make operator bool explicit (safe bool idiom) 4. make comparison operators symetric fix robj_sharedptr use in rdb.cpp Former-commit-id: ede524c0647c0875f1071978f26ff785c8d1183e --- src/rdb.cpp | 5 ++- src/server.h | 89 +++++++++++++++++++++++++++++----------------------- 2 files changed, 52 insertions(+), 42 deletions(-) diff --git a/src/rdb.cpp b/src/rdb.cpp index ea82cdbd8..5ee6baa25 100644 --- a/src/rdb.cpp +++ b/src/rdb.cpp @@ -2412,9 +2412,8 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) { if (fStaleMvccKey && !fExpiredKey && rsi != nullptr && rsi->mi != nullptr && rsi->mi->staleKeyMap != nullptr && lookupKeyRead(db, &keyobj) == nullptr) { // We have a key that we've already deleted and is not back in our database. // We'll need to inform the sending master of the delete if it is also a replica of us - robj *objKeyDup = createStringObject(key, sdslen(key)); - rsi->mi->staleKeyMap->operator[](db - g_pserver->db).push_back(objKeyDup); - decrRefCount(objKeyDup); + robj_sharedptr objKeyDup(createStringObject(key, sdslen(key))); + rsi->mi->staleKeyMap->operator[](db - g_pserver->db).push_back(objKeyDup); } fLastKeyExpired = true; sdsfree(key); diff --git a/src/server.h b/src/server.h index 712ffacb4..3a6375b32 100644 --- a/src/server.h +++ b/src/server.h @@ -166,22 +166,24 @@ class robj_sharedptr public: robj_sharedptr() - : m_ptr(nullptr) - {} - robj_sharedptr(redisObject *ptr) - : m_ptr(ptr) - { + : m_ptr(nullptr) + {} + explicit robj_sharedptr(redisObject *ptr) + : m_ptr(ptr) + { + if(m_ptr) incrRefCount(ptr); - } + } ~robj_sharedptr() { if (m_ptr) decrRefCount(m_ptr); } robj_sharedptr(const robj_sharedptr& other) - { - m_ptr = other.m_ptr; - incrRefCount(m_ptr); + : m_ptr(other.m_ptr) + { + if(m_ptr) + incrRefCount(m_ptr); } robj_sharedptr(robj_sharedptr&& other) @@ -192,41 +194,19 @@ public: robj_sharedptr &operator=(const robj_sharedptr& other) { - if (m_ptr) - decrRefCount(m_ptr); - m_ptr = other.m_ptr; - incrRefCount(m_ptr); + robj_sharedptr tmp(other); + using std::swap; + swap(m_ptr, tmp.m_ptr); return *this; } robj_sharedptr &operator=(redisObject *ptr) { - if (m_ptr) - decrRefCount(m_ptr); - m_ptr = ptr; - incrRefCount(m_ptr); + robj_sharedptr tmp(ptr); + using std::swap; + swap(m_ptr, tmp.m_ptr); return *this; } - - bool operator==(const robj_sharedptr &other) const - { - return m_ptr == other.m_ptr; - } - - bool operator==(const void *p) const - { - return m_ptr == p; - } - - bool operator!=(const robj_sharedptr &other) const - { - return m_ptr != other.m_ptr; - } - - bool operator!=(const void *p) const - { - return m_ptr != p; - } - + redisObject* operator->() const { return m_ptr; @@ -237,7 +217,7 @@ public: return !m_ptr; } - operator bool() const{ + explicit operator bool() const{ return !!m_ptr; } @@ -247,8 +227,39 @@ public: } redisObject *get() { return m_ptr; } + const redisObject *get() const { return m_ptr; } }; +inline bool operator==(const robj_sharedptr &lhs, const robj_sharedptr &rhs) +{ + return lhs.get() == rhs.get(); +} + +inline bool operator!=(const robj_sharedptr &lhs, const robj_sharedptr &rhs) +{ + return !(lhs == rhs); +} + +inline bool operator==(const robj_sharedptr &lhs, const void *p) +{ + return lhs.get() == p; +} + +inline bool operator==(const void *p, const robj_sharedptr &rhs) +{ + return rhs == p; +} + +inline bool operator!=(const robj_sharedptr &lhs, const void *p) +{ + return !(lhs == p); +} + +inline bool operator!=(const void *p, const robj_sharedptr &rhs) +{ + return !(rhs == p); +} + /* Error codes */ #define C_OK 0 #define C_ERR -1 From 63bb6cb41991f8eadc20d305ea19dfb2778663c1 Mon Sep 17 00:00:00 2001 From: Muhammad Zahalqa Date: Sat, 13 Jun 2020 20:14:51 +0300 Subject: [PATCH 19/29] Fix bug in condition_variabe.wait usage consition variable wait should always be called with a locked mutex. This code break the pre-conditions of using wait. Former-commit-id: 5b81303f04580010f043e518a109b43971a63054 --- src/ae.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ae.cpp b/src/ae.cpp index 5d306d980..1e81cbb4e 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -258,7 +258,7 @@ int aeCreateRemoteFileEvent(aeEventLoop *eventLoop, int fd, int mask, if (fSynchronous) { - std::unique_lock ulock(cmd.pctl->mutexcv, std::defer_lock); + std::unique_lock ulock(cmd.pctl->mutexcv); cmd.pctl->cv.wait(ulock); ret = cmd.pctl->rval; delete cmd.pctl; @@ -311,7 +311,7 @@ int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynch int ret = AE_OK; if (fSynchronous) { - std::unique_lock ulock(cmd.pctl->mutexcv, std::defer_lock); + std::unique_lock ulock(cmd.pctl->mutexcv); cmd.pctl->cv.wait(ulock); ret = cmd.pctl->rval; delete cmd.pctl; From ec281612719eb2a0c0e0aa65133bb56f36fc4dcd Mon Sep 17 00:00:00 2001 From: Muhammad Zahalqa Date: Sun, 14 Jun 2020 02:39:24 +0300 Subject: [PATCH 20/29] unique lock used with cv.wait already owns mutex Former-commit-id: 8bc70e9840222d11507af30a6db27210b16650a2 --- src/ae.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ae.cpp b/src/ae.cpp index 1e81cbb4e..44f302e69 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -258,7 +258,7 @@ int aeCreateRemoteFileEvent(aeEventLoop *eventLoop, int fd, int mask, if (fSynchronous) { - std::unique_lock ulock(cmd.pctl->mutexcv); + std::unique_lock ulock(cmd.pctl->mutexcv, std::adopt_lock); cmd.pctl->cv.wait(ulock); ret = cmd.pctl->rval; delete cmd.pctl; @@ -311,7 +311,7 @@ int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynch int ret = AE_OK; if (fSynchronous) { - std::unique_lock ulock(cmd.pctl->mutexcv); + std::unique_lock ulock(cmd.pctl->mutexcv, std::adopt_lock); cmd.pctl->cv.wait(ulock); ret = cmd.pctl->rval; delete cmd.pctl; From f024fe30f4ead9381d147ed2c5e38b1d11f6ade4 Mon Sep 17 00:00:00 2001 From: Muhammad Zahalqa Date: Sat, 13 Jun 2020 18:26:26 +0300 Subject: [PATCH 21/29] replace memcpy with cctor on objects Former-commit-id: 52897f8f9a882bcdbd1e8ede6bdf24e7435f5ce8 --- src/db.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/db.cpp b/src/db.cpp index dbb309734..770a1c9b4 100644 --- a/src/db.cpp +++ b/src/db.cpp @@ -1219,8 +1219,7 @@ int dbSwapDatabases(long id1, long id2) { if (id1 < 0 || id1 >= cserver.dbnum || id2 < 0 || id2 >= cserver.dbnum) return C_ERR; if (id1 == id2) return C_OK; - redisDb aux; - memcpy(&aux, &g_pserver->db[id1], sizeof(redisDb)); + redisDb aux(g_pserver->db[id1]); redisDb *db1 = &g_pserver->db[id1], *db2 = &g_pserver->db[id2]; /* Swap hash tables. Note that we don't swap blocking_keys, From ffc3aefc41c22b61e14e59927eed0a23541a5a83 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 1 Jul 2020 21:59:38 -0400 Subject: [PATCH 22/29] Latency fixes Former-commit-id: 6857c4c2085d3b0902d7b2ece66b3fe8828dd805 --- src/connection.cpp | 14 +++++++------- src/networking.cpp | 9 ++++++--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/connection.cpp b/src/connection.cpp index 05f49f689..defd5eb9d 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -164,7 +164,7 @@ static int connSocketWrite(connection *conn, const void *data, size_t data_len) int ret = write(conn->fd, data, data_len); if (ret < 0 && errno != EAGAIN) { conn->last_errno = errno; - conn->state = CONN_STATE_ERROR; + conn->state.store(CONN_STATE_ERROR, std::memory_order_relaxed); } return ret; @@ -173,10 +173,10 @@ static int connSocketWrite(connection *conn, const void *data, size_t data_len) static int connSocketRead(connection *conn, void *buf, size_t buf_len) { int ret = read(conn->fd, buf, buf_len); if (!ret) { - conn->state = CONN_STATE_CLOSED; + conn->state.store(CONN_STATE_CLOSED, std::memory_order_release); } else if (ret < 0 && errno != EAGAIN) { conn->last_errno = errno; - conn->state = CONN_STATE_ERROR; + conn->state.store(CONN_STATE_ERROR, std::memory_order_release); } return ret; @@ -253,14 +253,14 @@ static void connSocketEventHandler(struct aeEventLoop *el, int fd, void *clientD UNUSED(fd); connection *conn = (connection*)clientData; - if (conn->state == CONN_STATE_CONNECTING && + if (conn->state.load(std::memory_order_relaxed) == CONN_STATE_CONNECTING && (mask & AE_WRITABLE) && conn->conn_handler) { if (connGetSocketError(conn)) { conn->last_errno = errno; - conn->state = CONN_STATE_ERROR; + conn->state.store(CONN_STATE_ERROR, std::memory_order_release); } else { - conn->state = CONN_STATE_CONNECTED; + conn->state.store(CONN_STATE_CONNECTED, std::memory_order_release); } if (!conn->write_handler) aeDeleteFileEvent(serverTL->el,conn->fd,AE_WRITABLE); @@ -407,7 +407,7 @@ int connRecvTimeout(connection *conn, long long ms) { } int connGetState(connection *conn) { - return conn->state; + return conn->state.load(std::memory_order_relaxed); } void connSetThreadAffinity(connection *conn, int cpu) { diff --git a/src/networking.cpp b/src/networking.cpp index 619db0786..f54870795 100644 --- a/src/networking.cpp +++ b/src/networking.cpp @@ -1928,9 +1928,12 @@ int handleClientsWithPendingWrites(int iel, int aof_state) { } } - AeLocker locker; - locker.arm(nullptr); - ProcessPendingAsyncWrites(); + if (listLength(serverTL->clients_pending_asyncwrite)) + { + AeLocker locker; + locker.arm(nullptr); + ProcessPendingAsyncWrites(); + } return processed; } From 3af243a447ec38dd9900a155f4bc80f864de473d Mon Sep 17 00:00:00 2001 From: John Sully Date: Sat, 11 Jul 2020 21:38:17 +0000 Subject: [PATCH 23/29] Support missing Redis 6 config options Former-commit-id: f111c234152fd47d21c8c95029bcb191641182a7 --- src/config.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/config.cpp b/src/config.cpp index 490e872bc..8b318420d 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -518,7 +518,7 @@ void loadServerConfigFromString(char *config) { err = "KeyDB not compliled with scratch-file support."; goto loaderr; #endif - } else if (!strcasecmp(argv[0],"server-threads") && argc == 2) { + } else if ((!strcasecmp(argv[0],"server-threads") || !strcasecmp(argv[0],"io-threads")) && argc == 2) { cserver.cthreads = atoi(argv[1]); if (cserver.cthreads <= 0 || cserver.cthreads > MAX_EVENT_LOOPS) { err = "Invalid number of threads specified"; @@ -2185,6 +2185,7 @@ static int updateTlsCfgBool(int val, int prev, const char **err) { } #endif /* USE_OPENSSL */ +int fDummy = false; standardConfig configs[] = { /* Bool configs */ createBoolConfig("rdbchecksum", NULL, IMMUTABLE_CONFIG, g_pserver->rdb_checksum, 1, NULL, NULL), @@ -2200,6 +2201,7 @@ standardConfig configs[] = { createBoolConfig("lazyfree-lazy-eviction", NULL, MODIFIABLE_CONFIG, g_pserver->lazyfree_lazy_eviction, 0, NULL, NULL), createBoolConfig("lazyfree-lazy-expire", NULL, MODIFIABLE_CONFIG, g_pserver->lazyfree_lazy_expire, 0, NULL, NULL), createBoolConfig("lazyfree-lazy-server-del", NULL, MODIFIABLE_CONFIG, g_pserver->lazyfree_lazy_server_del, 0, NULL, NULL), + createBoolConfig("lazyfree-lazy-user-del", NULL, MODIFIABLE_CONFIG, g_pserver->lazyfree_lazy_user_del , 0, NULL, NULL), createBoolConfig("repl-disable-tcp-nodelay", NULL, MODIFIABLE_CONFIG, g_pserver->repl_disable_tcp_nodelay, 0, NULL, NULL), createBoolConfig("repl-diskless-sync", NULL, MODIFIABLE_CONFIG, g_pserver->repl_diskless_sync, 0, NULL, NULL), createBoolConfig("aof-rewrite-incremental-fsync", NULL, MODIFIABLE_CONFIG, g_pserver->aof_rewrite_incremental_fsync, 1, NULL, NULL), From 785779ee409cf43b3dd018a6cec09c61adbaacb0 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 12 Jul 2020 01:13:22 +0000 Subject: [PATCH 24/29] Fix failure to merge databases on active replica sync, due to bad merge with Redis 6 Former-commit-id: cd9514f4c8624932df2ec60ae3c2244899844aa6 --- src/replication.cpp | 6 ++++-- tests/integration/replication-active.tcl | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/replication.cpp b/src/replication.cpp index 768a53f0c..b502cf0e7 100644 --- a/src/replication.cpp +++ b/src/replication.cpp @@ -2069,7 +2069,6 @@ void readSyncBulkPayload(connection *conn) { * * 2. Or when we are done reading from the socket to the RDB file, in * such case we want just to read the RDB file in memory. */ - serverLog(LL_NOTICE, "MASTER <-> REPLICA sync: Flushing old data"); /* We need to stop any AOF rewriting child before flusing and parsing * the RDB, otherwise we'll create a copy-on-write disaster. */ @@ -2089,7 +2088,10 @@ void readSyncBulkPayload(connection *conn) { * (Where disklessLoadMakeBackups left server.db empty) because we * want to execute all the auxiliary logic of emptyDb (Namely, * fire module events) */ - emptyDb(-1,empty_db_flags,replicationEmptyDbCallback); + if (!fUpdate) { + serverLog(LL_NOTICE, "MASTER <-> REPLICA sync: Flushing old data"); + emptyDb(-1,empty_db_flags,replicationEmptyDbCallback); + } /* Before loading the DB into memory we need to delete the readable * handler, otherwise it will get called recursively since diff --git a/tests/integration/replication-active.tcl b/tests/integration/replication-active.tcl index 0fca9a829..6c3c6d674 100644 --- a/tests/integration/replication-active.tcl +++ b/tests/integration/replication-active.tcl @@ -237,3 +237,18 @@ start_server {tags {"active-repl"} overrides {active-replica yes}} { } } } + +start_server {tags {"active-repl"} overrides {active-replica yes}} { + set slave [srv 0 client] + set slave_host [srv 0 host] + set slave_port [srv 0 port] + start_server {tags {"active-repl"} overrides { active-replica yes}} { + r set testkeyB bar + test {Active Replica Merges Database On Sync} { + $slave set testkeyA foo + r replicaof $slave_host $slave_port + after 1000 + assert_equal 2 [r dbsize] + } + } +} From 6158cdfd29d0cd874d4bdca10748e5dfd6006348 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 12 Jul 2020 18:17:53 +0000 Subject: [PATCH 25/29] Prevent deadlock in RM_ThreadSafeContextLock() when triggered as part of a module callback in a server thread Former-commit-id: e01312642be3cc78e7b383dee958a9b5c0ffc103 --- src/module.cpp | 21 +++++++++++++++++++-- tests/modules/hooks.c | 3 +++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/module.cpp b/src/module.cpp index 859236e72..f6702d1d0 100644 --- a/src/module.cpp +++ b/src/module.cpp @@ -4895,14 +4895,17 @@ void RM_FreeThreadSafeContext(RedisModuleCtx *ctx) { zfree(ctx); } +static bool g_fModuleThread = false; /* Acquire the server lock before executing a thread safe API call. * This is not needed for `RedisModule_Reply*` calls when there is * a blocked client connected to the thread safe context. */ void RM_ThreadSafeContextLock(RedisModuleCtx *ctx) { UNUSED(ctx); - moduleAcquireGIL(FALSE /*fServerThread*/); - if (serverTL == nullptr) + if (serverTL == nullptr) { serverTL = &g_pserver->rgthreadvar[IDX_EVENT_LOOP_MAIN]; // arbitrary module threads get the main thread context + g_fModuleThread = true; + } + moduleAcquireGIL(FALSE /*fServerThread*/); } /* Release the server lock after a thread safe API call was executed. */ @@ -4911,10 +4914,20 @@ void RM_ThreadSafeContextUnlock(RedisModuleCtx *ctx) { moduleReleaseGIL(FALSE /*fServerThread*/); } +// A module may be triggered synchronously in a non-module context. In this scenario we don't lock again +// as the server thread acquisition is sufficient. If we did try to lock we would deadlock +static bool FModuleCallBackLock(bool fServerThread) +{ + return !fServerThread && aeThreadOwnsLock() && !g_fModuleThread && s_cAcquisitionsServer > 0; +} void moduleAcquireGIL(int fServerThread) { std::unique_lock lock(s_mutex); int *pcheck = fServerThread ? &s_cAcquisitionsModule : &s_cAcquisitionsServer; + if (FModuleCallBackLock(fServerThread)) { + return; + } + while (*pcheck > 0) s_cv.wait(lock); @@ -4932,6 +4945,10 @@ void moduleAcquireGIL(int fServerThread) { void moduleReleaseGIL(int fServerThread) { std::unique_lock lock(s_mutex); + if (FModuleCallBackLock(fServerThread)) { + return; + } + if (fServerThread) { --s_cAcquisitionsServer; diff --git a/tests/modules/hooks.c b/tests/modules/hooks.c index 665a20481..ff08c3d36 100644 --- a/tests/modules/hooks.c +++ b/tests/modules/hooks.c @@ -30,6 +30,7 @@ * POSSIBILITY OF SUCH DAMAGE. */ +#define REDISMODULE_EXPERIMENTAL_API 1 #include "redismodule.h" #include #include @@ -143,10 +144,12 @@ void flushdbCallback(RedisModuleCtx *ctx, RedisModuleEvent e, uint64_t sub, void { REDISMODULE_NOT_USED(e); + RedisModule_ThreadSafeContextLock(ctx); RedisModuleFlushInfo *fi = data; char *keyname = (sub == REDISMODULE_SUBEVENT_FLUSHDB_START) ? "flush-start" : "flush-end"; LogNumericEvent(ctx, keyname, fi->dbnum); + RedisModule_ThreadSafeContextUnlock(ctx); } void roleChangeCallback(RedisModuleCtx *ctx, RedisModuleEvent e, uint64_t sub, void *data) From c6e5ba831330a49b657cf719620dab759e105dde Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 12 Jul 2020 18:49:02 +0000 Subject: [PATCH 26/29] Document min-clients-per-thread Former-commit-id: 1b9ade7f657d9b142f78a46565e65b52904dad47 --- keydb.conf | 3 +++ 1 file changed, 3 insertions(+) diff --git a/keydb.conf b/keydb.conf index bd702b016..43ce0c8a6 100644 --- a/keydb.conf +++ b/keydb.conf @@ -1811,6 +1811,9 @@ jemalloc-bg-thread yes # Set bgsave child process to cpu affinity 1,10,11 # bgsave_cpulist 1,10-11 +# The minimum number of clients on a thread before KeyDB assigns new connections to a different thread +# Tuning this parameter is a tradeoff between locking overhead and distributing the workload over multiple cores +# min-clients-per-thread 50 # Path to directory for file backed scratchpad. The file backed scratchpad # reduces memory requirements by storing rarely accessed data on disk From f8531420838ce9a2049a36622e503a8e0a1b5b3b Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 12 Jul 2020 19:25:19 +0000 Subject: [PATCH 27/29] Add multi-master-no-forward command to reduce bus traffic with multi-master Former-commit-id: d99d06b1250a51ea4bc54f678f451acbb7901e33 --- keydb.conf | 6 ++++++ src/config.cpp | 8 ++++++++ src/replication.cpp | 2 +- src/server.h | 1 + tests/integration/replication-multimaster.tcl | 15 +++++++++++---- 5 files changed, 27 insertions(+), 5 deletions(-) diff --git a/keydb.conf b/keydb.conf index 43ce0c8a6..8979ee47c 100644 --- a/keydb.conf +++ b/keydb.conf @@ -1815,6 +1815,12 @@ jemalloc-bg-thread yes # Tuning this parameter is a tradeoff between locking overhead and distributing the workload over multiple cores # min-clients-per-thread 50 +# Avoid forwarding RREPLAY messages to other masters? +# WARNING: This setting is dangerous! You must be certain all masters are connected to each +# other in a true mesh topology or data loss will occur! +# This command can be used to reduce multimaster bus traffic +# multi-master-no-forward no + # Path to directory for file backed scratchpad. The file backed scratchpad # reduces memory requirements by storing rarely accessed data on disk # instead of RAM. A temporary file will be created in this directory. diff --git a/src/config.cpp b/src/config.cpp index 8b318420d..75360c4b7 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -2167,6 +2167,13 @@ static int updateMaxclients(long long val, long long prev, const char **err) { return 1; } +static int validateMultiMasterNoForward(int val, const char **) { + if (val) { + serverLog(LL_WARNING, "WARNING: multi-master-no-forward is set, you *must* use a mesh topology or dataloss will occur"); + } + return 1; +} + #ifdef USE_OPENSSL static int updateTlsCfg(char *val, char *prev, const char **err) { UNUSED(val); @@ -2222,6 +2229,7 @@ standardConfig configs[] = { createBoolConfig("cluster-enabled", NULL, IMMUTABLE_CONFIG, g_pserver->cluster_enabled, 0, NULL, NULL), createBoolConfig("appendonly", NULL, MODIFIABLE_CONFIG, g_pserver->aof_enabled, 0, NULL, updateAppendonly), createBoolConfig("cluster-allow-reads-when-down", NULL, MODIFIABLE_CONFIG, g_pserver->cluster_allow_reads_when_down, 0, NULL, NULL), + createBoolConfig("multi-master-no-forward", NULL, MODIFIABLE_CONFIG, cserver.multimaster_no_forward, 0, validateMultiMasterNoForward, NULL), /* String Configs */ createStringConfig("aclfile", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, g_pserver->acl_filename, "", NULL, NULL), diff --git a/src/replication.cpp b/src/replication.cpp index b502cf0e7..c664b6012 100644 --- a/src/replication.cpp +++ b/src/replication.cpp @@ -4251,7 +4251,7 @@ void replicaReplayCommand(client *c) serverTL->current_client = current_clientSave; // call() will not propogate this for us, so we do so here - if (!s_pstate->FCancelled() && s_pstate->FFirst()) + if (!s_pstate->FCancelled() && s_pstate->FFirst() && !cserver.multimaster_no_forward) alsoPropagate(cserver.rreplayCommand,c->db->id,c->argv,c->argc,PROPAGATE_AOF|PROPAGATE_REPL); s_pstate->Pop(); diff --git a/src/server.h b/src/server.h index 3a6375b32..e6d73ab81 100644 --- a/src/server.h +++ b/src/server.h @@ -1648,6 +1648,7 @@ struct redisServerConst { unsigned char uuid[UUID_BINARY_LEN]; /* This server's UUID - populated on boot */ bool fUsePro = false; int thread_min_client_threshold = 50; + int multimaster_no_forward; }; struct redisServer { diff --git a/tests/integration/replication-multimaster.tcl b/tests/integration/replication-multimaster.tcl index 858cff26a..0e753c147 100644 --- a/tests/integration/replication-multimaster.tcl +++ b/tests/integration/replication-multimaster.tcl @@ -1,4 +1,6 @@ foreach topology {mesh ring} { + +foreach noforward [expr {[string equal $topology "mesh"] ? {no yes} : {no}}] { start_server {tags {"multi-master"} overrides {hz 500 active-replica yes multi-master yes}} { start_server {overrides {hz 500 active-replica yes multi-master yes}} { start_server {overrides {hz 500 active-replica yes multi-master yes}} { @@ -8,8 +10,12 @@ start_server {overrides {hz 500 active-replica yes multi-master yes}} { set R($j) [srv [expr 0-$j] client] set R_host($j) [srv [expr 0-$j] host] set R_port($j) [srv [expr 0-$j] port] + + $R($j) config set multi-master-no-forward $noforward } + set topology_name "$topology[expr {[string equal $noforward "yes"] ? " no-forward" : ""}]" + # Initialize as mesh if [string equal $topology "mesh"] { for {set j 0} {$j < 4} {incr j} { @@ -31,7 +37,7 @@ start_server {overrides {hz 500 active-replica yes multi-master yes}} { $R(3) replicaof $R_host(2) $R_port(2) } - test "$topology all nodes up" { + test "$topology_name all nodes up" { for {set j 0} {$j < 4} {incr j} { wait_for_condition 50 100 { [string match {*master_global_link_status:up*} [$R($j) info replication]] @@ -41,7 +47,7 @@ start_server {overrides {hz 500 active-replica yes multi-master yes}} { } } - test "$topology replicates to all nodes" { + test "$topology_name replicates to all nodes" { $R(0) set testkey foo after 500 for {set n 0} {$n < 4} {incr n} { @@ -53,7 +59,7 @@ start_server {overrides {hz 500 active-replica yes multi-master yes}} { } } - test "$topology replicates only once" { + test "$topology_name replicates only once" { $R(0) set testkey 1 after 500 #wait_for_condition 50 100 { @@ -74,7 +80,7 @@ start_server {overrides {hz 500 active-replica yes multi-master yes}} { } } - test "$topology transaction replicates only once" { + test "$topology_name transaction replicates only once" { for {set j 0} {$j < 1000} {incr j} { $R(0) set testkey 1 $R(0) multi @@ -95,3 +101,4 @@ start_server {overrides {hz 500 active-replica yes multi-master yes}} { } } } +} From 70824b3bdb03f73ceb25dbe236bc62eccfbbe5d5 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 12 Jul 2020 21:42:11 +0000 Subject: [PATCH 28/29] Add the KEYDB.MEXISTS command, see issue #203 Former-commit-id: 5619f515285b08d9c443425de1f3092ae3058d40 --- src/db.cpp | 7 +++++++ src/help.h | 7 ++++++- src/server.cpp | 4 ++++ src/server.h | 1 + 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/db.cpp b/src/db.cpp index 770a1c9b4..11816262b 100644 --- a/src/db.cpp +++ b/src/db.cpp @@ -683,6 +683,13 @@ void existsCommand(client *c) { addReplyLongLong(c,count); } +void mexistsCommand(client *c) { + addReplyArrayLen(c, c->argc - 1); + for (int j = 1; j < c->argc; ++j) { + addReplyBool(c, lookupKeyRead(c->db, c->argv[j])); + } +} + void selectCommand(client *c) { long id; diff --git a/src/help.h b/src/help.h index 2f692dfc2..c6b614475 100644 --- a/src/help.h +++ b/src/help.h @@ -1325,7 +1325,12 @@ struct commandHelp { "Rename a hash key, copying the value.", 4, "6.5.3" - } + }, + { "KEYDB.MEXISTS", + "key [key ...]", + "Determine if a key exists", + 0, + "6.5.12" }, }; #endif diff --git a/src/server.cpp b/src/server.cpp index c197df741..c38ca1efa 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -247,6 +247,10 @@ struct redisCommand redisCommandTable[] = { "read-only fast @keyspace", 0,NULL,1,-1,1,0,0,0}, + {"keydb.mexists",mexistsCommand,-2, + "read-only fast @keyspace", + 0,NULL,1,-1,1,0,0,0}, + {"setbit",setbitCommand,4, "write use-memory @bitmap", 0,NULL,1,1,1,0,0,0}, diff --git a/src/server.h b/src/server.h index e6d73ab81..7479b4557 100644 --- a/src/server.h +++ b/src/server.h @@ -2822,6 +2822,7 @@ void getCommand(client *c); void delCommand(client *c); void unlinkCommand(client *c); void existsCommand(client *c); +void mexistsCommand(client *c); void setbitCommand(client *c); void getbitCommand(client *c); void bitfieldCommand(client *c); From 6c48ba994e2cfab79d7bde59e3c2eab0ef631d33 Mon Sep 17 00:00:00 2001 From: benschermel Date: Sun, 12 Jul 2020 18:03:21 -0400 Subject: [PATCH 29/29] update rpm permissions Former-commit-id: 201ffa9d6228c42f3c55e058fe99a72f06a7a1b7 --- pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb-sentinel.conf | 0 pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb.conf | 0 pkg/rpm/keydb_build/keydb_rpm/etc/logrotate.d/keydb | 0 .../etc/systemd/system/keydb-sentinel.service.d/limit.conf | 0 .../keydb_rpm/etc/systemd/system/keydb.service.d/limit.conf | 0 .../keydb_rpm/usr/lib/systemd/system/keydb-sentinel.service | 0 .../keydb_build/keydb_rpm/usr/lib/systemd/system/keydb.service | 0 7 files changed, 0 insertions(+), 0 deletions(-) mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb-sentinel.conf mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb.conf mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/etc/logrotate.d/keydb mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/etc/systemd/system/keydb-sentinel.service.d/limit.conf mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/etc/systemd/system/keydb.service.d/limit.conf mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/usr/lib/systemd/system/keydb-sentinel.service mode change 100755 => 100644 pkg/rpm/keydb_build/keydb_rpm/usr/lib/systemd/system/keydb.service diff --git a/pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb-sentinel.conf b/pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb-sentinel.conf old mode 100755 new mode 100644 diff --git a/pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb.conf b/pkg/rpm/keydb_build/keydb_rpm/etc/keydb/keydb.conf old mode 100755 new mode 100644 diff --git a/pkg/rpm/keydb_build/keydb_rpm/etc/logrotate.d/keydb b/pkg/rpm/keydb_build/keydb_rpm/etc/logrotate.d/keydb old mode 100755 new mode 100644 diff --git a/pkg/rpm/keydb_build/keydb_rpm/etc/systemd/system/keydb-sentinel.service.d/limit.conf b/pkg/rpm/keydb_build/keydb_rpm/etc/systemd/system/keydb-sentinel.service.d/limit.conf old mode 100755 new mode 100644 diff --git a/pkg/rpm/keydb_build/keydb_rpm/etc/systemd/system/keydb.service.d/limit.conf b/pkg/rpm/keydb_build/keydb_rpm/etc/systemd/system/keydb.service.d/limit.conf old mode 100755 new mode 100644 diff --git a/pkg/rpm/keydb_build/keydb_rpm/usr/lib/systemd/system/keydb-sentinel.service b/pkg/rpm/keydb_build/keydb_rpm/usr/lib/systemd/system/keydb-sentinel.service old mode 100755 new mode 100644 diff --git a/pkg/rpm/keydb_build/keydb_rpm/usr/lib/systemd/system/keydb.service b/pkg/rpm/keydb_build/keydb_rpm/usr/lib/systemd/system/keydb.service old mode 100755 new mode 100644