From 7993780dda22df01cebba42d16f805213d66e194 Mon Sep 17 00:00:00 2001 From: sundb Date: Tue, 15 Dec 2020 15:30:24 +0800 Subject: Fix some wrong server.dirty increments (#8140) Fix wrong server dirty increment in * spopWithCountCommand * hsetCommand * ltrimCommand * pfaddCommand Some didn't increment the amount of fields (just one per command). Others had excessive increments. --- src/db.c | 3 +++ src/hyperloglog.c | 2 +- src/t_hash.c | 2 +- src/t_list.c | 2 +- src/t_set.c | 4 +--- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/db.c b/src/db.c index 5045935c3..4f27534fc 100644 --- a/src/db.c +++ b/src/db.c @@ -616,6 +616,9 @@ void flushAllDataAndResetRDB(int flags) { rdbSave(server.rdb_filename,rsiptr); server.dirty = saved_dirty; } + + /* Without that extra dirty++, when db was already empty, FLUSHALL will + * not be replicated nor put into the AOF. */ server.dirty++; #if defined(USE_JEMALLOC) /* jemalloc 5 doesn't release pages back to the OS when there's no traffic. diff --git a/src/hyperloglog.c b/src/hyperloglog.c index d018e975e..648c26a02 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -1211,7 +1211,7 @@ void pfaddCommand(client *c) { if (updated) { signalModifiedKey(c,c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_STRING,"pfadd",c->argv[1],c->db->id); - server.dirty++; + server.dirty += updated; HLL_INVALIDATE_CACHE(hdr); } addReply(c, updated ? shared.cone : shared.czero); diff --git a/src/t_hash.c b/src/t_hash.c index ff9ac742e..51c7d6758 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -644,7 +644,7 @@ void hsetCommand(client *c) { } signalModifiedKey(c,c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_HASH,"hset",c->argv[1],c->db->id); - server.dirty++; + server.dirty += (c->argc - 2)/2; } void hincrbyCommand(client *c) { diff --git a/src/t_list.c b/src/t_list.c index 42b4f92df..106f960f6 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -508,7 +508,7 @@ void ltrimCommand(client *c) { notifyKeyspaceEvent(NOTIFY_GENERIC,"del",c->argv[1],c->db->id); } signalModifiedKey(c,c->db,c->argv[1]); - server.dirty++; + server.dirty += (ltrim + rtrim); addReply(c,shared.ok); } diff --git a/src/t_set.c b/src/t_set.c index 7c71dfc2f..fd9f4442a 100644 --- a/src/t_set.c +++ b/src/t_set.c @@ -476,7 +476,7 @@ void spopWithCountCommand(client *c) { /* Generate an SPOP keyspace notification */ notifyKeyspaceEvent(NOTIFY_SET,"spop",c->argv[1],c->db->id); - server.dirty += count; + server.dirty += (count >= size) ? size : count; /* CASE 1: * The number of requested elements is greater than or equal to @@ -492,7 +492,6 @@ void spopWithCountCommand(client *c) { /* Propagate this command as a DEL operation */ rewriteClientCommandVector(c,2,shared.del,c->argv[1]); signalModifiedKey(c,c->db,c->argv[1]); - server.dirty++; return; } @@ -594,7 +593,6 @@ void spopWithCountCommand(client *c) { decrRefCount(propargv[0]); preventCommandPropagation(c); signalModifiedKey(c,c->db,c->argv[1]); - server.dirty++; } void spopCommand(client *c) { -- cgit v1.2.1 From 1f42bd70572c8e85fa431a66952c7b79eb182a87 Mon Sep 17 00:00:00 2001 From: filipe oliveira Date: Tue, 15 Dec 2020 20:03:05 +0000 Subject: Included in redis.conf explicit explanation of tls-protocol defaults (#8193) --- redis.conf | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/redis.conf b/redis.conf index 849f171bc..af4b4be1f 100644 --- a/redis.conf +++ b/redis.conf @@ -196,9 +196,12 @@ tcp-keepalive 300 # # tls-cluster yes -# Explicitly specify TLS versions to support. Allowed values are case insensitive -# and include "TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3" (OpenSSL >= 1.1.1) or -# any combination. To enable only TLSv1.2 and TLSv1.3, use: +# By default, only TLSv1.2 and TLSv1.3 are enabled and it is highly recommended +# that older formally deprecated versions are kept disabled to reduce the attack surface. +# You can explicitly specify TLS versions to support. +# Allowed values are case insensitive and include "TLSv1", "TLSv1.1", "TLSv1.2", +# "TLSv1.3" (OpenSSL >= 1.1.1) or any combination. +# To enable only TLSv1.2 and TLSv1.3, use: # # tls-protocols "TLSv1.2 TLSv1.3" -- cgit v1.2.1 From 4f67d0b647caba9fa301a1a604214c0ecc40940a Mon Sep 17 00:00:00 2001 From: Wen Hui Date: Wed, 16 Dec 2020 16:19:12 -0500 Subject: fix wrong comment in cluster.h (#8191) --- src/cluster.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cluster.h b/src/cluster.h index 7e5f79c87..d58f350ce 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -40,7 +40,7 @@ typedef struct clusterLink { sds sndbuf; /* Packet send buffer */ char *rcvbuf; /* Packet reception buffer */ size_t rcvbuf_len; /* Used size of rcvbuf */ - size_t rcvbuf_alloc; /* Used size of rcvbuf */ + size_t rcvbuf_alloc; /* Allocated size of rcvbuf */ struct clusterNode *node; /* Node related to this link if any, or NULL */ } clusterLink; -- cgit v1.2.1 From 6413e5f81a8b6296f4bb57cae11f5b2534f5f663 Mon Sep 17 00:00:00 2001 From: Wang Yuan Date: Thu, 17 Dec 2020 16:22:13 +0800 Subject: [Redis-benchmark] Use IP from CLUSTER NODE reply for first node too (#8154) If we only has one node in cluster or before 8fdc857, we don't know myself ip, so we should use config.hostip for myself. However, we should use the IP from the command response to update node->ip if it exists and is different from config.hostip otherwise, when there's more than one node in cluster, if we use -h with virtual IP or DNS, benchmark doesn't show node real ip and port of myself even though it could get right IP and port by CLUSTER NODES command. --- src/redis-benchmark.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/redis-benchmark.c b/src/redis-benchmark.c index 4efed4b12..18e19b0e0 100644 --- a/src/redis-benchmark.c +++ b/src/redis-benchmark.c @@ -1182,8 +1182,8 @@ static int fetchClusterConfiguration() { } if (myself) { node = firstNode; - if (node->ip == NULL && ip != NULL) { - node->ip = ip; + if (ip != NULL && strcmp(node->ip, ip) != 0) { + node->ip = sdsnew(ip); node->port = port; } } else { -- cgit v1.2.1 From 407da77ea4f6de622d6f242ad16580aa6f4e7b85 Mon Sep 17 00:00:00 2001 From: sundb Date: Thu, 17 Dec 2020 17:02:17 +0800 Subject: Fix comment of georadiusGeneric function (#8202) --- src/geo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/geo.c b/src/geo.c index 2fd1cb21b..925c090ed 100644 --- a/src/geo.c +++ b/src/geo.c @@ -488,9 +488,9 @@ void geoaddCommand(client *c) { /* GEORADIUS key x y radius unit [WITHDIST] [WITHHASH] [WITHCOORD] [ASC|DESC] * [COUNT count] [STORE key] [STOREDIST key] * GEORADIUSBYMEMBER key member radius unit ... options ... - * GEOSEARCH key [FROMMEMBER member] [FORMLOG long lat] [BYRADIUS radius unit] + * GEOSEARCH key [FROMMEMBER member] [FROMLONLAT long lat] [BYRADIUS radius unit] * [BYBOX width height unit] [WITHCORD] [WITHDIST] [WITHASH] [COUNT count] [ASC|DESC] - * GEOSEARCHSTORE dest_key src_key [FROMMEMBER member] [FORMLOG long lat] [BYRADIUS radius unit] + * GEOSEARCHSTORE dest_key src_key [FROMMEMBER member] [FROMLONLAT long lat] [BYRADIUS radius unit] * [BYBOX width height unit] [WITHCORD] [WITHDIST] [WITHASH] [COUNT count] [ASC|DESC] [STOREDIST] * */ void georadiusGeneric(client *c, int srcKeyIndex, int flags) { -- cgit v1.2.1 From a56cbd30366ac7776bcd71e2ac8d9c421ca54cc8 Mon Sep 17 00:00:00 2001 From: Hanif Ariffin Date: Thu, 17 Dec 2020 03:00:48 -0800 Subject: More fixes to printf format specifier. (#7909) mostly signed / unsigned mismatches. Signed-off-by: Hanif Bin Ariffin Co-authored-by: Oran Agra --- src/dict.c | 8 ++++---- src/rax.c | 2 +- src/redis-cli.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/dict.c b/src/dict.c index 4736dacd5..2c59dd466 100644 --- a/src/dict.c +++ b/src/dict.c @@ -1135,10 +1135,10 @@ size_t _dictGetStatsHt(char *buf, size_t bufsize, dictht *ht, int tableid) { /* Generate human readable stats. */ l += snprintf(buf+l,bufsize-l, "Hash table %d stats (%s):\n" - " table size: %ld\n" - " number of elements: %ld\n" - " different slots: %ld\n" - " max chain length: %ld\n" + " table size: %lu\n" + " number of elements: %lu\n" + " different slots: %lu\n" + " max chain length: %lu\n" " avg chain length (counted): %.02f\n" " avg chain length (computed): %.02f\n" " Chain length distribution:\n", diff --git a/src/rax.c b/src/rax.c index 89345fe26..1ffb42287 100644 --- a/src/rax.c +++ b/src/rax.c @@ -1892,7 +1892,7 @@ void raxShow(rax *rax) { /* Used by debugnode() macro to show info about a given node. */ void raxDebugShowNode(const char *msg, raxNode *n) { if (raxDebugMsg == 0) return; - printf("%s: %p [%.*s] key:%d size:%d children:", + printf("%s: %p [%.*s] key:%u size:%u children:", msg, (void*)n, (int)n->size, (char*)n->data, n->iskey, n->size); int numcld = n->iscompr ? 1 : n->size; raxNode **cldptr = raxNodeLastChildPtr(n) - (numcld-1); diff --git a/src/redis-cli.c b/src/redis-cli.c index 4f5b0fe03..5310a1b21 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -5578,7 +5578,7 @@ static int clusterManagerCommandCreate(int argc, char **argv) { if (last > CLUSTER_MANAGER_SLOTS || i == (masters_count - 1)) last = CLUSTER_MANAGER_SLOTS - 1; if (last < first) last = first; - printf("Master[%d] -> Slots %lu - %lu\n", i, first, last); + printf("Master[%d] -> Slots %ld - %ld\n", i, first, last); master->slots_count = 0; for (j = first; j <= last; j++) { master->slots[j] = 1; -- cgit v1.2.1 From 0f3e0cb4adf6bdbb7dbd70679d129ad5a528df9d Mon Sep 17 00:00:00 2001 From: Nick Revin Date: Thu, 17 Dec 2020 16:49:19 +0400 Subject: install redis-check-rdb and redis-check-aof as symlinks to redis-server (#5745) --- src/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Makefile b/src/Makefile index 0329da8c9..32cedbac5 100644 --- a/src/Makefile +++ b/src/Makefile @@ -405,8 +405,8 @@ install: all $(REDIS_INSTALL) $(REDIS_SERVER_NAME) $(INSTALL_BIN) $(REDIS_INSTALL) $(REDIS_BENCHMARK_NAME) $(INSTALL_BIN) $(REDIS_INSTALL) $(REDIS_CLI_NAME) $(INSTALL_BIN) - $(REDIS_INSTALL) $(REDIS_CHECK_RDB_NAME) $(INSTALL_BIN) - $(REDIS_INSTALL) $(REDIS_CHECK_AOF_NAME) $(INSTALL_BIN) + @ln -sf $(REDIS_SERVER_NAME) $(INSTALL_BIN)/$(REDIS_CHECK_RDB_NAME) + @ln -sf $(REDIS_SERVER_NAME) $(INSTALL_BIN)/$(REDIS_CHECK_AOF_NAME) @ln -sf $(REDIS_SERVER_NAME) $(INSTALL_BIN)/$(REDIS_SENTINEL_NAME) uninstall: -- cgit v1.2.1 From f48afb4710e6e102b043add55a0649b22ef4a4ab Mon Sep 17 00:00:00 2001 From: Qu Chen Date: Thu, 17 Dec 2020 09:26:33 -0800 Subject: Handle binary safe string for REQUIREPASS and MASTERAUTH directives (#8200) * Handle binary safe string for REQUIREPASS and MASTERAUTH directives. --- src/config.c | 102 +++++++++++++++++++++++++++++++++++++++++++--------- src/replication.c | 30 ++++++++++------ src/server.h | 2 +- tests/unit/auth.tcl | 41 +++++++++++++++++++++ 4 files changed, 148 insertions(+), 27 deletions(-) diff --git a/src/config.c b/src/config.c index c858df3f3..4617e8678 100644 --- a/src/config.c +++ b/src/config.c @@ -166,6 +166,15 @@ typedef struct stringConfigData { be stored as a NULL value. */ } stringConfigData; +typedef struct sdsConfigData { + sds *config; /* Pointer to the server config this value is stored in. */ + const char *default_value; /* Default value of the config on rewrite. */ + int (*is_valid_fn)(sds val, char **err); /* Optional function to check validity of new value (generic doc above) */ + int (*update_fn)(sds val, sds prev, char **err); /* Optional function to apply new value at runtime (generic doc above) */ + int convert_empty_to_null; /* Boolean indicating if empty SDS strings should + be stored as a NULL value. */ +} sdsConfigData; + typedef struct enumConfigData { int *config; /* The pointer to the server config this value is stored in */ configEnum *enum_value; /* The underlying enum type this data represents */ @@ -212,6 +221,7 @@ typedef struct numericConfigData { typedef union typeData { boolConfigData yesno; stringConfigData string; + sdsConfigData sds; enumConfigData enumd; numericConfigData numeric; } typeData; @@ -512,7 +522,7 @@ void loadServerConfigFromString(char *config) { } server.repl_state = REPL_STATE_CONNECT; } else if (!strcasecmp(argv[0],"requirepass") && argc == 2) { - if (strlen(argv[1]) > CONFIG_AUTHPASS_MAX_LEN) { + if (sdslen(argv[1]) > CONFIG_AUTHPASS_MAX_LEN) { err = "Password is longer than CONFIG_AUTHPASS_MAX_LEN"; goto loaderr; } @@ -524,10 +534,10 @@ void loadServerConfigFromString(char *config) { sdsfree(server.requirepass); server.requirepass = NULL; if (sdslen(argv[1])) { - sds aclop = sdscatprintf(sdsempty(),">%s",argv[1]); + sds aclop = sdscatlen(sdsnew(">"), argv[1], sdslen(argv[1])); ACLSetUser(DefaultUser,aclop,sdslen(aclop)); sdsfree(aclop); - server.requirepass = sdsnew(argv[1]); + server.requirepass = sdsdup(argv[1]); } else { ACLSetUser(DefaultUser,"nopass",-1); } @@ -751,10 +761,10 @@ void configSetCommand(client *c) { sdsfree(server.requirepass); server.requirepass = NULL; if (sdslen(o->ptr)) { - sds aclop = sdscatprintf(sdsempty(),">%s",(char*)o->ptr); + sds aclop = sdscatlen(sdsnew(">"), o->ptr, sdslen(o->ptr)); ACLSetUser(DefaultUser,aclop,sdslen(aclop)); sdsfree(aclop); - server.requirepass = sdsnew(o->ptr); + server.requirepass = sdsdup(o->ptr); } else { ACLSetUser(DefaultUser,"nopass",-1); } @@ -1330,6 +1340,28 @@ void rewriteConfigStringOption(struct rewriteConfigState *state, const char *opt rewriteConfigRewriteLine(state,option,line,force); } +/* Rewrite a SDS string option. */ +void rewriteConfigSdsOption(struct rewriteConfigState *state, const char *option, sds value, const sds defvalue) { + int force = 1; + sds line; + + /* If there is no value set, we don't want the SDS option + * to be present in the configuration at all. */ + if (value == NULL) { + rewriteConfigMarkAsProcessed(state, option); + return; + } + + /* Set force to zero if the value is set to its default. */ + if (defvalue && sdscmp(value, defvalue) == 0) force = 0; + + line = sdsnew(option); + line = sdscatlen(line, " ", 1); + line = sdscatrepr(line, value, sdslen(value)); + + rewriteConfigRewriteLine(state, option, line, force); +} + /* Rewrite a numerical (long long range) option. */ void rewriteConfigNumericalOption(struct rewriteConfigState *state, const char *option, long long value, long long defvalue) { int force = value != defvalue; @@ -1802,22 +1834,14 @@ static void boolConfigRewrite(typeData data, const char *name, struct rewriteCon /* String Configs */ static void stringConfigInit(typeData data) { - if (data.string.convert_empty_to_null) { - *data.string.config = data.string.default_value ? zstrdup(data.string.default_value) : NULL; - } else { - *data.string.config = zstrdup(data.string.default_value); - } + *data.string.config = (data.string.convert_empty_to_null && !data.string.default_value) ? NULL : zstrdup(data.string.default_value); } static int stringConfigSet(typeData data, sds value, int update, char **err) { if (data.string.is_valid_fn && !data.string.is_valid_fn(value, err)) return 0; char *prev = *data.string.config; - if (data.string.convert_empty_to_null) { - *data.string.config = value[0] ? zstrdup(value) : NULL; - } else { - *data.string.config = zstrdup(value); - } + *data.string.config = (data.string.convert_empty_to_null && !value[0]) ? NULL : zstrdup(value); if (update && data.string.update_fn && !data.string.update_fn(*data.string.config, prev, err)) { zfree(*data.string.config); *data.string.config = prev; @@ -1835,6 +1859,38 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC rewriteConfigStringOption(state, name,*(data.string.config), data.string.default_value); } +/* SDS Configs */ +static void sdsConfigInit(typeData data) { + *data.sds.config = (data.sds.convert_empty_to_null && !data.sds.default_value) ? NULL: sdsnew(data.sds.default_value); +} + +static int sdsConfigSet(typeData data, sds value, int update, char **err) { + if (data.sds.is_valid_fn && !data.sds.is_valid_fn(value, err)) + return 0; + sds prev = *data.sds.config; + *data.sds.config = (data.sds.convert_empty_to_null && (sdslen(value) == 0)) ? NULL : sdsdup(value); + if (update && data.sds.update_fn && !data.sds.update_fn(*data.sds.config, prev, err)) { + sdsfree(*data.sds.config); + *data.sds.config = prev; + return 0; + } + sdsfree(prev); + return 1; +} + +static void sdsConfigGet(client *c, typeData data) { + if (*data.sds.config) { + addReplyBulkSds(c, sdsdup(*data.sds.config)); + } else { + addReplyBulkCString(c, ""); + } +} + +static void sdsConfigRewrite(typeData data, const char *name, struct rewriteConfigState *state) { + rewriteConfigSdsOption(state, name, *(data.sds.config), data.sds.default_value ? sdsnew(data.sds.default_value) : NULL); +} + + #define ALLOW_EMPTY_STRING 0 #define EMPTY_STRING_IS_NULL 1 @@ -1850,6 +1906,18 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC } \ } +#define createSDSConfig(name, alias, modifiable, empty_to_null, config_addr, default, is_valid, update) { \ + embedCommonConfig(name, alias, modifiable) \ + embedConfigInterface(sdsConfigInit, sdsConfigSet, sdsConfigGet, sdsConfigRewrite) \ + .data.sds = { \ + .config = &(config_addr), \ + .default_value = (default), \ + .is_valid_fn = (is_valid), \ + .update_fn = (update), \ + .convert_empty_to_null = (empty_to_null), \ + } \ +} + /* Enum configs */ static void enumConfigInit(typeData data) { *data.enumd.config = data.enumd.default_value; @@ -2349,7 +2417,6 @@ standardConfig configs[] = { createStringConfig("pidfile", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.pidfile, NULL, NULL, NULL), createStringConfig("replica-announce-ip", "slave-announce-ip", MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.slave_announce_ip, NULL, NULL, NULL), createStringConfig("masteruser", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masteruser, NULL, NULL, NULL), - createStringConfig("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL), createStringConfig("cluster-announce-ip", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_ip, NULL, NULL, NULL), createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "redis", NULL, NULL), createStringConfig("dbfilename", NULL, MODIFIABLE_CONFIG, ALLOW_EMPTY_STRING, server.rdb_filename, "dump.rdb", isValidDBfilename, NULL), @@ -2359,6 +2426,9 @@ standardConfig configs[] = { createStringConfig("aof_rewrite_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.aof_rewrite_cpulist, NULL, NULL, NULL), createStringConfig("bgsave_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.bgsave_cpulist, NULL, NULL, NULL), + /* SDS Configs */ + createSDSConfig("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL), + /* Enum Configs */ createEnumConfig("supervised", NULL, IMMUTABLE_CONFIG, supervised_mode_enum, server.supervised_mode, SUPERVISED_NONE, NULL, NULL), createEnumConfig("syslog-facility", NULL, IMMUTABLE_CONFIG, syslog_facility_enum, server.syslog_facility, LOG_LOCAL0, NULL, NULL), diff --git a/src/replication.c b/src/replication.c index 64aa41390..34ff44087 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1835,6 +1835,7 @@ error: */ #define SYNC_CMD_READ (1<<0) #define SYNC_CMD_WRITE (1<<1) +#define SYNC_CMD_WRITE_SDS (1<<2) #define SYNC_CMD_FULL (SYNC_CMD_READ|SYNC_CMD_WRITE) char *sendSynchronousCommand(int flags, connection *conn, ...) { @@ -1852,8 +1853,13 @@ char *sendSynchronousCommand(int flags, connection *conn, ...) { while(1) { arg = va_arg(ap, char*); if (arg == NULL) break; - - cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg); + if (flags & SYNC_CMD_WRITE_SDS) { + cmdargs = sdscatprintf(cmdargs,"$%zu\r\n", sdslen((sds)arg)); + cmdargs = sdscatsds(cmdargs, (sds)arg); + cmdargs = sdscat(cmdargs, "\r\n"); + } else { + cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg); + } argslen++; } @@ -2166,14 +2172,18 @@ void syncWithMaster(connection *conn) { /* AUTH with the master if required. */ if (server.repl_state == REPL_STATE_SEND_AUTH) { - if (server.masteruser && server.masterauth) { - err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH", - server.masteruser,server.masterauth,NULL); - if (err) goto write_error; - server.repl_state = REPL_STATE_RECEIVE_AUTH; - return; - } else if (server.masterauth) { - err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH",server.masterauth,NULL); + if (server.masterauth) { + sds auth = sdsnew("AUTH"); + if (server.masteruser) { + sds masteruser = sdsnew(server.masteruser); + err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth, + masteruser, server.masterauth, NULL); + sdsfree(masteruser); + } else { + err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth, + server.masterauth, NULL); + } + sdsfree(auth); if (err) goto write_error; server.repl_state = REPL_STATE_RECEIVE_AUTH; return; diff --git a/src/server.h b/src/server.h index 427d7c2f2..5c75aed08 100644 --- a/src/server.h +++ b/src/server.h @@ -1386,7 +1386,7 @@ struct redisServer { int repl_diskless_sync_delay; /* Delay to start a diskless repl BGSAVE. */ /* Replication (slave) */ char *masteruser; /* AUTH with this user and masterauth with master */ - char *masterauth; /* AUTH with this password with master */ + sds masterauth; /* AUTH with this password with master */ char *masterhost; /* Hostname of master */ int masterport; /* Port of master */ int repl_timeout; /* Timeout after N seconds of master idle */ diff --git a/tests/unit/auth.tcl b/tests/unit/auth.tcl index 9080d4bf7..b63cf0126 100644 --- a/tests/unit/auth.tcl +++ b/tests/unit/auth.tcl @@ -25,3 +25,44 @@ start_server {tags {"auth"} overrides {requirepass foobar}} { r incr foo } {101} } + +start_server {tags {"auth_binary_password"}} { + test {AUTH fails when binary password is wrong} { + r config set requirepass "abc\x00def" + catch {r auth abc} err + set _ $err + } {WRONGPASS*} + + test {AUTH succeeds when binary password is correct} { + r config set requirepass "abc\x00def" + r auth "abc\x00def" + } {OK} + + start_server {tags {"masterauth"}} { + set master [srv -1 client] + set master_host [srv -1 host] + set master_port [srv -1 port] + set slave [srv 0 client] + + test {MASTERAUTH test with binary password} { + $master config set requirepass "abc\x00def" + + # Configure the replica with masterauth + set loglines [count_log_lines 0] + $slave slaveof $master_host $master_port + $slave config set masterauth "abc" + + # Verify replica is not able to sync with master + wait_for_log_messages 0 {"*Unable to AUTH to MASTER*"} $loglines 1000 10 + assert_equal {down} [s 0 master_link_status] + + # Test replica with the correct masterauth + $slave config set masterauth "abc\x00def" + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Can't turn the instance into a replica" + } + } + } +} -- cgit v1.2.1 From 11b3325e9999721d35ec64afac7b917664f6291b Mon Sep 17 00:00:00 2001 From: Qu Chen Date: Thu, 17 Dec 2020 11:58:58 -0800 Subject: Not over-allocate client query buffer when reading large objects. (#5954) In response to large client query buffer optimization introduced in 1898e6c. The calculation of the amount of remaining bytes we need to write to the query buffer was calculated wrong, as a result we are unnecessarily growing the client query buffer by sdslen(c->querybuf) always. This fix corrects that behavior. Please note the previous behavior prior to the before-mentioned change was correctly calculating the remaining additional bytes, and this change makes that calculate to be consistent. Useful context, the argument of size `ll` starts at qb_pos (which is now the beginning of the sds), but much of it may have already been read from the socket, so we only need to grow the sds for the remainder of it. --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index df30206a3..b81060cbb 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1851,7 +1851,7 @@ int processMultibulkBuffer(client *c) { c->qb_pos = 0; /* Hint the sds library about the amount of bytes this string is * going to contain. */ - c->querybuf = sdsMakeRoomFor(c->querybuf,ll+2); + c->querybuf = sdsMakeRoomFor(c->querybuf,ll+2-sdslen(c->querybuf)); } } c->bulklen = ll; -- cgit v1.2.1 From 51eb0da824d2dae45bf5966e83f2089fba8d9c24 Mon Sep 17 00:00:00 2001 From: sundb Date: Fri, 18 Dec 2020 20:55:39 +0800 Subject: Fix command reset's arity (#8212) --- src/server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.c b/src/server.c index ac43c32e7..de7e2736d 100644 --- a/src/server.c +++ b/src/server.c @@ -1059,7 +1059,7 @@ struct redisCommand redisCommandTable[] = { "read-only @string", 0,lcsGetKeys,0,0,0,0,0,0}, - {"reset",resetCommand,-1, + {"reset",resetCommand,1, "no-script ok-stale ok-loading fast @connection", 0,NULL,0,0,0,0,0,0} }; -- cgit v1.2.1 From 4c13945c370b98b69886c6bcd1e71d4934344cae Mon Sep 17 00:00:00 2001 From: valentinogeron Date: Mon, 21 Dec 2020 15:40:20 +0200 Subject: Fix PFDEBUG commands flag (#8222) - Mark it as a @hyperloglog command (ACL) - Should not be allowed in OOM - Add firstkey, lastkey, step - Add comment that explains the 'write' flag --- src/server.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index de7e2736d..279e75f2a 100644 --- a/src/server.c +++ b/src/server.c @@ -975,9 +975,12 @@ struct redisCommand redisCommandTable[] = { "write use-memory @hyperloglog", 0,NULL,1,-1,1,0,0,0}, + /* Unlike PFCOUNT that is considered as a read-only command (although + * it changes a bit), PFDEBUG may change the entire key when converting + * from sparse to dense representation */ {"pfdebug",pfdebugCommand,-3, - "admin write", - 0,NULL,0,0,0,0,0,0}, + "admin write use-memory @hyperloglog", + 0,NULL,2,2,1,0,0,0}, {"xadd",xaddCommand,-5, "write use-memory fast random @stream", -- cgit v1.2.1 From 4bc14da2b3330c7ce8b9366ea3880eba65ba2ff9 Mon Sep 17 00:00:00 2001 From: sundb Date: Tue, 22 Dec 2020 14:57:45 +0800 Subject: Fix some redundancy use of semicolon in do-while macros (#8221) * Fix some redundancy use of semicolon in do-while macros --- deps/hiredis/async_private.h | 2 +- src/config.c | 7 +++---- src/debugmacro.h | 2 +- src/dict.c | 2 +- src/modules/testmodule.c | 2 +- src/quicklist.c | 2 +- src/server.h | 4 ++-- src/testhelp.h | 4 ++-- 8 files changed, 12 insertions(+), 13 deletions(-) diff --git a/deps/hiredis/async_private.h b/deps/hiredis/async_private.h index b9d23fffd..ea0558d42 100644 --- a/deps/hiredis/async_private.h +++ b/deps/hiredis/async_private.h @@ -51,7 +51,7 @@ #define _EL_CLEANUP(ctx) do { \ if ((ctx)->ev.cleanup) (ctx)->ev.cleanup((ctx)->ev.data); \ ctx->ev.cleanup = NULL; \ - } while(0); + } while(0) static inline void refreshTimeout(redisAsyncContext *ctx) { #define REDIS_TIMER_ISSET(tvp) \ diff --git a/src/config.c b/src/config.c index 4617e8678..a92b41624 100644 --- a/src/config.c +++ b/src/config.c @@ -915,7 +915,7 @@ badfmt: /* Bad format errors */ addReplyBulkCString(c,_var ? _var : ""); \ matches++; \ } \ -} while(0); +} while(0) #define config_get_bool_field(_name,_var) do { \ if (stringmatch(pattern,_name,1)) { \ @@ -923,7 +923,7 @@ badfmt: /* Bad format errors */ addReplyBulkCString(c,_var ? "yes" : "no"); \ matches++; \ } \ -} while(0); +} while(0) #define config_get_numerical_field(_name,_var) do { \ if (stringmatch(pattern,_name,1)) { \ @@ -932,8 +932,7 @@ badfmt: /* Bad format errors */ addReplyBulkCString(c,buf); \ matches++; \ } \ -} while(0); - +} while(0) void configGetCommand(client *c) { robj *o = c->argv[2]; diff --git a/src/debugmacro.h b/src/debugmacro.h index ded2d2667..58e6577e5 100644 --- a/src/debugmacro.h +++ b/src/debugmacro.h @@ -38,4 +38,4 @@ fprintf(fp,__VA_ARGS__); \ fprintf(fp,"\n"); \ fclose(fp); \ - } while (0); + } while (0) diff --git a/src/dict.c b/src/dict.c index 2c59dd466..4f0916a27 100644 --- a/src/dict.c +++ b/src/dict.c @@ -1215,7 +1215,7 @@ dictType BenchmarkDictType = { #define end_benchmark(msg) do { \ elapsed = timeInMilliseconds()-start; \ printf(msg ": %ld items in %lld ms\n", count, elapsed); \ -} while(0); +} while(0) /* dict-benchmark [count] */ int main(int argc, char **argv) { diff --git a/src/modules/testmodule.c b/src/modules/testmodule.c index 5634530dd..078c02c5c 100644 --- a/src/modules/testmodule.c +++ b/src/modules/testmodule.c @@ -364,7 +364,7 @@ int TestAssertIntegerReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, lon do { \ RedisModule_Log(ctx,"warning","Testing %s", name); \ reply = RedisModule_Call(ctx,name,__VA_ARGS__); \ - } while (0); + } while (0) /* TEST.IT -- Run all the tests. */ int TestIt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { diff --git a/src/quicklist.c b/src/quicklist.c index 5ed9b6a5d..4dc3576ae 100644 --- a/src/quicklist.c +++ b/src/quicklist.c @@ -69,7 +69,7 @@ static const size_t optimization_level[] = {4096, 8192, 16384, 32768, 65536}; printf("%s:%s:%d:\t", __FILE__, __FUNCTION__, __LINE__); \ printf(__VA_ARGS__); \ printf("\n"); \ - } while (0); + } while (0) #endif /* Bookmarks forward declarations */ diff --git a/src/server.h b/src/server.h index 5c75aed08..8a1f191f3 100644 --- a/src/server.h +++ b/src/server.h @@ -605,7 +605,7 @@ typedef struct RedisModuleIO { iovar.ver = 0; \ iovar.key = keyptr; \ iovar.ctx = NULL; \ -} while(0); +} while(0) /* This is a structure used to export DEBUG DIGEST capabilities to Redis * modules. We want to capture both the ordered and unordered elements of @@ -621,7 +621,7 @@ typedef struct RedisModuleDigest { #define moduleInitDigestContext(mdvar) do { \ memset(mdvar.o,0,sizeof(mdvar.o)); \ memset(mdvar.x,0,sizeof(mdvar.x)); \ -} while(0); +} while(0) /* Objects encoding. Some kind of objects like Strings and Hashes can be * internally represented in multiple ways. The 'encoding' field of the object diff --git a/src/testhelp.h b/src/testhelp.h index 450334046..c6c1b55bf 100644 --- a/src/testhelp.h +++ b/src/testhelp.h @@ -44,7 +44,7 @@ int __test_num = 0; #define test_cond(descr,_c) do { \ __test_num++; printf("%d - %s: ", __test_num, descr); \ if(_c) printf("PASSED\n"); else {printf("FAILED\n"); __failed_tests++;} \ -} while(0); +} while(0) #define test_report() do { \ printf("%d tests, %d passed, %d failed\n", __test_num, \ __test_num-__failed_tests, __failed_tests); \ @@ -52,6 +52,6 @@ int __test_num = 0; printf("=== WARNING === We have failed tests here...\n"); \ exit(1); \ } \ -} while(0); +} while(0) #endif -- cgit v1.2.1 From 411c18bbce26dacf39e4dd792e4a091a48270349 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Tue, 22 Dec 2020 12:03:49 +0200 Subject: Remove read-only flag from non-keyspace cmds, different approach for EXEC to propagate MULTI (#8216) In the distant history there was only the read flag for commands, and whatever command that didn't have the read flag was a write one. Then we added the write flag, but some portions of the code still used !read Also some commands that don't work on the keyspace at all, still have the read flag. Changes in this commit: 1. remove the read-only flag from TIME, ECHO, ROLE and LASTSAVE 2. EXEC command used to decides if it should propagate a MULTI by looking at the command flags (!read & !admin). When i was about to change it to look at the write flag instead, i realized that this would cause it not to propagate a MULTI for PUBLISH, EVAL, and SCRIPT, all 3 are not marked as either a read command or a write one (as they should), but all 3 are calling forceCommandPropagation. So instead of introducing a new flag to denote a command that "writes" but not into the keyspace, and still needs propagation, i decided to rely on the forceCommandPropagation, and just fix the code to propagate MULTI when needed rather than depending on the command flags at all. The implication of my change then is that now it won't decide to propagate MULTI when it sees one of these: SELECT, PING, INFO, COMMAND, TIME and other commands which are neither read nor write. 3. Changing getNodeByQuery and clusterRedirectBlockedClientIfNeeded in cluster.c to look at !write rather than read flag. This should have no implications, since these code paths are only reachable for commands which access keys, and these are always marked as either read or write. This commit improve MULTI propagation tests, for modules and a bunch of other special cases, all of which used to pass already before that commit. the only one that test change that uncovered a change of behavior is the one that DELs a non-existing key, it used to propagate an empty multi-exec block, and no longer does. --- src/cluster.c | 11 ++-- src/module.c | 2 +- src/multi.c | 24 ++------ src/scripting.c | 4 +- src/server.c | 26 ++++++--- src/server.h | 4 +- tests/modules/propagate.c | 44 +++++++++----- tests/unit/moduleapi/propagate.tcl | 114 +++++++++++++++++++++++++++++++------ tests/unit/multi.tcl | 87 +++++++++++++++++++++++++++- 9 files changed, 242 insertions(+), 74 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 8651a81d3..2cd6b2521 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -5763,7 +5763,7 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in * cluster is down. */ if (error_code) *error_code = CLUSTER_REDIR_DOWN_STATE; return NULL; - } else if (!(cmd->flags & CMD_READONLY) && !(cmd->proc == evalCommand) + } else if ((cmd->flags & CMD_WRITE) && !(cmd->proc == evalCommand) && !(cmd->proc == evalShaCommand)) { /* The cluster is configured to allow read only commands @@ -5812,11 +5812,10 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in /* Handle the read-only client case reading from a slave: if this * node is a slave and the request is about a hash slot our master * is serving, we can reply without redirection. */ - int is_readonly_command = (c->cmd->flags & CMD_READONLY) || - (c->cmd->proc == execCommand && !(c->mstate.cmd_inv_flags & CMD_READONLY)); + int is_write_command = (c->cmd->flags & CMD_WRITE) || + (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_WRITE)); if (c->flags & CLIENT_READONLY && - (is_readonly_command || cmd->proc == evalCommand || - cmd->proc == evalShaCommand) && + (!is_write_command || cmd->proc == evalCommand || cmd->proc == evalShaCommand) && nodeIsSlave(myself) && myself->slaveof == n) { @@ -5901,7 +5900,7 @@ int clusterRedirectBlockedClientIfNeeded(client *c) { /* if the client is read-only and attempting to access key that our * replica can handle, allow it. */ if ((c->flags & CLIENT_READONLY) && - (c->lastcmd->flags & CMD_READONLY) && + !(c->lastcmd->flags & CMD_WRITE) && nodeIsSlave(myself) && myself->slaveof == node) { node = myself; diff --git a/src/module.c b/src/module.c index da9ac29e8..11f5f4489 100644 --- a/src/module.c +++ b/src/module.c @@ -1629,7 +1629,7 @@ void moduleReplicateMultiIfNeeded(RedisModuleCtx *ctx) { ctx->saved_oparray = server.also_propagate; redisOpArrayInit(&server.also_propagate); } - execCommandPropagateMulti(ctx->client); + execCommandPropagateMulti(ctx->client->db->id); } /* Replicate the specified command and arguments to slaves and AOF, as effect diff --git a/src/multi.c b/src/multi.c index a2f9ecccf..af0b0c612 100644 --- a/src/multi.c +++ b/src/multi.c @@ -127,15 +127,15 @@ void beforePropagateMultiOrExec(int multi) { /* Send a MULTI command to all the slaves and AOF file. Check the execCommand * implementation for more information. */ -void execCommandPropagateMulti(client *c) { +void execCommandPropagateMulti(int dbid) { beforePropagateMultiOrExec(1); - propagate(server.multiCommand,c->db->id,&shared.multi,1, + propagate(server.multiCommand,dbid,&shared.multi,1, PROPAGATE_AOF|PROPAGATE_REPL); } -void execCommandPropagateExec(client *c) { +void execCommandPropagateExec(int dbid) { beforePropagateMultiOrExec(0); - propagate(server.execCommand,c->db->id,&shared.exec,1, + propagate(server.execCommand,dbid,&shared.exec,1, PROPAGATE_AOF|PROPAGATE_REPL); } @@ -162,7 +162,6 @@ void execCommand(client *c) { robj **orig_argv; int orig_argc; struct redisCommand *orig_cmd; - int must_propagate = 0; /* Need to propagate MULTI/EXEC to AOF / slaves? */ int was_master = server.masterhost == NULL; if (!(c->flags & CLIENT_MULTI)) { @@ -202,19 +201,6 @@ void execCommand(client *c) { c->argv = c->mstate.commands[j].argv; c->cmd = c->mstate.commands[j].cmd; - /* Propagate a MULTI request once we encounter the first command which - * is not readonly nor an administrative one. - * This way we'll deliver the MULTI/..../EXEC block as a whole and - * both the AOF and the replication link will have the same consistency - * and atomicity guarantees. */ - if (!must_propagate && - !server.loading && - !(c->cmd->flags & (CMD_READONLY|CMD_ADMIN))) - { - execCommandPropagateMulti(c); - must_propagate = 1; - } - /* ACL permissions are also checked at the time of execution in case * they were changed after the commands were ququed. */ int acl_errpos; @@ -265,7 +251,7 @@ void execCommand(client *c) { /* Make sure the EXEC command will be propagated as well if MULTI * was already propagated. */ - if (must_propagate) { + if (server.propagate_in_transaction) { int is_master = server.masterhost == NULL; server.dirty++; beforePropagateMultiOrExec(0); diff --git a/src/scripting.c b/src/scripting.c index 8dca84478..bc137de25 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -721,7 +721,7 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { server.lua_write_dirty && server.lua_repl != PROPAGATE_NONE) { - execCommandPropagateMulti(server.lua_caller); + execCommandPropagateMulti(server.lua_caller->db->id); server.lua_multi_emitted = 1; /* Now we are in the MULTI context, the lua_client should be * flag as CLIENT_MULTI. */ @@ -1638,7 +1638,7 @@ void evalGenericCommand(client *c, int evalsha) { if (server.lua_replicate_commands) { preventCommandPropagation(c); if (server.lua_multi_emitted) { - execCommandPropagateExec(c); + execCommandPropagateExec(c->db->id); } } diff --git a/src/server.c b/src/server.c index 279e75f2a..29c810928 100644 --- a/src/server.c +++ b/src/server.c @@ -115,9 +115,9 @@ struct redisServer server; /* Server global state */ * * write: Write command (may modify the key space). * - * read-only: All the non special commands just reading from keys without - * changing the content, or returning other information like - * the TIME command. Special commands such administrative commands + * read-only: Commands just reading from keys without changing the content. + * Note that commands that don't read from the keyspace such as + * TIME, SELECT, INFO, administrative commands, and connection * or transaction related commands (multi, exec, discard, ...) * are not flagged as read-only commands, since they affect the * server or the connection in other ways. @@ -685,7 +685,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"echo",echoCommand,2, - "read-only fast @connection", + "fast @connection", 0,NULL,0,0,0,0,0,0}, {"save",saveCommand,1, @@ -705,7 +705,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"lastsave",lastsaveCommand,1, - "read-only random fast ok-loading ok-stale @admin @dangerous", + "random fast ok-loading ok-stale @admin @dangerous", 0,NULL,0,0,0,0,0,0}, {"type",typeCommand,2, @@ -781,7 +781,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"role",roleCommand,1, - "ok-loading ok-stale no-script fast read-only @dangerous", + "ok-loading ok-stale no-script fast @dangerous", 0,NULL,0,0,0,0,0,0}, {"debug",debugCommand,-2, @@ -891,7 +891,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"time",timeCommand,1, - "read-only random fast ok-loading ok-stale", + "random fast ok-loading ok-stale", 0,NULL,0,0,0,0,0,0}, {"bitop",bitopCommand,-4, @@ -3377,6 +3377,14 @@ struct redisCommand *lookupCommandOrOriginal(sds name) { void propagate(struct redisCommand *cmd, int dbid, robj **argv, int argc, int flags) { + /* Propagate a MULTI request once we encounter the first command which + * is a write command. + * This way we'll deliver the MULTI/..../EXEC block as a whole and + * both the AOF and the replication link will have the same consistency + * and atomicity guarantees. */ + if (server.in_exec && !server.propagate_in_transaction) + execCommandPropagateMulti(dbid); + if (server.aof_state != AOF_OFF && flags & PROPAGATE_AOF) feedAppendOnlyFile(cmd,dbid,argv,argc); if (flags & PROPAGATE_REPL) @@ -3607,7 +3615,7 @@ void call(client *c, int flags) { !(c->flags & CLIENT_MULTI) && !(flags & CMD_CALL_NOWRAP)) { - execCommandPropagateMulti(c); + execCommandPropagateMulti(c->db->id); multi_emitted = 1; } @@ -3622,7 +3630,7 @@ void call(client *c, int flags) { } if (multi_emitted) { - execCommandPropagateExec(c); + execCommandPropagateExec(c->db->id); } } redisOpArrayFree(&server.also_propagate); diff --git a/src/server.h b/src/server.h index 8a1f191f3..04fd709a8 100644 --- a/src/server.h +++ b/src/server.h @@ -1861,8 +1861,8 @@ void touchWatchedKeysOnFlush(int dbid); void discardTransaction(client *c); void flagTransaction(client *c); void execCommandAbort(client *c, sds error); -void execCommandPropagateMulti(client *c); -void execCommandPropagateExec(client *c); +void execCommandPropagateMulti(int dbid); +void execCommandPropagateExec(int dbid); void beforePropagateMultiOrExec(int multi); /* Redis object implementation */ diff --git a/tests/modules/propagate.c b/tests/modules/propagate.c index 13277b19d..70cddacbd 100644 --- a/tests/modules/propagate.c +++ b/tests/modules/propagate.c @@ -51,18 +51,31 @@ void timerHandler(RedisModuleCtx *ctx, void *data) { RedisModule_Replicate(ctx,"INCR","c","timer"); times++; - if (times < 10) + if (times < 3) RedisModule_CreateTimer(ctx,100,timerHandler,NULL); else times = 0; } +int propagateTestTimerCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + REDISMODULE_NOT_USED(argv); + REDISMODULE_NOT_USED(argc); + + RedisModuleTimerID timer_id = + RedisModule_CreateTimer(ctx,100,timerHandler,NULL); + REDISMODULE_NOT_USED(timer_id); + + RedisModule_ReplyWithSimpleString(ctx,"OK"); + return REDISMODULE_OK; +} + /* The thread entry point. */ void *threadMain(void *arg) { REDISMODULE_NOT_USED(arg); RedisModuleCtx *ctx = RedisModule_GetThreadSafeContext(NULL); RedisModule_SelectDb(ctx,9); /* Tests ran in database number 9. */ - for (int i = 0; i < 10; i++) { + for (int i = 0; i < 3; i++) { RedisModule_ThreadSafeContextLock(ctx); RedisModule_Replicate(ctx,"INCR","c","a-from-thread"); RedisModule_Replicate(ctx,"INCR","c","b-from-thread"); @@ -72,15 +85,11 @@ void *threadMain(void *arg) { return NULL; } -int propagateTestCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int propagateTestThreadCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc); - RedisModuleTimerID timer_id = - RedisModule_CreateTimer(ctx,100,timerHandler,NULL); - REDISMODULE_NOT_USED(timer_id); - pthread_t tid; if (pthread_create(&tid,NULL,threadMain,NULL) != 0) return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread"); @@ -90,7 +99,7 @@ int propagateTestCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc return REDISMODULE_OK; } -int propagateTest2Command(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int propagateTestSimpleCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc); @@ -102,7 +111,7 @@ int propagateTest2Command(RedisModuleCtx *ctx, RedisModuleString **argv, int arg return REDISMODULE_OK; } -int propagateTest3Command(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int propagateTestMixedCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc); @@ -129,18 +138,23 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_Init(ctx,"propagate-test",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (RedisModule_CreateCommand(ctx,"propagate-test", - propagateTestCommand, + if (RedisModule_CreateCommand(ctx,"propagate-test.timer", + propagateTestTimerCommand, + "",1,1,1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + + if (RedisModule_CreateCommand(ctx,"propagate-test.thread", + propagateTestThreadCommand, "",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (RedisModule_CreateCommand(ctx,"propagate-test-2", - propagateTest2Command, + if (RedisModule_CreateCommand(ctx,"propagate-test.simple", + propagateTestSimpleCommand, "",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (RedisModule_CreateCommand(ctx,"propagate-test-3", - propagateTest3Command, + if (RedisModule_CreateCommand(ctx,"propagate-test.mixed", + propagateTestMixedCommand, "",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; diff --git a/tests/unit/moduleapi/propagate.tcl b/tests/unit/moduleapi/propagate.tcl index aa0f55e5e..adebd37a6 100644 --- a/tests/unit/moduleapi/propagate.tcl +++ b/tests/unit/moduleapi/propagate.tcl @@ -14,25 +14,103 @@ tags "modules" { # Start the replication process... $replica replicaof $master_host $master_port wait_for_sync $replica - after 1000 - $master propagate-test - wait_for_condition 5000 10 { - ([$replica get timer] eq "10") && \ - ([$replica get a-from-thread] eq "10") - } else { - fail "The two counters don't match the expected value." + test {module propagates from timer} { + set repl [attach_to_replication_stream] + + $master propagate-test.timer + + wait_for_condition 5000 10 { + [$replica get timer] eq "3" + } else { + fail "The two counters don't match the expected value." + } + + assert_replication_stream $repl { + {select *} + {multi} + {incr timer} + {exec} + {multi} + {incr timer} + {exec} + {multi} + {incr timer} + {exec} + } + close_replication_stream $repl } - $master propagate-test-2 - $master propagate-test-3 - $master multi - $master propagate-test-2 - $master propagate-test-3 - $master exec - wait_for_ofs_sync $master $replica + test {module propagates from thread} { + set repl [attach_to_replication_stream] + + $master propagate-test.thread + + wait_for_condition 5000 10 { + [$replica get a-from-thread] eq "3" + } else { + fail "The two counters don't match the expected value." + } + + assert_replication_stream $repl { + {select *} + {incr a-from-thread} + {incr b-from-thread} + {incr a-from-thread} + {incr b-from-thread} + {incr a-from-thread} + {incr b-from-thread} + } + close_replication_stream $repl + } + test {module propagates from from command} { + set repl [attach_to_replication_stream] + + $master propagate-test.simple + $master propagate-test.mixed + + # Note the 'after-call' propagation below is out of order (known limitation) + assert_replication_stream $repl { + {select *} + {multi} + {incr counter-1} + {incr counter-2} + {exec} + {multi} + {incr using-call} + {incr after-call} + {incr counter-1} + {incr counter-2} + {exec} + } + close_replication_stream $repl + } + + test {module propagates from from multi-exec} { + set repl [attach_to_replication_stream] + + $master multi + $master propagate-test.simple + $master propagate-test.mixed + $master exec + wait_for_ofs_sync $master $replica + + # Note the 'after-call' propagation below is out of order (known limitation) + assert_replication_stream $repl { + {select *} + {multi} + {incr counter-1} + {incr counter-2} + {incr using-call} + {incr after-call} + {incr counter-1} + {incr counter-2} + {exec} + } + close_replication_stream $repl + } assert_equal [s -1 unexpected_error_replies] 0 } } @@ -47,11 +125,11 @@ tags "modules aof" { r config set auto-aof-rewrite-percentage 0 ; # Disable auto-rewrite. waitForBgrewriteaof r - r propagate-test-2 - r propagate-test-3 + r propagate-test.simple + r propagate-test.mixed r multi - r propagate-test-2 - r propagate-test-3 + r propagate-test.simple + r propagate-test.mixed r exec # Load the AOF diff --git a/tests/unit/multi.tcl b/tests/unit/multi.tcl index 43259b1c0..25e417055 100644 --- a/tests/unit/multi.tcl +++ b/tests/unit/multi.tcl @@ -299,10 +299,14 @@ start_server {tags {"multi"}} { r multi r del foo r exec + + # add another command so that when we see it we know multi-exec wasn't + # propagated + r incr foo + assert_replication_stream $repl { {select *} - {multi} - {exec} + {incr foo} } close_replication_stream $repl } @@ -521,4 +525,83 @@ start_server {tags {"multi"}} { list $m $res } {OK {{} {} {} {} {} {} {} {}}} + + test {MULTI propagation of PUBLISH} { + set repl [attach_to_replication_stream] + + # make sure that PUBLISH inside MULTI is propagated in a transaction + r multi + r publish bla bla + r exec + + assert_replication_stream $repl { + {select *} + {multi} + {publish bla bla} + {exec} + } + close_replication_stream $repl + } + + test {MULTI propagation of SCRIPT LOAD} { + set repl [attach_to_replication_stream] + + # make sure that SCRIPT LOAD inside MULTI is propagated in a transaction + r multi + r script load {redis.call('set', KEYS[1], 'foo')} + set res [r exec] + set sha [lindex $res 0] + + assert_replication_stream $repl { + {select *} + {multi} + {script load *} + {exec} + } + close_replication_stream $repl + } + + test {MULTI propagation of SCRIPT LOAD} { + set repl [attach_to_replication_stream] + + # make sure that EVAL inside MULTI is propagated in a transaction + r config set lua-replicate-commands no + r multi + r eval {redis.call('set', KEYS[1], 'bar')} 1 bar + r exec + + assert_replication_stream $repl { + {select *} + {multi} + {eval *} + {exec} + } + close_replication_stream $repl + } + + tags {"stream"} { + test {MULTI propagation of XREADGROUP} { + # stream is a special case because it calls propagate() directly for XREADGROUP + set repl [attach_to_replication_stream] + + r XADD mystream * foo bar + r XGROUP CREATE mystream mygroup 0 + + # make sure the XCALIM (propagated by XREADGROUP) is indeed inside MULTI/EXEC + r multi + r XREADGROUP GROUP mygroup consumer1 STREAMS mystream ">" + r exec + + assert_replication_stream $repl { + {select *} + {xadd *} + {xgroup CREATE *} + {multi} + {xclaim *} + {exec} + } + close_replication_stream $repl + } + } + } -- cgit v1.2.1 From e7047ec2fcc20e150c0c8cf29addf582638d7e80 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Tue, 22 Dec 2020 12:24:20 +0200 Subject: Fix crashes with io-threads-do-reads enabled. (#8230) Normally IO threads should simply read data from the socket into the buffer and attempt to parse it. If a protocol error is detected, a reply is generated which may result with installing a write handler which is not thread safe. This fix delays that until the client is processed back in the main thread. Fixes #8220 --- src/networking.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index b81060cbb..45c504e46 100644 --- a/src/networking.c +++ b/src/networking.c @@ -259,8 +259,14 @@ int prepareClientToWrite(client *c) { if (!c->conn) return C_ERR; /* Fake client for AOF loading. */ /* Schedule the client to write the output buffers to the socket, unless - * it should already be setup to do so (it has already pending data). */ - if (!clientHasPendingReplies(c)) clientInstallWriteHandler(c); + * it should already be setup to do so (it has already pending data). + * + * If CLIENT_PENDING_READ is set, we're in an IO thread and should + * not install a write handler. Instead, it will be done by + * handleClientsWithPendingReadsUsingThreads() upon return. + */ + if (!clientHasPendingReplies(c) && !(c->flags & CLIENT_PENDING_READ)) + clientInstallWriteHandler(c); /* Authorize the caller to queue in the output buffer of this client. */ return C_OK; @@ -3509,6 +3515,12 @@ int handleClientsWithPendingReadsUsingThreads(void) { } } processInputBuffer(c); + + /* We may have pending replies if a thread readQueryFromClient() produced + * replies and did not install a write handler (it can't). + */ + if (!(c->flags & CLIENT_PENDING_WRITE) && clientHasPendingReplies(c)) + clientInstallWriteHandler(c); } /* Update processed count on server */ -- cgit v1.2.1 From 92a483bca2df734aff5caada6c23409ed6256773 Mon Sep 17 00:00:00 2001 From: "Meir Shpilraien (Spielrein)" Date: Tue, 22 Dec 2020 15:17:39 +0200 Subject: Fix issue where fork process deletes the parent pidfile (#8231) Turns out that when the fork child crashes, the crash log was deleting the pidfile from the disk (although the parent is still running. Now we set the pidfile of the fork process to NULL so the fork process will never deletes it. --- src/debug.c | 2 +- src/server.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/debug.c b/src/debug.c index 6ce8b3bdc..26c1a91fc 100644 --- a/src/debug.c +++ b/src/debug.c @@ -1808,7 +1808,7 @@ void bugReportEnd(int killViaSignal, int sig) { ); /* free(messages); Don't call free() with possibly corrupted memory. */ - if (server.daemonize && server.supervised == 0) unlink(server.pidfile); + if (server.daemonize && server.supervised == 0 && server.pidfile) unlink(server.pidfile); if (!killViaSignal) { if (server.use_exit_on_panic) diff --git a/src/server.c b/src/server.c index 29c810928..0be8fb003 100644 --- a/src/server.c +++ b/src/server.c @@ -5200,6 +5200,10 @@ void closeClildUnusedResourceAfterFork() { closeListeningSockets(0); if (server.cluster_enabled && server.cluster_config_file_lock_fd != -1) close(server.cluster_config_file_lock_fd); /* don't care if this fails */ + + /* Clear server.pidfile, this is the parent pidfile which should not + * be touched (or deleted) by the child (on exit / crash) */ + server.pidfile = NULL; } /* purpose is one of CHILD_TYPE_ types */ -- cgit v1.2.1 From 781d7b0d9b51d1d80097c1f6ddec6270c1532ede Mon Sep 17 00:00:00 2001 From: Wen Hui Date: Tue, 22 Dec 2020 09:14:15 -0500 Subject: Sentinel: add missing calls for sentinelflushconfig when config master at runtime (#8229) --- src/sentinel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/sentinel.c b/src/sentinel.c index 95cfa84ad..7693ac5ae 100644 --- a/src/sentinel.c +++ b/src/sentinel.c @@ -3579,14 +3579,13 @@ void sentinelSetCommand(client *c) { "Reconfiguration of scripts path is denied for " "security reasons. Check the deny-scripts-reconfig " "configuration directive in your Sentinel configuration"); - return; + goto seterr; } if (strlen(value) && access(value,X_OK) == -1) { addReplyError(c, "Notification script seems non existing or non executable"); - if (changes) sentinelFlushConfig(); - return; + goto seterr; } sdsfree(ri->notification_script); ri->notification_script = strlen(value) ? sdsnew(value) : NULL; @@ -3599,15 +3598,14 @@ void sentinelSetCommand(client *c) { "Reconfiguration of scripts path is denied for " "security reasons. Check the deny-scripts-reconfig " "configuration directive in your Sentinel configuration"); - return; + goto seterr; } if (strlen(value) && access(value,X_OK) == -1) { addReplyError(c, "Client reconfiguration script seems non existing or " "non executable"); - if (changes) sentinelFlushConfig(); - return; + goto seterr; } sdsfree(ri->client_reconfig_script); ri->client_reconfig_script = strlen(value) ? sdsnew(value) : NULL; @@ -3657,8 +3655,7 @@ void sentinelSetCommand(client *c) { } else { addReplyErrorFormat(c,"Unknown option or number of arguments for " "SENTINEL SET '%s'", option); - if (changes) sentinelFlushConfig(); - return; + goto seterr; } /* Log the event. */ @@ -3684,9 +3681,11 @@ void sentinelSetCommand(client *c) { return; badfmt: /* Bad format errors */ - if (changes) sentinelFlushConfig(); addReplyErrorFormat(c,"Invalid argument '%s' for SENTINEL SET '%s'", (char*)c->argv[badarg]->ptr,option); +seterr: + if (changes) sentinelFlushConfig(); + return; } /* Our fake PUBLISH command: it is actually useful only to receive hello messages -- cgit v1.2.1 From b51f5da31446a56f6e83e021f4f44fb472999c7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20B=C3=BCnemann?= Date: Wed, 23 Dec 2020 08:46:23 +0100 Subject: Fix TLS build on macOS arm64 systems (#8197) Homebrew for darwin-arm64 uses /opt/homebrew instead of /usr/local as the prefix, so that it can coexist with darwin-x86_64 using Rosetta 2. --- src/Makefile | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Makefile b/src/Makefile index 32cedbac5..068dbc9da 100644 --- a/src/Makefile +++ b/src/Makefile @@ -115,8 +115,18 @@ else ifeq ($(uname_S),Darwin) # Darwin FINAL_LIBS+= -ldl + # Homebrew's OpenSSL is not linked to /usr/local to avoid + # conflicts with the system's LibreSSL installation so it + # must be referenced explicitly during build. +ifeq ($(uname_M),arm64) + # Homebrew arm64 uses /opt/homebrew as HOMEBREW_PREFIX + OPENSSL_CFLAGS=-I/opt/homebrew/opt/openssl/include + OPENSSL_LDFLAGS=-L/opt/homebrew/opt/openssl/lib +else + # Homebrew x86/ppc uses /usr/local as HOMEBREW_PREFIX OPENSSL_CFLAGS=-I/usr/local/opt/openssl/include OPENSSL_LDFLAGS=-L/usr/local/opt/openssl/lib +endif else ifeq ($(uname_S),AIX) # AIX -- cgit v1.2.1 From 2426aaa099e5dfee29cce17af39298d0ce14cc2a Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 23 Dec 2020 12:55:05 +0200 Subject: fix valgrind warning created by recent pidfile fix (#8235) This isn't a leak, just an warning due to unreachable allocation on the fork child. Problem created by 92a483b --- src/server.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server.c b/src/server.c index 0be8fb003..4e9bb88eb 100644 --- a/src/server.c +++ b/src/server.c @@ -5203,6 +5203,7 @@ void closeClildUnusedResourceAfterFork() { /* Clear server.pidfile, this is the parent pidfile which should not * be touched (or deleted) by the child (on exit / crash) */ + zfree(server.pidfile); server.pidfile = NULL; } -- cgit v1.2.1 From 266949c7fcfab9d10f81314fd7480a00638ced80 Mon Sep 17 00:00:00 2001 From: Greg Femec Date: Wed, 23 Dec 2020 05:52:07 -0800 Subject: Fix random element selection for large hash tables. (#8133) When a database on a 64 bit build grows past 2^31 keys, the underlying hash table expands to 2^32 buckets. After this point, the algorithms for selecting random elements only return elements from half of the available buckets because they use random() which has a range of 0 to 2^31 - 1. This causes problems for eviction policies which use dictGetSomeKeys or dictGetRandomKey. Over time they cause the hash table to become unbalanced because, while new keys are spread out evenly across all buckets, evictions come from only half of the available buckets. Eventually this half of the table starts to run out of keys and it takes longer and longer to find candidates for eviction. This continues until no more evictions can happen. This solution addresses this by using a 64 bit PRNG instead of libc random(). Co-authored-by: Greg Femec --- src/Makefile | 8 +-- src/dict.c | 15 ++-- src/dict.h | 14 +++- src/mt19937-64.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/mt19937-64.h | 87 +++++++++++++++++++++++ src/redis-benchmark.c | 2 + src/redis-check-rdb.c | 9 +++ src/redis-cli.c | 5 ++ src/server.c | 2 + 9 files changed, 319 insertions(+), 10 deletions(-) create mode 100644 src/mt19937-64.c create mode 100644 src/mt19937-64.h diff --git a/src/Makefile b/src/Makefile index 068dbc9da..3bc9f11c0 100644 --- a/src/Makefile +++ b/src/Makefile @@ -270,11 +270,11 @@ endif REDIS_SERVER_NAME=redis-server$(PROG_SUFFIX) REDIS_SENTINEL_NAME=redis-sentinel$(PROG_SUFFIX) -REDIS_SERVER_OBJ=adlist.o quicklist.o ae.o anet.o dict.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o crc16.o endianconv.o slowlog.o scripting.o bio.o rio.o rand.o memtest.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o redis-check-rdb.o redis-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o gopher.o tracking.o connection.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o +REDIS_SERVER_OBJ=adlist.o quicklist.o ae.o anet.o dict.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o crc16.o endianconv.o slowlog.o scripting.o bio.o rio.o rand.o memtest.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o redis-check-rdb.o redis-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o gopher.o tracking.o connection.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o REDIS_CLI_NAME=redis-cli$(PROG_SUFFIX) -REDIS_CLI_OBJ=anet.o adlist.o dict.o redis-cli.o zmalloc.o release.o ae.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o +REDIS_CLI_OBJ=anet.o adlist.o dict.o redis-cli.o zmalloc.o release.o ae.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o REDIS_BENCHMARK_NAME=redis-benchmark$(PROG_SUFFIX) -REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o adlist.o dict.o zmalloc.o release.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o +REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o adlist.o dict.o zmalloc.o release.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o REDIS_CHECK_RDB_NAME=redis-check-rdb$(PROG_SUFFIX) REDIS_CHECK_AOF_NAME=redis-check-aof$(PROG_SUFFIX) @@ -346,7 +346,7 @@ $(REDIS_CLI_NAME): $(REDIS_CLI_OBJ) $(REDIS_BENCHMARK_NAME): $(REDIS_BENCHMARK_OBJ) $(REDIS_LD) -o $@ $^ ../deps/hiredis/libhiredis.a ../deps/hdr_histogram/hdr_histogram.o $(FINAL_LIBS) -dict-benchmark: dict.c zmalloc.c sds.c siphash.c +dict-benchmark: dict.c zmalloc.c sds.c siphash.c mt19937-64.c $(REDIS_CC) $(FINAL_CFLAGS) $^ -D DICT_BENCHMARK_MAIN -o $@ $(FINAL_LIBS) DEP = $(REDIS_SERVER_OBJ:%.o=%.d) $(REDIS_CLI_OBJ:%.o=%.d) $(REDIS_BENCHMARK_OBJ:%.o=%.d) diff --git a/src/dict.c b/src/dict.c index 4f0916a27..6c203b850 100644 --- a/src/dict.c +++ b/src/dict.c @@ -646,13 +646,13 @@ dictEntry *dictGetRandomKey(dict *d) do { /* We are sure there are no elements in indexes from 0 * to rehashidx-1 */ - h = d->rehashidx + (random() % (dictSlots(d) - d->rehashidx)); + h = d->rehashidx + (randomULong() % (dictSlots(d) - d->rehashidx)); he = (h >= d->ht[0].size) ? d->ht[1].table[h - d->ht[0].size] : d->ht[0].table[h]; } while(he == NULL); } else { do { - h = random() & d->ht[0].sizemask; + h = randomULong() & d->ht[0].sizemask; he = d->ht[0].table[h]; } while(he == NULL); } @@ -718,7 +718,7 @@ unsigned int dictGetSomeKeys(dict *d, dictEntry **des, unsigned int count) { maxsizemask = d->ht[1].sizemask; /* Pick a random point inside the larger table. */ - unsigned long i = random() & maxsizemask; + unsigned long i = randomULong() & maxsizemask; unsigned long emptylen = 0; /* Continuous empty entries so far. */ while(stored < count && maxsteps--) { for (j = 0; j < tables; j++) { @@ -743,7 +743,7 @@ unsigned int dictGetSomeKeys(dict *d, dictEntry **des, unsigned int count) { if (he == NULL) { emptylen++; if (emptylen >= 5 && emptylen > count) { - i = random() & maxsizemask; + i = randomULong() & maxsizemask; emptylen = 0; } } else { @@ -1270,6 +1270,13 @@ int main(int argc, char **argv) { } end_benchmark("Random access of existing elements"); + start_benchmark(); + for (j = 0; j < count; j++) { + dictEntry *de = dictGetRandomKey(dict); + assert(de != NULL); + } + end_benchmark("Accessing random keys"); + start_benchmark(); for (j = 0; j < count; j++) { sds key = sdsfromlonglong(rand() % count); diff --git a/src/dict.h b/src/dict.h index f7515e905..d96c3148f 100644 --- a/src/dict.h +++ b/src/dict.h @@ -33,11 +33,14 @@ * POSSIBILITY OF SUCH DAMAGE. */ -#include - #ifndef __DICT_H #define __DICT_H +#include "mt19937-64.h" +#include +#include +#include + #define DICT_OK 0 #define DICT_ERR 1 @@ -148,6 +151,13 @@ typedef void (dictScanBucketFunction)(void *privdata, dictEntry **bucketref); #define dictSize(d) ((d)->ht[0].used+(d)->ht[1].used) #define dictIsRehashing(d) ((d)->rehashidx != -1) +/* If our unsigned long type can store a 64 bit number, use a 64 bit PRNG. */ +#if ULONG_MAX >= 0xffffffffffffffff +#define randomULong() ((unsigned long) genrand64_int64()) +#else +#define randomULong() random() +#endif + /* API */ dict *dictCreate(dictType *type, void *privDataPtr); int dictExpand(dict *d, unsigned long size); diff --git a/src/mt19937-64.c b/src/mt19937-64.c new file mode 100644 index 000000000..a0c897ff6 --- /dev/null +++ b/src/mt19937-64.c @@ -0,0 +1,187 @@ +/* + A C-program for MT19937-64 (2004/9/29 version). + Coded by Takuji Nishimura and Makoto Matsumoto. + + This is a 64-bit version of Mersenne Twister pseudorandom number + generator. + + Before using, initialize the state by using init_genrand64(seed) + or init_by_array64(init_key, key_length). + + Copyright (C) 2004, Makoto Matsumoto and Takuji Nishimura, + All rights reserved. + + Redistribution and use in source and binary forms, with or without + modification, are permitted provided that the following conditions + are met: + + 1. Redistributions of source code must retain the above copyright + notice, this list of conditions and the following disclaimer. + + 2. Redistributions in binary form must reproduce the above copyright + notice, this list of conditions and the following disclaimer in the + documentation and/or other materials provided with the distribution. + + 3. The names of its contributors may not be used to endorse or promote + products derived from this software without specific prior written + permission. + + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, + EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, + PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR + PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF + LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING + NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + + References: + T. Nishimura, ``Tables of 64-bit Mersenne Twisters'' + ACM Transactions on Modeling and + Computer Simulation 10. (2000) 348--357. + M. Matsumoto and T. Nishimura, + ``Mersenne Twister: a 623-dimensionally equidistributed + uniform pseudorandom number generator'' + ACM Transactions on Modeling and + Computer Simulation 8. (Jan. 1998) 3--30. + + Any feedback is very welcome. + http://www.math.hiroshima-u.ac.jp/~m-mat/MT/emt.html + email: m-mat @ math.sci.hiroshima-u.ac.jp (remove spaces) +*/ + + +#include "mt19937-64.h" +#include + +#define NN 312 +#define MM 156 +#define MATRIX_A 0xB5026F5AA96619E9ULL +#define UM 0xFFFFFFFF80000000ULL /* Most significant 33 bits */ +#define LM 0x7FFFFFFFULL /* Least significant 31 bits */ + + +/* The array for the state vector */ +static unsigned long long mt[NN]; +/* mti==NN+1 means mt[NN] is not initialized */ +static int mti=NN+1; + +/* initializes mt[NN] with a seed */ +void init_genrand64(unsigned long long seed) +{ + mt[0] = seed; + for (mti=1; mti> 62)) + mti); +} + +/* initialize by an array with array-length */ +/* init_key is the array for initializing keys */ +/* key_length is its length */ +void init_by_array64(unsigned long long init_key[], + unsigned long long key_length) +{ + unsigned long long i, j, k; + init_genrand64(19650218ULL); + i=1; j=0; + k = (NN>key_length ? NN : key_length); + for (; k; k--) { + mt[i] = (mt[i] ^ ((mt[i-1] ^ (mt[i-1] >> 62)) * 3935559000370003845ULL)) + + init_key[j] + j; /* non linear */ + i++; j++; + if (i>=NN) { mt[0] = mt[NN-1]; i=1; } + if (j>=key_length) j=0; + } + for (k=NN-1; k; k--) { + mt[i] = (mt[i] ^ ((mt[i-1] ^ (mt[i-1] >> 62)) * 2862933555777941757ULL)) + - i; /* non linear */ + i++; + if (i>=NN) { mt[0] = mt[NN-1]; i=1; } + } + + mt[0] = 1ULL << 63; /* MSB is 1; assuring non-zero initial array */ +} + +/* generates a random number on [0, 2^64-1]-interval */ +unsigned long long genrand64_int64(void) +{ + int i; + unsigned long long x; + static unsigned long long mag01[2]={0ULL, MATRIX_A}; + + if (mti >= NN) { /* generate NN words at one time */ + + /* if init_genrand64() has not been called, */ + /* a default initial seed is used */ + if (mti == NN+1) + init_genrand64(5489ULL); + + for (i=0;i>1) ^ mag01[(int)(x&1ULL)]; + } + for (;i>1) ^ mag01[(int)(x&1ULL)]; + } + x = (mt[NN-1]&UM)|(mt[0]&LM); + mt[NN-1] = mt[MM-1] ^ (x>>1) ^ mag01[(int)(x&1ULL)]; + + mti = 0; + } + + x = mt[mti++]; + + x ^= (x >> 29) & 0x5555555555555555ULL; + x ^= (x << 17) & 0x71D67FFFEDA60000ULL; + x ^= (x << 37) & 0xFFF7EEE000000000ULL; + x ^= (x >> 43); + + return x; +} + +/* generates a random number on [0, 2^63-1]-interval */ +long long genrand64_int63(void) +{ + return (long long)(genrand64_int64() >> 1); +} + +/* generates a random number on [0,1]-real-interval */ +double genrand64_real1(void) +{ + return (genrand64_int64() >> 11) * (1.0/9007199254740991.0); +} + +/* generates a random number on [0,1)-real-interval */ +double genrand64_real2(void) +{ + return (genrand64_int64() >> 11) * (1.0/9007199254740992.0); +} + +/* generates a random number on (0,1)-real-interval */ +double genrand64_real3(void) +{ + return ((genrand64_int64() >> 12) + 0.5) * (1.0/4503599627370496.0); +} + +#ifdef MT19937_64_MAIN +int main(void) +{ + int i; + unsigned long long init[4]={0x12345ULL, 0x23456ULL, 0x34567ULL, 0x45678ULL}, length=4; + init_by_array64(init, length); + printf("1000 outputs of genrand64_int64()\n"); + for (i=0; i<1000; i++) { + printf("%20llu ", genrand64_int64()); + if (i%5==4) printf("\n"); + } + printf("\n1000 outputs of genrand64_real2()\n"); + for (i=0; i<1000; i++) { + printf("%10.8f ", genrand64_real2()); + if (i%5==4) printf("\n"); + } + return 0; +} +#endif diff --git a/src/mt19937-64.h b/src/mt19937-64.h new file mode 100644 index 000000000..b98348fd4 --- /dev/null +++ b/src/mt19937-64.h @@ -0,0 +1,87 @@ +/* + A C-program for MT19937-64 (2004/9/29 version). + Coded by Takuji Nishimura and Makoto Matsumoto. + + This is a 64-bit version of Mersenne Twister pseudorandom number + generator. + + Before using, initialize the state by using init_genrand64(seed) + or init_by_array64(init_key, key_length). + + Copyright (C) 2004, Makoto Matsumoto and Takuji Nishimura, + All rights reserved. + + Redistribution and use in source and binary forms, with or without + modification, are permitted provided that the following conditions + are met: + + 1. Redistributions of source code must retain the above copyright + notice, this list of conditions and the following disclaimer. + + 2. Redistributions in binary form must reproduce the above copyright + notice, this list of conditions and the following disclaimer in the + documentation and/or other materials provided with the distribution. + + 3. The names of its contributors may not be used to endorse or promote + products derived from this software without specific prior written + permission. + + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, + EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, + PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR + PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF + LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING + NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + + References: + T. Nishimura, ``Tables of 64-bit Mersenne Twisters'' + ACM Transactions on Modeling and + Computer Simulation 10. (2000) 348--357. + M. Matsumoto and T. Nishimura, + ``Mersenne Twister: a 623-dimensionally equidistributed + uniform pseudorandom number generator'' + ACM Transactions on Modeling and + Computer Simulation 8. (Jan. 1998) 3--30. + + Any feedback is very welcome. + http://www.math.hiroshima-u.ac.jp/~m-mat/MT/emt.html + email: m-mat @ math.sci.hiroshima-u.ac.jp (remove spaces) +*/ + +#ifndef __MT19937_64_H +#define __MT19937_64_H + +/* initializes mt[NN] with a seed */ +void init_genrand64(unsigned long long seed); + +/* initialize by an array with array-length */ +/* init_key is the array for initializing keys */ +/* key_length is its length */ +void init_by_array64(unsigned long long init_key[], + unsigned long long key_length); + +/* generates a random number on [0, 2^64-1]-interval */ +unsigned long long genrand64_int64(void); + + +/* generates a random number on [0, 2^63-1]-interval */ +long long genrand64_int63(void); + +/* generates a random number on [0,1]-real-interval */ +double genrand64_real1(void); + +/* generates a random number on [0,1)-real-interval */ +double genrand64_real2(void); + +/* generates a random number on (0,1)-real-interval */ +double genrand64_real3(void); + +/* generates a random number on (0,1]-real-interval */ +double genrand64_real4(void); + +#endif diff --git a/src/redis-benchmark.c b/src/redis-benchmark.c index 18e19b0e0..a955c0d4c 100644 --- a/src/redis-benchmark.c +++ b/src/redis-benchmark.c @@ -59,6 +59,7 @@ #include "crc16_slottable.h" #include "hdr_histogram.h" #include "cli_common.h" +#include "mt19937-64.h" #define UNUSED(V) ((void) V) #define RANDPTR_INITIAL_SIZE 8 @@ -1677,6 +1678,7 @@ int main(int argc, const char **argv) { client c; srandom(time(NULL) ^ getpid()); + init_genrand64(ustime() ^ getpid()); signal(SIGHUP, SIG_IGN); signal(SIGPIPE, SIG_IGN); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index 79dbf3229..335e35189 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -27,10 +27,13 @@ * POSSIBILITY OF SUCH DAMAGE. */ +#include "mt19937-64.h" #include "server.h" #include "rdb.h" #include +#include +#include void createSharedObjects(void); void rdbLoadProgressCallback(rio *r, const void *buf, size_t len); @@ -362,10 +365,16 @@ err: * Otherwise if called with a non NULL fp, the function returns C_OK or * C_ERR depending on the success or failure. */ int redis_check_rdb_main(int argc, char **argv, FILE *fp) { + struct timeval tv; + if (argc != 2 && fp == NULL) { fprintf(stderr, "Usage: %s \n", argv[0]); exit(1); } + + gettimeofday(&tv, NULL); + init_genrand64(((long long) tv.tv_sec * 1000000 + tv.tv_usec) ^ getpid()); + /* In order to call the loading functions we need to create the shared * integer objects, however since this function may be called from * an already initialized Redis instance, check if we really need to. */ diff --git a/src/redis-cli.c b/src/redis-cli.c index 5310a1b21..ff29de748 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -62,6 +62,7 @@ #include "anet.h" #include "ae.h" #include "cli_common.h" +#include "mt19937-64.h" #define UNUSED(V) ((void) V) @@ -8123,6 +8124,7 @@ static sds askPassword(const char *msg) { int main(int argc, char **argv) { int firstarg; + struct timeval tv; config.hostip = sdsnew("127.0.0.1"); config.hostport = 6379; @@ -8219,6 +8221,9 @@ int main(int argc, char **argv) { } #endif + gettimeofday(&tv, NULL); + init_genrand64(((long long) tv.tv_sec * 1000000 + tv.tv_usec) ^ getpid()); + /* Cluster Manager mode */ if (CLUSTER_MANAGER_MODE()) { clusterManagerCommandProc *proc = validateClusterManagerCommand(); diff --git a/src/server.c b/src/server.c index 4e9bb88eb..611935a6c 100644 --- a/src/server.c +++ b/src/server.c @@ -34,6 +34,7 @@ #include "bio.h" #include "latency.h" #include "atomicvar.h" +#include "mt19937-64.h" #include #include @@ -5452,6 +5453,7 @@ int main(int argc, char **argv) { srand(time(NULL)^getpid()); srandom(time(NULL)^getpid()); gettimeofday(&tv,NULL); + init_genrand64(((long long) tv.tv_sec * 1000000 + tv.tv_usec) ^ getpid()); crc64_init(); uint8_t hashseed[16]; -- cgit v1.2.1 From ee59dc1b5cdcc631438aba3904d6d3b520354c80 Mon Sep 17 00:00:00 2001 From: Yang Bodong Date: Wed, 23 Dec 2020 22:28:17 +0800 Subject: Tests: fix the problem that Darwin memory leak detection may fail (#8213) Apparently the "leaks" took reports a different error string about process that's not found in each version of MacOS. This cause the test suite to fail on some OS versions, since some tests terminate the process before looking for leaks. Instead of looking at the error string, we now look at the (documented) exit code. --- tests/support/server.tcl | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 1cddb7068..77ba31d84 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -50,11 +50,17 @@ proc kill_server config { tags {"leaks"} { test "Check for memory leaks (pid $pid)" { set output {0 leaks} - catch {exec leaks $pid} output - if {[string match {*process does not exist*} $output] || - [string match {*cannot examine*} $output]} { - # In a few tests we kill the server process. - set output "0 leaks" + catch {exec leaks $pid} output option + # In a few tests we kill the server process, so leaks will not find it. + # It'll exits with exit code >1 on error, so we ignore these. + if {[dict exists $option -errorcode]} { + set details [dict get $option -errorcode] + if {[lindex $details 0] eq "CHILDSTATUS"} { + set status [lindex $details 2] + if {$status > 1} { + set output "0 leaks" + } + } } set output } {*0 leaks*} -- cgit v1.2.1 From 58e9c261152819bfa5fe8d37ea943340c3575421 Mon Sep 17 00:00:00 2001 From: sundb Date: Thu, 24 Dec 2020 00:37:33 +0800 Subject: Fix redundancy incrRefCount in lmoveGenericCommand (#8218) --- src/t_list.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/t_list.c b/src/t_list.c index 106f960f6..1b4062c38 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -729,10 +729,6 @@ void lmoveGenericCommand(client *c, int wherefrom, int whereto) { if (checkType(c,dobj,OBJ_LIST)) return; value = listTypePop(sobj,wherefrom); serverAssert(value); /* assertion for valgrind (avoid NPD) */ - /* We saved touched key, and protect it, since lmoveHandlePush - * may change the client command argument vector (it does not - * currently). */ - incrRefCount(touchedkey); lmoveHandlePush(c,c->argv[2],dobj,value,whereto); /* listTypePop returns an object with its refcount incremented */ @@ -749,7 +745,6 @@ void lmoveGenericCommand(client *c, int wherefrom, int whereto) { touchedkey,c->db->id); } signalModifiedKey(c,c->db,touchedkey); - decrRefCount(touchedkey); server.dirty++; if (c->cmd->proc == blmoveCommand) { rewriteClientCommandVector(c,5,shared.lmove, -- cgit v1.2.1 From 55abd1c6d078e0d30ee01a819f069fa7737c3dc2 Mon Sep 17 00:00:00 2001 From: Wang Yuan Date: Thu, 24 Dec 2020 01:16:49 +0800 Subject: Add semicolon to calls of test_cond() (#8238) --- src/sds.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/sds.c b/src/sds.c index 6a85eb4aa..5efb36de6 100644 --- a/src/sds.c +++ b/src/sds.c @@ -1159,8 +1159,8 @@ void sds_free(void *ptr) { s_free(ptr); } #ifdef REDIS_TEST #include +#include #include "testhelp.h" -#include "limits.h" #define UNUSED(x) (void)(x) int sdsTest(int argc, char **argv) { @@ -1171,12 +1171,12 @@ int sdsTest(int argc, char **argv) { sds x = sdsnew("foo"), y; test_cond("Create a string and obtain the length", - sdslen(x) == 3 && memcmp(x,"foo\0",4) == 0) + sdslen(x) == 3 && memcmp(x,"foo\0",4) == 0); sdsfree(x); x = sdsnewlen("foo",2); test_cond("Create a string with specified length", - sdslen(x) == 2 && memcmp(x,"fo\0",3) == 0) + sdslen(x) == 2 && memcmp(x,"fo\0",3) == 0); x = sdscat(x,"bar"); test_cond("Strings concatenation", @@ -1184,22 +1184,22 @@ int sdsTest(int argc, char **argv) { x = sdscpy(x,"a"); test_cond("sdscpy() against an originally longer string", - sdslen(x) == 1 && memcmp(x,"a\0",2) == 0) + sdslen(x) == 1 && memcmp(x,"a\0",2) == 0); x = sdscpy(x,"xyzxxxxxxxxxxyyyyyyyyyykkkkkkkkkk"); test_cond("sdscpy() against an originally shorter string", sdslen(x) == 33 && - memcmp(x,"xyzxxxxxxxxxxyyyyyyyyyykkkkkkkkkk\0",33) == 0) + memcmp(x,"xyzxxxxxxxxxxyyyyyyyyyykkkkkkkkkk\0",33) == 0); sdsfree(x); x = sdscatprintf(sdsempty(),"%d",123); test_cond("sdscatprintf() seems working in the base case", - sdslen(x) == 3 && memcmp(x,"123\0",4) == 0) + sdslen(x) == 3 && memcmp(x,"123\0",4) == 0); sdsfree(x); x = sdscatprintf(sdsempty(),"a%cb",0); test_cond("sdscatprintf() seems working with \\0 inside of result", - sdslen(x) == 3 && memcmp(x,"a\0""b\0",4) == 0) + sdslen(x) == 3 && memcmp(x,"a\0""b\0",4) == 0); { sdsfree(x); @@ -1209,7 +1209,7 @@ int sdsTest(int argc, char **argv) { } x = sdscatprintf(sdsempty(),"%0*d",(int)sizeof(etalon),0); test_cond("sdscatprintf() can print 1MB", - sdslen(x) == sizeof(etalon) && memcmp(x,etalon,sizeof(etalon)) == 0) + sdslen(x) == sizeof(etalon) && memcmp(x,etalon,sizeof(etalon)) == 0); } sdsfree(x); @@ -1218,7 +1218,7 @@ int sdsTest(int argc, char **argv) { test_cond("sdscatfmt() seems working in the base case", sdslen(x) == 60 && memcmp(x,"--Hello Hi! World -9223372036854775808," - "9223372036854775807--",60) == 0) + "9223372036854775807--",60) == 0); printf("[%s]\n",x); sdsfree(x); @@ -1226,85 +1226,85 @@ int sdsTest(int argc, char **argv) { x = sdscatfmt(x, "%u,%U--", UINT_MAX, ULLONG_MAX); test_cond("sdscatfmt() seems working with unsigned numbers", sdslen(x) == 35 && - memcmp(x,"--4294967295,18446744073709551615--",35) == 0) + memcmp(x,"--4294967295,18446744073709551615--",35) == 0); sdsfree(x); x = sdsnew(" x "); sdstrim(x," x"); test_cond("sdstrim() works when all chars match", - sdslen(x) == 0) + sdslen(x) == 0); sdsfree(x); x = sdsnew(" x "); sdstrim(x," "); test_cond("sdstrim() works when a single char remains", - sdslen(x) == 1 && x[0] == 'x') + sdslen(x) == 1 && x[0] == 'x'); sdsfree(x); x = sdsnew("xxciaoyyy"); sdstrim(x,"xy"); test_cond("sdstrim() correctly trims characters", - sdslen(x) == 4 && memcmp(x,"ciao\0",5) == 0) + sdslen(x) == 4 && memcmp(x,"ciao\0",5) == 0); y = sdsdup(x); sdsrange(y,1,1); test_cond("sdsrange(...,1,1)", - sdslen(y) == 1 && memcmp(y,"i\0",2) == 0) + sdslen(y) == 1 && memcmp(y,"i\0",2) == 0); sdsfree(y); y = sdsdup(x); sdsrange(y,1,-1); test_cond("sdsrange(...,1,-1)", - sdslen(y) == 3 && memcmp(y,"iao\0",4) == 0) + sdslen(y) == 3 && memcmp(y,"iao\0",4) == 0); sdsfree(y); y = sdsdup(x); sdsrange(y,-2,-1); test_cond("sdsrange(...,-2,-1)", - sdslen(y) == 2 && memcmp(y,"ao\0",3) == 0) + sdslen(y) == 2 && memcmp(y,"ao\0",3) == 0); sdsfree(y); y = sdsdup(x); sdsrange(y,2,1); test_cond("sdsrange(...,2,1)", - sdslen(y) == 0 && memcmp(y,"\0",1) == 0) + sdslen(y) == 0 && memcmp(y,"\0",1) == 0); sdsfree(y); y = sdsdup(x); sdsrange(y,1,100); test_cond("sdsrange(...,1,100)", - sdslen(y) == 3 && memcmp(y,"iao\0",4) == 0) + sdslen(y) == 3 && memcmp(y,"iao\0",4) == 0); sdsfree(y); y = sdsdup(x); sdsrange(y,100,100); test_cond("sdsrange(...,100,100)", - sdslen(y) == 0 && memcmp(y,"\0",1) == 0) + sdslen(y) == 0 && memcmp(y,"\0",1) == 0); sdsfree(y); sdsfree(x); x = sdsnew("foo"); y = sdsnew("foa"); - test_cond("sdscmp(foo,foa)", sdscmp(x,y) > 0) + test_cond("sdscmp(foo,foa)", sdscmp(x,y) > 0); sdsfree(y); sdsfree(x); x = sdsnew("bar"); y = sdsnew("bar"); - test_cond("sdscmp(bar,bar)", sdscmp(x,y) == 0) + test_cond("sdscmp(bar,bar)", sdscmp(x,y) == 0); sdsfree(y); sdsfree(x); x = sdsnew("aar"); y = sdsnew("bar"); - test_cond("sdscmp(bar,bar)", sdscmp(x,y) < 0) + test_cond("sdscmp(bar,bar)", sdscmp(x,y) < 0); sdsfree(y); sdsfree(x); x = sdsnewlen("\a\n\0foo\r",7); y = sdscatrepr(sdsempty(),x,sdslen(x)); test_cond("sdscatrepr(...data...)", - memcmp(y,"\"\\a\\n\\x00foo\\r\"",15) == 0) + memcmp(y,"\"\\a\\n\\x00foo\\r\"",15) == 0); { unsigned int oldfree; @@ -1343,7 +1343,7 @@ int sdsTest(int argc, char **argv) { sdsfree(x); } } - test_report() + test_report(); return 0; } #endif -- cgit v1.2.1 From efaf09ee4b6437c69c467acdb0c62a510207e993 Mon Sep 17 00:00:00 2001 From: Madelyn Olson <34459052+madolson@users.noreply.github.com> Date: Wed, 23 Dec 2020 19:06:25 -0800 Subject: Flow through the error handling path for most errors (#8226) Properly throw errors for invalid replication stream and support https://github.com/redis/redis/pull/8217 --- src/acl.c | 2 +- src/bitops.c | 8 ++++---- src/blocked.c | 4 ++-- src/cluster.c | 31 +++++++++++++++---------------- src/db.c | 14 +++++++------- src/debug.c | 8 ++++---- src/geo.c | 6 +++--- src/hyperloglog.c | 21 ++++++++++----------- src/networking.c | 16 ++++++++-------- src/object.c | 6 +++--- src/rdb.c | 6 +++--- src/replication.c | 4 ++-- src/scripting.c | 8 ++++---- src/sentinel.c | 2 +- src/server.c | 13 ++++++++----- src/sort.c | 4 ++-- src/t_list.c | 8 ++++---- src/t_set.c | 4 ++-- src/t_stream.c | 15 +++++++-------- src/t_string.c | 6 +++--- src/t_zset.c | 20 ++++++++++---------- 21 files changed, 103 insertions(+), 103 deletions(-) diff --git a/src/acl.c b/src/acl.c index a1a7c4237..606b61cb7 100644 --- a/src/acl.c +++ b/src/acl.c @@ -2224,7 +2224,7 @@ void addReplyCommandCategories(client *c, struct redisCommand *cmd) { void authCommand(client *c) { /* Only two or three argument forms are allowed. */ if (c->argc > 3) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } diff --git a/src/bitops.c b/src/bitops.c index 5e996679b..afd79ad88 100644 --- a/src/bitops.c +++ b/src/bitops.c @@ -611,7 +611,7 @@ void bitopCommand(client *c) { else if((opname[0] == 'n' || opname[0] == 'N') && !strcasecmp(opname,"not")) op = BITOP_NOT; else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -813,7 +813,7 @@ void bitcountCommand(client *c) { end = strlen-1; } else { /* Syntax error. */ - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -878,7 +878,7 @@ void bitposCommand(client *c) { end = strlen-1; } else { /* Syntax error. */ - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -970,7 +970,7 @@ void bitfieldGeneric(client *c, int flags) { } continue; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); zfree(ops); return; } diff --git a/src/blocked.c b/src/blocked.c index d85723458..e3f7b74d6 100644 --- a/src/blocked.c +++ b/src/blocked.c @@ -200,9 +200,9 @@ void disconnectAllBlockedClients(void) { client *c = listNodeValue(ln); if (c->flags & CLIENT_BLOCKED) { - addReplySds(c,sdsnew( + addReplyError(c, "-UNBLOCKED force unblock from blocking operation, " - "instance state changed (master -> replica?)\r\n")); + "instance state changed (master -> replica?)"); unblockClient(c); c->flags |= CLIENT_CLOSE_AFTER_REPLY; } diff --git a/src/cluster.c b/src/cluster.c index 2cd6b2521..29bdb2109 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -4820,7 +4820,7 @@ NULL takeover = 1; force = 1; /* Takeover also implies force. */ } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -4911,7 +4911,7 @@ NULL } else if (!strcasecmp(c->argv[2]->ptr,"soft")) { hard = 0; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -5049,7 +5049,7 @@ void restoreCommand(client *c) { } j++; /* Consume additional arg. */ } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -5057,7 +5057,7 @@ void restoreCommand(client *c) { /* Make sure this key does not already exist here... */ robj *key = c->argv[1]; if (!replace && lookupKeyWrite(c->db,key) != NULL) { - addReply(c,shared.busykeyerr); + addReplyErrorObject(c,shared.busykeyerr); return; } @@ -5170,8 +5170,7 @@ migrateCachedSocket* migrateGetSocket(client *c, robj *host, robj *port, long ti conn = server.tls_cluster ? connCreateTLS() : connCreateSocket(); if (connBlockingConnect(conn, c->argv[1]->ptr, atoi(c->argv[2]->ptr), timeout) != C_OK) { - addReplySds(c, - sdsnew("-IOERR error or timeout connecting to the client\r\n")); + addReplyError(c,"-IOERR error or timeout connecting to the client"); connClose(conn); sdsfree(name); return NULL; @@ -5259,14 +5258,14 @@ void migrateCommand(client *c) { replace = 1; } else if (!strcasecmp(c->argv[j]->ptr,"auth")) { if (!moreargs) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } j++; password = c->argv[j]->ptr; } else if (!strcasecmp(c->argv[j]->ptr,"auth2")) { if (moreargs < 2) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } username = c->argv[++j]->ptr; @@ -5282,7 +5281,7 @@ void migrateCommand(client *c) { num_keys = c->argc - j - 1; break; /* All the remaining args are keys. */ } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -5837,23 +5836,23 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in * be set to the hash slot that caused the redirection. */ void clusterRedirectClient(client *c, clusterNode *n, int hashslot, int error_code) { if (error_code == CLUSTER_REDIR_CROSS_SLOT) { - addReplySds(c,sdsnew("-CROSSSLOT Keys in request don't hash to the same slot\r\n")); + addReplyError(c,"-CROSSSLOT Keys in request don't hash to the same slot"); } else if (error_code == CLUSTER_REDIR_UNSTABLE) { /* The request spawns multiple keys in the same slot, * but the slot is not "stable" currently as there is * a migration or import in progress. */ - addReplySds(c,sdsnew("-TRYAGAIN Multiple keys request during rehashing of slot\r\n")); + addReplyError(c,"-TRYAGAIN Multiple keys request during rehashing of slot"); } else if (error_code == CLUSTER_REDIR_DOWN_STATE) { - addReplySds(c,sdsnew("-CLUSTERDOWN The cluster is down\r\n")); + addReplyError(c,"-CLUSTERDOWN The cluster is down"); } else if (error_code == CLUSTER_REDIR_DOWN_RO_STATE) { - addReplySds(c,sdsnew("-CLUSTERDOWN The cluster is down and only accepts read commands\r\n")); + addReplyError(c,"-CLUSTERDOWN The cluster is down and only accepts read commands"); } else if (error_code == CLUSTER_REDIR_DOWN_UNBOUND) { - addReplySds(c,sdsnew("-CLUSTERDOWN Hash slot not served\r\n")); + addReplyError(c,"-CLUSTERDOWN Hash slot not served"); } else if (error_code == CLUSTER_REDIR_MOVED || error_code == CLUSTER_REDIR_ASK) { - addReplySds(c,sdscatprintf(sdsempty(), - "-%s %d %s:%d\r\n", + addReplyErrorSds(c,sdscatprintf(sdsempty(), + "-%s %d %s:%d", (error_code == CLUSTER_REDIR_ASK) ? "ASK" : "MOVED", hashslot,n->ip,n->port)); } else { diff --git a/src/db.c b/src/db.c index 4f27534fc..d5e8b07d3 100644 --- a/src/db.c +++ b/src/db.c @@ -593,7 +593,7 @@ int getFlushCommandFlags(client *c, int *flags) { /* Parse the optional ASYNC option. */ if (c->argc > 1) { if (c->argc > 2 || strcasecmp(c->argv[1]->ptr,"async")) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return C_ERR; } *flags = EMPTYDB_ASYNC; @@ -842,7 +842,7 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { } if (count < 1) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); goto cleanup; } @@ -861,7 +861,7 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { typename = c->argv[i+1]->ptr; i+= 2; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); goto cleanup; } } @@ -1050,7 +1050,7 @@ void shutdownCommand(client *c) { int flags = 0; if (c->argc > 2) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 2) { if (!strcasecmp(c->argv[1]->ptr,"nosave")) { @@ -1058,7 +1058,7 @@ void shutdownCommand(client *c) { } else if (!strcasecmp(c->argv[1]->ptr,"save")) { flags |= SHUTDOWN_SAVE; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -1144,7 +1144,7 @@ void moveCommand(client *c) { /* If the user is moving using as target the same * DB as the source DB it is probably an error. */ if (src == dst) { - addReply(c,shared.sameobjecterr); + addReplyErrorObject(c,shared.sameobjecterr); return; } @@ -1224,7 +1224,7 @@ void copyCommand(client *c) { robj *key = c->argv[1]; robj *newkey = c->argv[2]; if (src == dst && (sdscmp(key->ptr, newkey->ptr) == 0)) { - addReply(c,shared.sameobjecterr); + addReplyErrorObject(c,shared.sameobjecterr); return; } diff --git a/src/debug.c b/src/debug.c index 26c1a91fc..f95aaba0d 100644 --- a/src/debug.c +++ b/src/debug.c @@ -474,7 +474,7 @@ NULL rdbSaveInfo rsi, *rsiptr; rsiptr = rdbPopulateSaveInfo(&rsi); if (rdbSave(server.rdb_filename,rsiptr) != C_OK) { - addReply(c,shared.err); + addReplyErrorObject(c,shared.err); return; } } @@ -500,7 +500,7 @@ NULL int ret = loadAppendOnlyFile(server.aof_filename); unprotectClient(c); if (ret != C_OK) { - addReply(c,shared.err); + addReplyErrorObject(c,shared.err); return; } server.dirty = 0; /* Prevent AOF / replication */ @@ -512,7 +512,7 @@ NULL char *strenc; if ((de = dictFind(c->db->dict,c->argv[2]->ptr)) == NULL) { - addReply(c,shared.nokeyerr); + addReplyErrorObject(c,shared.nokeyerr); return; } val = dictGetVal(de); @@ -564,7 +564,7 @@ NULL sds key; if ((de = dictFind(c->db->dict,c->argv[2]->ptr)) == NULL) { - addReply(c,shared.nokeyerr); + addReplyErrorObject(c,shared.nokeyerr); return; } val = dictGetVal(de); diff --git a/src/geo.c b/src/geo.c index 925c090ed..75a88398e 100644 --- a/src/geo.c +++ b/src/geo.c @@ -100,8 +100,8 @@ int extractLongLatOrReply(client *c, robj **argv, double *xy) { } if (xy[0] < GEO_LONG_MIN || xy[0] > GEO_LONG_MAX || xy[1] < GEO_LAT_MIN || xy[1] > GEO_LAT_MAX) { - addReplySds(c, sdscatprintf(sdsempty(), - "-ERR invalid longitude,latitude pair %f,%f\r\n",xy[0],xy[1])); + addReplyErrorFormat(c, + "-ERR invalid longitude,latitude pair %f,%f\r\n",xy[0],xy[1]); return C_ERR; } return C_OK; @@ -902,7 +902,7 @@ void geodistCommand(client *c) { to_meter = extractUnitOrReply(c,c->argv[4]); if (to_meter < 0) return; } else if (c->argc > 5) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } diff --git a/src/hyperloglog.c b/src/hyperloglog.c index 648c26a02..75a04227c 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -205,7 +205,7 @@ struct hllhdr { #define HLL_RAW 255 /* Only used internally, never exposed. */ #define HLL_MAX_ENCODING 1 -static char *invalid_hll_err = "-INVALIDOBJ Corrupted HLL object detected\r\n"; +static char *invalid_hll_err = "-INVALIDOBJ Corrupted HLL object detected"; /* =========================== Low level bit macros ========================= */ @@ -1171,9 +1171,8 @@ int isHLLObjectOrReply(client *c, robj *o) { return C_OK; invalid: - addReplySds(c, - sdsnew("-WRONGTYPE Key is not a valid " - "HyperLogLog string value.\r\n")); + addReplyError(c,"-WRONGTYPE Key is not a valid " + "HyperLogLog string value."); return C_ERR; } @@ -1203,7 +1202,7 @@ void pfaddCommand(client *c) { updated++; break; case -1: - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } } @@ -1245,7 +1244,7 @@ void pfcountCommand(client *c) { /* Merge with this HLL with our 'max' HLL by setting max[i] * to MAX(max[i],hll[i]). */ if (hllMerge(registers,o) == C_ERR) { - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } } @@ -1285,7 +1284,7 @@ void pfcountCommand(client *c) { /* Recompute it and update the cached value. */ card = hllCount(hdr,&invalid); if (invalid) { - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } hdr->card[0] = card & 0xff; @@ -1332,7 +1331,7 @@ void pfmergeCommand(client *c) { /* Merge with this HLL with our 'max' HLL by setting max[i] * to MAX(max[i],hll[i]). */ if (hllMerge(max,o) == C_ERR) { - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } } @@ -1355,7 +1354,7 @@ void pfmergeCommand(client *c) { /* Convert the destination object to dense representation if at least * one of the inputs was dense. */ if (use_dense && hllSparseToDense(o) == C_ERR) { - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } @@ -1512,7 +1511,7 @@ void pfdebugCommand(client *c) { if (hdr->encoding == HLL_SPARSE) { if (hllSparseToDense(o) == C_ERR) { - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } server.dirty++; /* Force propagation on encoding change. */ @@ -1577,7 +1576,7 @@ void pfdebugCommand(client *c) { if (hdr->encoding == HLL_SPARSE) { if (hllSparseToDense(o) == C_ERR) { - addReplySds(c,sdsnew(invalid_hll_err)); + addReplyError(c,invalid_hll_err); return; } conv = 1; diff --git a/src/networking.c b/src/networking.c index 45c504e46..8fee298c6 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2427,7 +2427,7 @@ NULL } } } else if (c->argc != 2) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -2446,7 +2446,7 @@ NULL if (!(c->flags & CLIENT_REPLY_OFF)) c->flags |= CLIENT_REPLY_SKIP_NEXT; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } else if (!strcasecmp(c->argv[1]->ptr,"kill")) { @@ -2502,17 +2502,17 @@ NULL } else if (!strcasecmp(c->argv[i+1]->ptr,"no")) { skipme = 0; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } i += 2; } } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -2649,7 +2649,7 @@ NULL prefix[numprefix++] = c->argv[j]; } else { zfree(prefix); - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -2711,7 +2711,7 @@ NULL disableTracking(c); } else { zfree(prefix); - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } zfree(prefix); @@ -2740,7 +2740,7 @@ NULL return; } } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } diff --git a/src/object.c b/src/object.c index 71eceb6d6..583d13737 100644 --- a/src/object.c +++ b/src/object.c @@ -404,7 +404,7 @@ robj *resetRefCount(robj *obj) { int checkType(client *c, robj *o, int type) { /* A NULL is considered an empty key */ if (o && o->type != type) { - addReply(c,shared.wrongtypeerr); + addReplyErrorObject(c,shared.wrongtypeerr); return 1; } return 0; @@ -1321,13 +1321,13 @@ NULL if (getLongLongFromObjectOrReply(c,c->argv[j+1],&samples,NULL) == C_ERR) return; if (samples < 0) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } if (samples == 0) samples = LLONG_MAX; j++; /* skip option argument. */ } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } diff --git a/src/rdb.c b/src/rdb.c index 58e7d2cff..abcfeadd7 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2852,7 +2852,7 @@ void saveCommand(client *c) { if (rdbSave(server.rdb_filename,rsiptr) == C_OK) { addReply(c,shared.ok); } else { - addReply(c,shared.err); + addReplyErrorObject(c,shared.err); } } @@ -2866,7 +2866,7 @@ void bgsaveCommand(client *c) { if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"schedule")) { schedule = 1; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -2889,7 +2889,7 @@ void bgsaveCommand(client *c) { } else if (rdbSaveBackground(server.rdb_filename,rsiptr) == C_OK) { addReplyStatus(c,"Background saving started"); } else { - addReply(c,shared.err); + addReplyErrorObject(c,shared.err); } } diff --git a/src/replication.c b/src/replication.c index 34ff44087..92d7294f5 100644 --- a/src/replication.c +++ b/src/replication.c @@ -715,7 +715,7 @@ void syncCommand(client *c) { /* Refuse SYNC requests if we are a slave but the link with our master * is not ok... */ if (server.masterhost && server.repl_state != REPL_STATE_CONNECTED) { - addReplySds(c,sdsnew("-NOMASTERLINK Can't SYNC while not connected with my master\r\n")); + addReplyError(c,"-NOMASTERLINK Can't SYNC while not connected with my master"); return; } @@ -866,7 +866,7 @@ void replconfCommand(client *c) { if ((c->argc % 2) == 0) { /* Number of arguments must be odd to make sure that every * option has a corresponding value. */ - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } diff --git a/src/scripting.c b/src/scripting.c index bc137de25..2ac8268ed 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -368,7 +368,7 @@ void luaReplyToRedisReply(client *c, lua_State *lua) { if (t == LUA_TSTRING) { sds err = sdsnew(lua_tostring(lua,-1)); sdsmapchars(err,"\r\n"," ",2); - addReplySds(c,sdscatprintf(sdsempty(),"-%s\r\n",err)); + addReplyErrorSds(c,sdscatprintf(sdsempty(),"-%s",err)); sdsfree(err); lua_pop(lua,2); return; @@ -1740,11 +1740,11 @@ NULL forceCommandPropagation(c,PROPAGATE_REPL|PROPAGATE_AOF); } else if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"kill")) { if (server.lua_caller == NULL) { - addReplySds(c,sdsnew("-NOTBUSY No scripts in execution right now.\r\n")); + addReplyError(c,"-NOTBUSY No scripts in execution right now."); } else if (server.lua_caller->flags & CLIENT_MASTER) { - addReplySds(c,sdsnew("-UNKILLABLE The busy script was sent by a master instance in the context of replication and cannot be killed.\r\n")); + addReplyError(c,"-UNKILLABLE The busy script was sent by a master instance in the context of replication and cannot be killed."); } else if (server.lua_write_dirty) { - addReplySds(c,sdsnew("-UNKILLABLE Sorry the script already executed write commands against the dataset. You can either wait the script termination or kill the server in a hard way using the SHUTDOWN NOSAVE command.\r\n")); + addReplyError(c,"-UNKILLABLE Sorry the script already executed write commands against the dataset. You can either wait the script termination or kill the server in a hard way using the SHUTDOWN NOSAVE command."); } else { server.lua_kill = 1; addReply(c,shared.ok); diff --git a/src/sentinel.c b/src/sentinel.c index 7693ac5ae..9764f9004 100644 --- a/src/sentinel.c +++ b/src/sentinel.c @@ -3446,7 +3446,7 @@ numargserr: /* SENTINEL INFO [section] */ void sentinelInfoCommand(client *c) { if (c->argc > 2) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } diff --git a/src/server.c b/src/server.c index 611935a6c..f28c6e114 100644 --- a/src/server.c +++ b/src/server.c @@ -2380,9 +2380,9 @@ void afterSleep(struct aeEventLoop *eventLoop) { void createSharedObjects(void) { int j; + /* Shared command responses */ shared.crlf = createObject(OBJ_STRING,sdsnew("\r\n")); shared.ok = createObject(OBJ_STRING,sdsnew("+OK\r\n")); - shared.err = createObject(OBJ_STRING,sdsnew("-ERR\r\n")); shared.emptybulk = createObject(OBJ_STRING,sdsnew("$0\r\n\r\n")); shared.czero = createObject(OBJ_STRING,sdsnew(":0\r\n")); shared.cone = createObject(OBJ_STRING,sdsnew(":1\r\n")); @@ -2390,8 +2390,14 @@ void createSharedObjects(void) { shared.pong = createObject(OBJ_STRING,sdsnew("+PONG\r\n")); shared.queued = createObject(OBJ_STRING,sdsnew("+QUEUED\r\n")); shared.emptyscan = createObject(OBJ_STRING,sdsnew("*2\r\n$1\r\n0\r\n*0\r\n")); + shared.space = createObject(OBJ_STRING,sdsnew(" ")); + shared.colon = createObject(OBJ_STRING,sdsnew(":")); + shared.plus = createObject(OBJ_STRING,sdsnew("+")); + + /* Shared command error responses */ shared.wrongtypeerr = createObject(OBJ_STRING,sdsnew( "-WRONGTYPE Operation against a key holding the wrong kind of value\r\n")); + shared.err = createObject(OBJ_STRING,sdsnew("-ERR\r\n")); shared.nokeyerr = createObject(OBJ_STRING,sdsnew( "-ERR no such key\r\n")); shared.syntaxerr = createObject(OBJ_STRING,sdsnew( @@ -2422,9 +2428,6 @@ void createSharedObjects(void) { "-NOREPLICAS Not enough good replicas to write.\r\n")); shared.busykeyerr = createObject(OBJ_STRING,sdsnew( "-BUSYKEY Target key name already exists.\r\n")); - shared.space = createObject(OBJ_STRING,sdsnew(" ")); - shared.colon = createObject(OBJ_STRING,sdsnew(":")); - shared.plus = createObject(OBJ_STRING,sdsnew("+")); /* The shared NULL depends on the protocol version. */ shared.null[0] = NULL; @@ -4956,7 +4959,7 @@ void infoCommand(client *c) { char *section = c->argc == 2 ? c->argv[1]->ptr : "default"; if (c->argc > 2) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } sds info = genRedisInfoString(section); diff --git a/src/sort.c b/src/sort.c index aeef53e6a..44637720b 100644 --- a/src/sort.c +++ b/src/sort.c @@ -256,7 +256,7 @@ void sortCommand(client *c) { getop++; j++; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); syntax_error++; break; } @@ -279,7 +279,7 @@ void sortCommand(client *c) { sortval->type != OBJ_ZSET) { listRelease(operations); - addReply(c,shared.wrongtypeerr); + addReplyErrorObject(c,shared.wrongtypeerr); return; } diff --git a/src/t_list.c b/src/t_list.c index 1b4062c38..c8323c612 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -294,7 +294,7 @@ void linsertCommand(client *c) { } else if (strcasecmp(c->argv[2]->ptr,"before") == 0) { where = LIST_HEAD; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -373,7 +373,7 @@ void lsetCommand(client *c) { int replaced = quicklistReplaceAtIndex(ql, index, value->ptr, sdslen(value->ptr)); if (!replaced) { - addReply(c,shared.outofrangeerr); + addReplyErrorObject(c,shared.outofrangeerr); } else { addReply(c,shared.ok); signalModifiedKey(c,c->db,c->argv[1]); @@ -566,7 +566,7 @@ void lposCommand(client *c) { return; } } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -698,7 +698,7 @@ int getListPositionFromObjectOrReply(client *c, robj *arg, int *position) { } else if (strcasecmp(arg->ptr,"left") == 0) { *position = LIST_HEAD; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return C_ERR; } return C_OK; diff --git a/src/t_set.c b/src/t_set.c index fd9f4442a..64bbbd3a0 100644 --- a/src/t_set.c +++ b/src/t_set.c @@ -605,7 +605,7 @@ void spopCommand(client *c) { spopWithCountCommand(c); return; } else if (c->argc > 3) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -801,7 +801,7 @@ void srandmemberCommand(client *c) { srandmemberWithCountCommand(c); return; } else if (c->argc > 3) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } diff --git a/src/t_stream.c b/src/t_stream.c index d61fb3eab..3ed9618fc 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -1534,7 +1534,7 @@ void xrangeGenericCommand(client *c, int rev) { if (count < 0) count = 0; j++; /* Consume additional arg. */ } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -1643,14 +1643,14 @@ void xreadCommand(client *c) { } noack = 1; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } /* STREAMS option is mandatory. */ if (streams_arg == 0) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -2076,8 +2076,7 @@ NULL notifyKeyspaceEvent(NOTIFY_STREAM,"xgroup-create", c->argv[2],c->db->id); } else { - addReplySds(c, - sdsnew("-BUSYGROUP Consumer Group name already exists\r\n")); + addReplyError(c,"-BUSYGROUP Consumer Group name already exists"); } } else if (!strcasecmp(opt,"SETID") && c->argc == 5) { streamID id; @@ -2237,7 +2236,7 @@ void xpendingCommand(client *c) { /* Start and stop, and the consumer, can be omitted. Also the IDLE modifier. */ if (c->argc != 3 && (c->argc < 6 || c->argc > 9)) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -2251,7 +2250,7 @@ void xpendingCommand(client *c) { return; if (c->argc < 8) { /* If IDLE was provided we must have at least 'start end count' */ - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } /* Search for rest of arguments after 'IDLE ' */ @@ -2758,7 +2757,7 @@ void xtrimCommand(client *c) { i++; maxlen_arg_idx = i; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } diff --git a/src/t_string.c b/src/t_string.c index 3ecc473bd..2792f5557 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -153,7 +153,7 @@ void setCommand(client *c) { expire = next; j++; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -528,7 +528,7 @@ void stralgoCommand(client *c) { if (!strcasecmp(c->argv[1]->ptr,"lcs")) { stralgoLCS(c); } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); } } @@ -589,7 +589,7 @@ void stralgoLCS(client *c) { b = objb->ptr; j += 2; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); goto cleanup; } } diff --git a/src/t_zset.c b/src/t_zset.c index a9564828a..25c7dda4c 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -1701,7 +1701,7 @@ void zaddGenericCommand(client *c, int flags) { * we expect any number of score-element pairs. */ elements = c->argc-scoreidx; if (elements % 2 || !elements) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } elements /= 2; /* Now this holds the number of score-element pairs. */ @@ -2525,7 +2525,7 @@ void zunionInterDiffGenericCommand(client *c, robj *dstkey, int numkeysIndex, in /* test if the expected number of keys would overflow */ if (setnum > (c->argc-(numkeysIndex+1))) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -2536,7 +2536,7 @@ void zunionInterDiffGenericCommand(client *c, robj *dstkey, int numkeysIndex, in if (obj != NULL) { if (obj->type != OBJ_ZSET && obj->type != OBJ_SET) { zfree(src); - addReply(c,shared.wrongtypeerr); + addReplyErrorObject(c,shared.wrongtypeerr); return; } @@ -2582,7 +2582,7 @@ void zunionInterDiffGenericCommand(client *c, robj *dstkey, int numkeysIndex, in aggregate = REDIS_AGGR_MAX; } else { zfree(src); - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } j++; remaining--; @@ -2594,7 +2594,7 @@ void zunionInterDiffGenericCommand(client *c, robj *dstkey, int numkeysIndex, in withscores = 1; } else { zfree(src); - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -2793,7 +2793,7 @@ void zrangeGenericCommand(client *c, int reverse) { if (c->argc == 5 && !strcasecmp(c->argv[4]->ptr,"withscores")) { withscores = 1; } else if (c->argc >= 5) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } @@ -2938,7 +2938,7 @@ void genericZrangebyscoreCommand(client *c, int reverse) { } pos += 3; remaining -= 3; } else { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -3289,7 +3289,7 @@ void genericZrangebylexCommand(client *c, int reverse) { pos += 3; remaining -= 3; } else { zslFreeLexRange(&range); - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } @@ -3632,7 +3632,7 @@ void genericZpopCommand(client *c, robj **keyv, int keyc, int where, int emitkey /* ZPOPMIN key [] */ void zpopminCommand(client *c) { if (c->argc > 3) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } genericZpopCommand(c,&c->argv[1],1,ZSET_MIN,0, @@ -3642,7 +3642,7 @@ void zpopminCommand(client *c) { /* ZMAXPOP key [] */ void zpopmaxCommand(client *c) { if (c->argc > 3) { - addReply(c,shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } genericZpopCommand(c,&c->argv[1],1,ZSET_MAX,0, -- cgit v1.2.1 From 59ff42c42156dcf4a5d9b637d8cb20c37ddc5c65 Mon Sep 17 00:00:00 2001 From: Madelyn Olson <34459052+madolson@users.noreply.github.com> Date: Wed, 23 Dec 2020 19:13:12 -0800 Subject: Cleanup key tracking documentation and table management (#8039) Cleanup key tracking documentation, always cleanup the tracking table, and free the tracking table in an async manner when applicable. --- redis.conf | 2 +- src/aof.c | 4 +-- src/bio.c | 61 ++++++++++++++++++++++++----------------- src/bio.h | 6 ++++- src/db.c | 6 ++--- src/lazyfree.c | 81 ++++++++++++++++++++++++++++++++++--------------------- src/replication.c | 4 +-- src/server.h | 5 ++-- src/tracking.c | 27 ++++++++++++------- 9 files changed, 121 insertions(+), 75 deletions(-) diff --git a/redis.conf b/redis.conf index af4b4be1f..891bb1663 100644 --- a/redis.conf +++ b/redis.conf @@ -691,7 +691,7 @@ replica-priority 100 # Redis implements server assisted support for client side caching of values. # This is implemented using an invalidation table that remembers, using -# 16 millions of slots, what clients may have certain subsets of keys. In turn +# a radix key indexed by key name, what clients have which keys. In turn # this is used in order to send invalidation messages to clients. Please # check this page to understand more about the feature: # diff --git a/src/aof.c b/src/aof.c index 79b2f1284..2accc781e 100644 --- a/src/aof.c +++ b/src/aof.c @@ -206,7 +206,7 @@ int aofFsyncInProgress(void) { /* Starts a background task that performs fsync() against the specified * file descriptor (the one of the AOF file) in another thread. */ void aof_background_fsync(int fd) { - bioCreateBackgroundJob(BIO_AOF_FSYNC,(void*)(long)fd,NULL,NULL); + bioCreateFsyncJob(fd); } /* Kills an AOFRW child process if exists */ @@ -1909,7 +1909,7 @@ void backgroundRewriteDoneHandler(int exitcode, int bysignal) { server.aof_state = AOF_ON; /* Asynchronously close the overwritten AOF. */ - if (oldfd != -1) bioCreateBackgroundJob(BIO_CLOSE_FILE,(void*)(long)oldfd,NULL,NULL); + if (oldfd != -1) bioCreateCloseJob(oldfd); serverLog(LL_VERBOSE, "Background AOF rewrite signal handler took %lldus", ustime()-now); diff --git a/src/bio.c b/src/bio.c index a11bcb18b..c6e17f49d 100644 --- a/src/bio.c +++ b/src/bio.c @@ -78,15 +78,13 @@ static unsigned long long bio_pending[BIO_NUM_OPS]; * file as the API does not expose the internals at all. */ struct bio_job { time_t time; /* Time at which the job was created. */ - /* Job specific arguments pointers. If we need to pass more than three - * arguments we can just pass a pointer to a structure or alike. */ - void *arg1, *arg2, *arg3; + /* Job specific arguments.*/ + int fd; /* Fd for file based background jobs */ + lazy_free_fn *free_fn; /* Function that will free the provided arguments */ + void *free_args[]; /* List of arguments to be passed to the free function */ }; void *bioProcessBackgroundJobs(void *arg); -void lazyfreeFreeObjectFromBioThread(robj *o); -void lazyfreeFreeDatabaseFromBioThread(dict *ht1, dict *ht2); -void lazyfreeFreeSlotsMapFromBioThread(rax *rt); /* Make sure we have enough stack to perform all the things we do in the * main thread. */ @@ -128,13 +126,8 @@ void bioInit(void) { } } -void bioCreateBackgroundJob(int type, void *arg1, void *arg2, void *arg3) { - struct bio_job *job = zmalloc(sizeof(*job)); - +void bioSubmitJob(int type, struct bio_job *job) { job->time = time(NULL); - job->arg1 = arg1; - job->arg2 = arg2; - job->arg3 = arg3; pthread_mutex_lock(&bio_mutex[type]); listAddNodeTail(bio_jobs[type],job); bio_pending[type]++; @@ -142,6 +135,35 @@ void bioCreateBackgroundJob(int type, void *arg1, void *arg2, void *arg3) { pthread_mutex_unlock(&bio_mutex[type]); } +void bioCreateLazyFreeJob(lazy_free_fn free_fn, int arg_count, ...) { + va_list valist; + /* Allocate memory for the job structure and all required + * arguments */ + struct bio_job *job = zmalloc(sizeof(*job) + sizeof(void *) * (arg_count)); + job->free_fn = free_fn; + + va_start(valist, arg_count); + for (int i = 0; i < arg_count; i++) { + job->free_args[i] = va_arg(valist, void *); + } + va_end(valist); + bioSubmitJob(BIO_LAZY_FREE, job); +} + +void bioCreateCloseJob(int fd) { + struct bio_job *job = zmalloc(sizeof(*job)); + job->fd = fd; + + bioSubmitJob(BIO_CLOSE_FILE, job); +} + +void bioCreateFsyncJob(int fd) { + struct bio_job *job = zmalloc(sizeof(*job)); + job->fd = fd; + + bioSubmitJob(BIO_AOF_FSYNC, job); +} + void *bioProcessBackgroundJobs(void *arg) { struct bio_job *job; unsigned long type = (unsigned long) arg; @@ -196,20 +218,11 @@ void *bioProcessBackgroundJobs(void *arg) { /* Process the job accordingly to its type. */ if (type == BIO_CLOSE_FILE) { - close((long)job->arg1); + close(job->fd); } else if (type == BIO_AOF_FSYNC) { - redis_fsync((long)job->arg1); + redis_fsync(job->fd); } else if (type == BIO_LAZY_FREE) { - /* What we free changes depending on what arguments are set: - * arg1 -> free the object at pointer. - * arg2 & arg3 -> free two dictionaries (a Redis DB). - * only arg3 -> free the radix tree. */ - if (job->arg1) - lazyfreeFreeObjectFromBioThread(job->arg1); - else if (job->arg2 && job->arg3) - lazyfreeFreeDatabaseFromBioThread(job->arg2,job->arg3); - else if (job->arg3) - lazyfreeFreeSlotsMapFromBioThread(job->arg3); + job->free_fn(job->free_args); } else { serverPanic("Wrong job type in bioProcessBackgroundJobs()."); } diff --git a/src/bio.h b/src/bio.h index 6c2155941..1e6e97297 100644 --- a/src/bio.h +++ b/src/bio.h @@ -30,13 +30,17 @@ #ifndef __BIO_H #define __BIO_H +typedef void lazy_free_fn(void *args[]); + /* Exported API */ void bioInit(void); -void bioCreateBackgroundJob(int type, void *arg1, void *arg2, void *arg3); unsigned long long bioPendingJobsOfType(int type); unsigned long long bioWaitStepOfType(int type); time_t bioOlderJobOfType(int type); void bioKillThreads(void); +void bioCreateCloseJob(int fd); +void bioCreateFsyncJob(int fd); +void bioCreateLazyFreeJob(lazy_free_fn free_fn, int arg_count, ...); /* Background job opcodes */ #define BIO_CLOSE_FILE 0 /* Deferred close(2) syscall. */ diff --git a/src/db.c b/src/db.c index d5e8b07d3..64a473bf3 100644 --- a/src/db.c +++ b/src/db.c @@ -433,7 +433,7 @@ long long emptyDb(int dbnum, int flags, void(callback)(void*)) { /* Make sure the WATCHed keys are affected by the FLUSH* commands. * Note that we need to call the function while the keys are still * there. */ - signalFlushedDb(dbnum); + signalFlushedDb(dbnum, async); /* Empty redis database structure. */ removed = emptyDbStructure(server.db, dbnum, async, callback); @@ -572,9 +572,9 @@ void signalModifiedKey(client *c, redisDb *db, robj *key) { trackingInvalidateKey(c,key); } -void signalFlushedDb(int dbid) { +void signalFlushedDb(int dbid, int async) { touchWatchedKeysOnFlush(dbid); - trackingInvalidateKeysOnFlush(dbid); + trackingInvalidateKeysOnFlush(async); } /*----------------------------------------------------------------------------- diff --git a/src/lazyfree.c b/src/lazyfree.c index 125e6a1b0..8b9f0e2dc 100644 --- a/src/lazyfree.c +++ b/src/lazyfree.c @@ -6,6 +6,49 @@ static redisAtomic size_t lazyfree_objects = 0; static redisAtomic size_t lazyfreed_objects = 0; +/* Release objects from the lazyfree thread. It's just decrRefCount() + * updating the count of objects to release. */ +void lazyfreeFreeObject(void *args[]) { + robj *o = (robj *) args[0]; + decrRefCount(o); + atomicDecr(lazyfree_objects,1); + atomicIncr(lazyfreed_objects,1); +} + +/* Release a database from the lazyfree thread. The 'db' pointer is the + * database which was substituted with a fresh one in the main thread + * when the database was logically deleted. */ +void lazyfreeFreeDatabase(void *args[]) { + dict *ht1 = (dict *) args[0]; + dict *ht2 = (dict *) args[1]; + + size_t numkeys = dictSize(ht1); + dictRelease(ht1); + dictRelease(ht2); + atomicDecr(lazyfree_objects,numkeys); + atomicIncr(lazyfreed_objects,numkeys); +} + +/* Release the skiplist mapping Redis Cluster keys to slots in the + * lazyfree thread. */ +void lazyfreeFreeSlotsMap(void *args[]) { + rax *rt = args[0]; + size_t len = rt->numele; + raxFree(rt); + atomicDecr(lazyfree_objects,len); + atomicIncr(lazyfreed_objects,len); +} + +/* Release the rax mapping Redis Cluster keys to slots in the + * lazyfree thread. */ +void lazyFreeTrackingTable(void *args[]) { + rax *rt = args[0]; + size_t len = rt->numele; + raxFree(rt); + atomicDecr(lazyfree_objects,len); + atomicIncr(lazyfreed_objects,len); +} + /* Return the number of currently pending objects to free. */ size_t lazyfreeGetPendingObjectsCount(void) { size_t aux; @@ -120,7 +163,7 @@ int dbAsyncDelete(redisDb *db, robj *key) { * equivalent to just calling decrRefCount(). */ if (free_effort > LAZYFREE_THRESHOLD && val->refcount == 1) { atomicIncr(lazyfree_objects,1); - bioCreateBackgroundJob(BIO_LAZY_FREE,val,NULL,NULL); + bioCreateLazyFreeJob(lazyfreeFreeObject,1, val); dictSetVal(db->dict,de,NULL); } } @@ -141,7 +184,7 @@ void freeObjAsync(robj *key, robj *obj) { size_t free_effort = lazyfreeGetFreeEffort(key,obj); if (free_effort > LAZYFREE_THRESHOLD && obj->refcount == 1) { atomicIncr(lazyfree_objects,1); - bioCreateBackgroundJob(BIO_LAZY_FREE,obj,NULL,NULL); + bioCreateLazyFreeJob(lazyfreeFreeObject,1,obj); } else { decrRefCount(obj); } @@ -155,39 +198,17 @@ void emptyDbAsync(redisDb *db) { db->dict = dictCreate(&dbDictType,NULL); db->expires = dictCreate(&dbExpiresDictType,NULL); atomicIncr(lazyfree_objects,dictSize(oldht1)); - bioCreateBackgroundJob(BIO_LAZY_FREE,NULL,oldht1,oldht2); + bioCreateLazyFreeJob(lazyfreeFreeDatabase,2,oldht1,oldht2); } /* Release the radix tree mapping Redis Cluster keys to slots asynchronously. */ void freeSlotsToKeysMapAsync(rax *rt) { atomicIncr(lazyfree_objects,rt->numele); - bioCreateBackgroundJob(BIO_LAZY_FREE,NULL,NULL,rt); -} - -/* Release objects from the lazyfree thread. It's just decrRefCount() - * updating the count of objects to release. */ -void lazyfreeFreeObjectFromBioThread(robj *o) { - decrRefCount(o); - atomicDecr(lazyfree_objects,1); - atomicIncr(lazyfreed_objects,1); + bioCreateLazyFreeJob(lazyfreeFreeSlotsMap,1,rt); } -/* Release a database from the lazyfree thread. The 'db' pointer is the - * database which was substituted with a fresh one in the main thread - * when the database was logically deleted. */ -void lazyfreeFreeDatabaseFromBioThread(dict *ht1, dict *ht2) { - size_t numkeys = dictSize(ht1); - dictRelease(ht1); - dictRelease(ht2); - atomicDecr(lazyfree_objects,numkeys); - atomicIncr(lazyfreed_objects,numkeys); -} - -/* Release the radix tree mapping Redis Cluster keys to slots in the - * lazyfree thread. */ -void lazyfreeFreeSlotsMapFromBioThread(rax *rt) { - size_t len = rt->numele; - raxFree(rt); - atomicDecr(lazyfree_objects,len); - atomicIncr(lazyfreed_objects,len); +/* Free an object, if the object is huge enough, free it in async way. */ +void freeTrackingRadixTreeAsync(rax *tracking) { + atomicIncr(lazyfree_objects,tracking->numele); + bioCreateLazyFreeJob(lazyFreeTrackingTable,1,tracking); } diff --git a/src/replication.c b/src/replication.c index 92d7294f5..8a15c01a7 100644 --- a/src/replication.c +++ b/src/replication.c @@ -102,7 +102,7 @@ int bg_unlink(const char *filename) { errno = old_errno; return -1; } - bioCreateBackgroundJob(BIO_CLOSE_FILE,(void*)(long)fd,NULL,NULL); + bioCreateCloseJob(fd); return 0; /* Success. */ } } @@ -1752,7 +1752,7 @@ void readSyncBulkPayload(connection *conn) { return; } /* Close old rdb asynchronously. */ - if (old_rdb_fd != -1) bioCreateBackgroundJob(BIO_CLOSE_FILE,(void*)(long)old_rdb_fd,NULL,NULL); + if (old_rdb_fd != -1) bioCreateCloseJob(old_rdb_fd); if (rdbLoad(server.rdb_filename,&rsi,RDBFLAGS_REPLICATION) != C_OK) { serverLog(LL_WARNING, diff --git a/src/server.h b/src/server.h index 04fd709a8..887f7752e 100644 --- a/src/server.h +++ b/src/server.h @@ -1826,7 +1826,8 @@ void enableTracking(client *c, uint64_t redirect_to, uint64_t options, robj **pr void disableTracking(client *c); void trackingRememberKeys(client *c); void trackingInvalidateKey(client *c, robj *keyobj); -void trackingInvalidateKeysOnFlush(int dbid); +void trackingInvalidateKeysOnFlush(int async); +void freeTrackingRadixTreeAsync(rax *rt); void trackingLimitUsedSlots(void); uint64_t trackingGetTotalItems(void); uint64_t trackingGetTotalKeys(void); @@ -2260,7 +2261,7 @@ void discardDbBackup(dbBackup *buckup, int flags, void(callback)(void*)); int selectDb(client *c, int id); void signalModifiedKey(client *c, redisDb *db, robj *key); -void signalFlushedDb(int dbid); +void signalFlushedDb(int dbid, int async); unsigned int getKeysInSlot(unsigned int hashslot, robj **keys, unsigned int count); unsigned int countKeysInSlot(unsigned int hashslot); unsigned int delKeysInSlot(unsigned int hashslot); diff --git a/src/tracking.c b/src/tracking.c index 913577eab..852fa229b 100644 --- a/src/tracking.c +++ b/src/tracking.c @@ -350,19 +350,22 @@ void trackingInvalidateKey(client *c, robj *keyobj) { } /* This function is called when one or all the Redis databases are - * flushed (dbid == -1 in case of FLUSHALL). Caching keys are not - * specific for each DB but are global: currently what we do is send a - * special notification to clients with tracking enabled, sending a - * RESP NULL, which means, "all the keys", in order to avoid flooding - * clients with many invalidation messages for all the keys they may - * hold. + * flushed. Caching keys are not specific for each DB but are global: + * currently what we do is send a special notification to clients with + * tracking enabled, sending a RESP NULL, which means, "all the keys", + * in order to avoid flooding clients with many invalidation messages + * for all the keys they may hold. */ -void freeTrackingRadixTree(void *rt) { +void freeTrackingRadixTreeCallback(void *rt) { raxFree(rt); } +void freeTrackingRadixTree(rax *rt) { + raxFreeWithCallback(rt,freeTrackingRadixTreeCallback); +} + /* A RESP NULL is sent to indicate that all keys are invalid */ -void trackingInvalidateKeysOnFlush(int dbid) { +void trackingInvalidateKeysOnFlush(int async) { if (server.tracking_clients) { listNode *ln; listIter li; @@ -376,8 +379,12 @@ void trackingInvalidateKeysOnFlush(int dbid) { } /* In case of FLUSHALL, reclaim all the memory used by tracking. */ - if (dbid == -1 && TrackingTable) { - raxFreeWithCallback(TrackingTable,freeTrackingRadixTree); + if (TrackingTable) { + if (async) { + freeTrackingRadixTreeAsync(TrackingTable); + } else { + freeTrackingRadixTree(TrackingTable); + } TrackingTable = raxNew(); TrackingTableTotalItems = 0; } -- cgit v1.2.1 From b3dc23c5a8365a5e61b9ef8113970c1c36de6a13 Mon Sep 17 00:00:00 2001 From: xhe Date: Tue, 9 Jun 2020 19:46:49 +0800 Subject: add a read-only variant for HELLO As discussed in https://github.com/antirez/redis/issues/7364, it is good to have a HELLO command variant, which does not switch the current proto version of a redis server. While `HELLO` will work, it introduced a certain difficulty on parsing options of the command. We will need to offset the index of authentication and setname option by -1. So 0 is marked a special version meaning non-switching. And we do not need to change the code much. --- src/networking.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/networking.c b/src/networking.c index 8fee298c6..e16a39a14 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2763,7 +2763,7 @@ void helloCommand(client *c) { long long ver; if (getLongLongFromObject(c->argv[1],&ver) != C_OK || - ver < 2 || ver > 3) + (ver != 0 && ver < 2) || ver > 3) { addReplyError(c,"-NOPROTO unsupported protocol version"); return; @@ -2797,7 +2797,7 @@ void helloCommand(client *c) { } /* Let's switch to the specified RESP mode. */ - c->resp = ver; + if (ver != 0) c->resp = ver; addReplyMapLen(c,6 + !server.sentinel_mode); addReplyBulkCString(c,"server"); @@ -2807,7 +2807,7 @@ void helloCommand(client *c) { addReplyBulkCString(c,REDIS_VERSION); addReplyBulkCString(c,"proto"); - addReplyLongLong(c,ver); + addReplyLongLong(c,c->resp); addReplyBulkCString(c,"id"); addReplyLongLong(c,c->id); -- cgit v1.2.1 From 2e8f8c9b0c08d8c7c8b4dc72d65285d2761c5183 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 13:13:25 +0800 Subject: HELLO without protover Signed-off-by: xhe --- src/networking.c | 11 +++++------ src/sentinel.c | 2 +- src/server.c | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/networking.c b/src/networking.c index e16a39a14..37d4bf3d3 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2758,18 +2758,17 @@ NULL } } -/* HELLO [AUTH ] [SETNAME ] */ +/* HELLO [protocol-version] [AUTH ] [SETNAME ] */ void helloCommand(client *c) { - long long ver; + long long ver = 0; - if (getLongLongFromObject(c->argv[1],&ver) != C_OK || - (ver != 0 && ver < 2) || ver > 3) - { + if (c->argc >= 2 && getLongLongFromObject(c->argv[1],&ver) == C_OK && + (ver < 2 || ver > 3)) { addReplyError(c,"-NOPROTO unsupported protocol version"); return; } - for (int j = 2; j < c->argc; j++) { + for (int j = ver == 0 ? 1 : 2; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { diff --git a/src/sentinel.c b/src/sentinel.c index 9764f9004..1c1b4bf5b 100644 --- a/src/sentinel.c +++ b/src/sentinel.c @@ -469,7 +469,7 @@ struct redisCommand sentinelcmds[] = { {"client",clientCommand,-2,"admin random @connection",0,NULL,0,0,0,0,0}, {"shutdown",shutdownCommand,-1,"admin",0,NULL,0,0,0,0,0}, {"auth",authCommand,-2,"no-auth fast @connection",0,NULL,0,0,0,0,0}, - {"hello",helloCommand,-2,"no-auth fast @connection",0,NULL,0,0,0,0,0}, + {"hello",helloCommand,-1,"no-auth fast @connection",0,NULL,0,0,0,0,0}, {"acl",aclCommand,-2,"admin",0,NULL,0,0,0,0,0,0}, {"command",commandCommand,-1, "random @connection", 0,NULL,0,0,0,0,0,0} }; diff --git a/src/server.c b/src/server.c index f28c6e114..0eabe8ea2 100644 --- a/src/server.c +++ b/src/server.c @@ -869,7 +869,7 @@ struct redisCommand redisCommandTable[] = { "admin no-script random ok-loading ok-stale @connection", 0,NULL,0,0,0,0,0,0}, - {"hello",helloCommand,-2, + {"hello",helloCommand,-1, "no-auth no-script fast no-monitor ok-loading ok-stale no-slowlog @connection", 0,NULL,0,0,0,0,0,0}, -- cgit v1.2.1 From 7a7c60459e0853b1880bd1692fb15c4b67e00fa9 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 15:26:24 +0800 Subject: add a test Signed-off-by: xhe --- tests/unit/tracking.tcl | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index fc2800791..deecc1d06 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -135,6 +135,26 @@ start_server {tags {"tracking"}} { assert {[lindex $reply 2] eq {proto 3}} } + test {HELLO without protover} { + set reply [r HELLO 3] + assert {[lindex $reply 2] eq {proto 3}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 3}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 3}} + + set reply [r HELLO 2] + assert {[lindex $reply 2] eq {proto 2}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 2}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 2}} + } + test {RESP3 based basic invalidation} { r CLIENT TRACKING off r CLIENT TRACKING on -- cgit v1.2.1 From 50d750733e52dd9fff26fbcf8274d8d6c81ebfca Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 15:29:17 +0800 Subject: prefer ! Signed-off-by: xhe --- src/networking.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index 37d4bf3d3..d66eb2cea 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2768,7 +2768,7 @@ void helloCommand(client *c) { return; } - for (int j = ver == 0 ? 1 : 2; j < c->argc; j++) { + for (int j = !ver ? 1 : 2; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { @@ -2796,7 +2796,7 @@ void helloCommand(client *c) { } /* Let's switch to the specified RESP mode. */ - if (ver != 0) c->resp = ver; + if (!!ver) c->resp = ver; addReplyMapLen(c,6 + !server.sentinel_mode); addReplyBulkCString(c,"server"); -- cgit v1.2.1 From 35fc7fda7aac1699cf6f7ba902cf414d07c1cd3b Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Thu, 24 Dec 2020 03:42:52 -0500 Subject: Typo: timout -> timeout (#8228) --- src/networking.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index 8fee298c6..ef1210b99 100644 --- a/src/networking.c +++ b/src/networking.c @@ -856,7 +856,7 @@ void addReplySubcommandSyntaxError(client *c) { sdsfree(cmd); } -/* Append 'src' client output buffers into 'dst' client output buffers. +/* Append 'src' client output buffers into 'dst' client output buffers. * This function clears the output buffers of 'src' */ void AddReplyFromClient(client *dst, client *src) { /* If the source client contains a partial response due to client output @@ -2380,7 +2380,7 @@ void clientCommand(client *c) { "LIST [options ...] -- Return information about client connections. Options:", " TYPE (normal|master|replica|pubsub) -- Return clients of specified type.", " ID id [id ...] -- Return clients of specified IDs only.", -"PAUSE -- Suspend all Redis clients for milliseconds.", +"PAUSE -- Suspend all Redis clients for milliseconds.", "REPLY (on|off|skip) -- Control the replies sent to the current connection.", "SETNAME -- Assign the name to the current connection.", "UNBLOCK [TIMEOUT|ERROR] -- Unblock the specified blocked client.", -- cgit v1.2.1 From 60f13e7a865f7c0d4908b25803f42058c7a19201 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 16:50:08 +0800 Subject: try to fix the test Signed-off-by: xhe --- tests/unit/tracking.tcl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index deecc1d06..4d7da755d 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -146,13 +146,13 @@ start_server {tags {"tracking"}} { assert {[lindex $reply 2] eq {proto 3}} set reply [r HELLO 2] - assert {[lindex $reply 2] eq {proto 2}} + assert {[lindex $reply 6] eq 2} set reply [r HELLO] - assert {[lindex $reply 2] eq {proto 2}} + assert {[lindex $reply 6] eq 2} set reply [r HELLO] - assert {[lindex $reply 2] eq {proto 2}} + assert {[lindex $reply 6] eq 2} } test {RESP3 based basic invalidation} { -- cgit v1.2.1 From 456c347d4520bce417e48d0d01a87035483755a5 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:22 +0800 Subject: simplify Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index d66eb2cea..0953b8e5e 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2796,7 +2796,7 @@ void helloCommand(client *c) { } /* Let's switch to the specified RESP mode. */ - if (!!ver) c->resp = ver; + if (ver) c->resp = ver; addReplyMapLen(c,6 + !server.sentinel_mode); addReplyBulkCString(c,"server"); -- cgit v1.2.1 From c07d3bd8dd8ba108f743c09b059f157e9799d5c5 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:36 +0800 Subject: simplify Co-authored-by: Oran Agra --- src/networking.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/networking.c b/src/networking.c index 0953b8e5e..132596f9e 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2761,6 +2761,7 @@ NULL /* HELLO [protocol-version] [AUTH ] [SETNAME ] */ void helloCommand(client *c) { long long ver = 0; + int next_arg = 1; if (c->argc >= 2 && getLongLongFromObject(c->argv[1],&ver) == C_OK && (ver < 2 || ver > 3)) { -- cgit v1.2.1 From 723b4a15a34ce25490882f40ef51b24a5b61304c Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:45 +0800 Subject: simplify Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 132596f9e..32ffb3ce8 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2763,7 +2763,7 @@ void helloCommand(client *c) { long long ver = 0; int next_arg = 1; - if (c->argc >= 2 && getLongLongFromObject(c->argv[1],&ver) == C_OK && + if (c->argc >= 2 && getLongLongFromObject(c->argv[next_arg++],&ver) == C_OK && (ver < 2 || ver > 3)) { addReplyError(c,"-NOPROTO unsupported protocol version"); return; -- cgit v1.2.1 From 98f39a37fb14f2b1c1965de143011402728dafbc Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:53 +0800 Subject: simplify Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 32ffb3ce8..e29256488 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2769,7 +2769,7 @@ void helloCommand(client *c) { return; } - for (int j = !ver ? 1 : 2; j < c->argc; j++) { + for (int j = next_arg; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { -- cgit v1.2.1 From ef14c18c8e83f3f11f5436e3dbeee9dfd5665de1 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:31:50 +0800 Subject: fix the test Signed-off-by: xhe --- tests/unit/tracking.tcl | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index 4d7da755d..1beaf71a9 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -146,13 +146,16 @@ start_server {tags {"tracking"}} { assert {[lindex $reply 2] eq {proto 3}} set reply [r HELLO 2] - assert {[lindex $reply 6] eq 2} + assert {[lindex $reply 4] eq "proto"} + assert {[lindex $reply 5] eq 2} set reply [r HELLO] - assert {[lindex $reply 6] eq 2} + assert {[lindex $reply 4] eq "proto"} + assert {[lindex $reply 5] eq 2} set reply [r HELLO] - assert {[lindex $reply 6] eq 2} + assert {[lindex $reply 4] eq "proto"} + assert {[lindex $reply 5] eq 2} } test {RESP3 based basic invalidation} { -- cgit v1.2.1 From 9bd212cf241154d22849ef2addd0d5a7551b3a55 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Fri, 18 Dec 2020 00:02:57 +0200 Subject: syncWithMaster: sendSynchronousCommand split to send, and receive This is just a refactoring commit. This function was never actually used as a synchronous (do both send or receive), it was always used only ine one of the two modes, which meant it has to take extra arguments that are not relevant for the other. Besides that, a tool that sends a synchronous command, it not something we want in our toolbox (synchronous IO in single threaded app is evil). sendSynchronousCommand was now refactored into separate sending and receiving APIs, and the sending part has two variants, one taking vaargs, and the other taking argc+argv (and an optional length array which means you can use binary sds strings). --- src/replication.c | 177 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 100 insertions(+), 77 deletions(-) diff --git a/src/replication.c b/src/replication.c index 8a15c01a7..4977bba42 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1827,72 +1827,94 @@ error: return; } -/* Send a synchronous command to the master. Used to send AUTH and - * REPLCONF commands before starting the replication with SYNC. +char *receiveSynchronousResponse(connection *conn) { + char buf[256]; + /* Read the reply from the server. */ + if (connSyncReadLine(conn,buf,sizeof(buf),server.repl_syncio_timeout*1000) == -1) + { + return sdscatprintf(sdsempty(),"-Reading from master: %s", + strerror(errno)); + } + server.repl_transfer_lastio = server.unixtime; + return sdsnew(buf); +} + +/* Send a pre-formatted multi-bulk command to the connection. */ +char* sendCommandRaw(connection *conn, sds cmd) { + if (connSyncWrite(conn,cmd,sdslen(cmd),server.repl_syncio_timeout*1000) == -1) { + return sdscatprintf(sdsempty(),"-Writing to master: %s", + connGetLastError(conn)); + } + return NULL; +} + +/* Compose a multi-bulk command and send it to the connection. + * Used to send AUTH and REPLCONF commands to the master before starting the + * replication. + * + * Takes a list of char* arguments, terminated by a NULL argument. * * The command returns an sds string representing the result of the * operation. On error the first byte is a "-". */ -#define SYNC_CMD_READ (1<<0) -#define SYNC_CMD_WRITE (1<<1) -#define SYNC_CMD_WRITE_SDS (1<<2) -#define SYNC_CMD_FULL (SYNC_CMD_READ|SYNC_CMD_WRITE) -char *sendSynchronousCommand(int flags, connection *conn, ...) { +char *sendCommand(connection *conn, ...) { + va_list ap; + sds cmd = sdsempty(); + sds cmdargs = sdsempty(); + size_t argslen = 0; + char *arg; /* Create the command to send to the master, we use redis binary * protocol to make sure correct arguments are sent. This function * is not safe for all binary data. */ - if (flags & SYNC_CMD_WRITE) { - char *arg; - va_list ap; - sds cmd = sdsempty(); - sds cmdargs = sdsempty(); - size_t argslen = 0; - va_start(ap,conn); - - while(1) { - arg = va_arg(ap, char*); - if (arg == NULL) break; - if (flags & SYNC_CMD_WRITE_SDS) { - cmdargs = sdscatprintf(cmdargs,"$%zu\r\n", sdslen((sds)arg)); - cmdargs = sdscatsds(cmdargs, (sds)arg); - cmdargs = sdscat(cmdargs, "\r\n"); - } else { - cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg); - } - argslen++; - } - - va_end(ap); - - cmd = sdscatprintf(cmd,"*%zu\r\n",argslen); - cmd = sdscatsds(cmd,cmdargs); - sdsfree(cmdargs); - - /* Transfer command to the server. */ - if (connSyncWrite(conn,cmd,sdslen(cmd),server.repl_syncio_timeout*1000) - == -1) - { - sdsfree(cmd); - return sdscatprintf(sdsempty(),"-Writing to master: %s", - connGetLastError(conn)); - } - sdsfree(cmd); - } + va_start(ap,conn); + while(1) { + arg = va_arg(ap, char*); + if (arg == NULL) break; + cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg); + argslen++; + } + + cmd = sdscatprintf(cmd,"*%zu\r\n",argslen); + cmd = sdscatsds(cmd,cmdargs); + sdsfree(cmdargs); + + va_end(ap); + char* err = sendCommandRaw(conn, cmd); + sdsfree(cmd); + if(err) + return err; + return NULL; +} - /* Read the reply from the server. */ - if (flags & SYNC_CMD_READ) { - char buf[256]; +/* Compose a multi-bulk command and send it to the connection. + * Used to send AUTH and REPLCONF commands to the master before starting the + * replication. + * + * argv_lens is optional, when NULL, strlen is used. + * + * The command returns an sds string representing the result of the + * operation. On error the first byte is a "-". + */ +char *sendCommandArgv(connection *conn, int argc, char **argv, size_t *argv_lens) { + sds cmd = sdsempty(); + char *arg; + int i; - if (connSyncReadLine(conn,buf,sizeof(buf),server.repl_syncio_timeout*1000) - == -1) - { - return sdscatprintf(sdsempty(),"-Reading from master: %s", - strerror(errno)); - } - server.repl_transfer_lastio = server.unixtime; - return sdsnew(buf); - } + /* Create the command to send to the master. */ + cmd = sdscatfmt(cmd,"*%i\r\n",argc); + for (i=0; i Date: Fri, 18 Dec 2020 22:10:31 +0200 Subject: syncWithMaster: use pipeline for AUTH+REPLCONF*3 The commit deals with the syncWithMaster and the ugly state machine in it. It attempts to clean it a bit, but more importantly it uses pipeline for part of the work (rather than 7 round trips, we now have 4). i.e. the connect and PING are separate, then AUTH + 3 REPLCONF in one pipeline, and finally the PSYNC (must be separate since the master has to have an empty output buffer). --- src/replication.c | 131 ++++++++++++++++++++++++++---------------------------- src/server.h | 35 +++++++-------- 2 files changed, 81 insertions(+), 85 deletions(-) diff --git a/src/replication.c b/src/replication.c index 4977bba42..520c43fa4 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1334,8 +1334,8 @@ void shiftReplicationId(void) { /* Returns 1 if the given replication state is a handshake state, * 0 otherwise. */ int slaveIsInHandshakeState(void) { - return server.repl_state >= REPL_STATE_RECEIVE_PONG && - server.repl_state <= REPL_STATE_RECEIVE_PSYNC; + return server.repl_state >= REPL_STATE_RECEIVE_PING_REPLY && + server.repl_state <= REPL_STATE_RECEIVE_PSYNC_REPLY; } /* Avoid the master to detect the slave is timing out while loading the @@ -2159,7 +2159,7 @@ void syncWithMaster(connection *conn) { * registered and we can wait for the PONG reply. */ connSetReadHandler(conn, syncWithMaster); connSetWriteHandler(conn, NULL); - server.repl_state = REPL_STATE_RECEIVE_PONG; + server.repl_state = REPL_STATE_RECEIVE_PING_REPLY; /* Send the PING, don't check for errors at all, we have the timeout * that will take care about this. */ err = sendCommand(conn,"PING",NULL); @@ -2168,7 +2168,7 @@ void syncWithMaster(connection *conn) { } /* Receive the PONG command. */ - if (server.repl_state == REPL_STATE_RECEIVE_PONG) { + if (server.repl_state == REPL_STATE_RECEIVE_PING_REPLY) { err = receiveSynchronousResponse(conn); /* We accept only two replies as valid, a positive +PONG reply @@ -2189,11 +2189,12 @@ void syncWithMaster(connection *conn) { "Master replied to PING, replication can continue..."); } sdsfree(err); - server.repl_state = REPL_STATE_SEND_AUTH; + err = NULL; + server.repl_state = REPL_STATE_SEND_HANDSHAKE; } - /* AUTH with the master if required. */ - if (server.repl_state == REPL_STATE_SEND_AUTH) { + if (server.repl_state == REPL_STATE_SEND_HANDSHAKE) { + /* AUTH with the master if required. */ if (server.masterauth) { char *args[3] = {"AUTH",NULL,NULL}; size_t lens[3] = {4,0,0}; @@ -2208,15 +2209,53 @@ void syncWithMaster(connection *conn) { argc++; err = sendCommandArgv(conn, argc, args, lens); if (err) goto write_error; - server.repl_state = REPL_STATE_RECEIVE_AUTH; - return; - } else { - server.repl_state = REPL_STATE_SEND_PORT; } + + /* Set the slave port, so that Master's INFO command can list the + * slave listening port correctly. */ + { + int port; + if (server.slave_announce_port) + port = server.slave_announce_port; + else if (server.tls_replication && server.tls_port) + port = server.tls_port; + else + port = server.port; + sds portstr = sdsfromlonglong(port); + err = sendCommand(conn,"REPLCONF", + "listening-port",portstr, NULL); + sdsfree(portstr); + if (err) goto write_error; + } + + /* Set the slave ip, so that Master's INFO command can list the + * slave IP address port correctly in case of port forwarding or NAT. + * Skip REPLCONF ip-address if there is no slave-announce-ip option set. */ + if (server.slave_announce_ip) { + err = sendCommand(conn,"REPLCONF", + "ip-address",server.slave_announce_ip, NULL); + if (err) goto write_error; + } + + /* Inform the master of our (slave) capabilities. + * + * EOF: supports EOF-style RDB transfer for diskless replication. + * PSYNC2: supports PSYNC v2, so understands +CONTINUE . + * + * The master will ignore capabilities it does not understand. */ + err = sendCommand(conn,"REPLCONF", + "capa","eof","capa","psync2",NULL); + if (err) goto write_error; + + server.repl_state = REPL_STATE_RECEIVE_AUTH_REPLY; + return; } + if (server.repl_state == REPL_STATE_RECEIVE_AUTH_REPLY && !server.masterauth) + server.repl_state = REPL_STATE_RECEIVE_PORT_REPLY; + /* Receive AUTH reply. */ - if (server.repl_state == REPL_STATE_RECEIVE_AUTH) { + if (server.repl_state == REPL_STATE_RECEIVE_AUTH_REPLY) { err = receiveSynchronousResponse(conn); if (err[0] == '-') { serverLog(LL_WARNING,"Unable to AUTH to MASTER: %s",err); @@ -2224,28 +2263,13 @@ void syncWithMaster(connection *conn) { goto error; } sdsfree(err); - server.repl_state = REPL_STATE_SEND_PORT; - } - - /* Set the slave port, so that Master's INFO command can list the - * slave listening port correctly. */ - if (server.repl_state == REPL_STATE_SEND_PORT) { - int port; - if (server.slave_announce_port) port = server.slave_announce_port; - else if (server.tls_replication && server.tls_port) port = server.tls_port; - else port = server.port; - sds portstr = sdsfromlonglong(port); - err = sendCommand(conn,"REPLCONF", - "listening-port",portstr, NULL); - sdsfree(portstr); - if (err) goto write_error; - sdsfree(err); - server.repl_state = REPL_STATE_RECEIVE_PORT; + err = NULL; + server.repl_state = REPL_STATE_RECEIVE_PORT_REPLY; return; } /* Receive REPLCONF listening-port reply. */ - if (server.repl_state == REPL_STATE_RECEIVE_PORT) { + if (server.repl_state == REPL_STATE_RECEIVE_PORT_REPLY) { err = receiveSynchronousResponse(conn); /* Ignore the error if any, not all the Redis versions support * REPLCONF listening-port. */ @@ -2254,29 +2278,15 @@ void syncWithMaster(connection *conn) { "REPLCONF listening-port: %s", err); } sdsfree(err); - server.repl_state = REPL_STATE_SEND_IP; - } - - /* Skip REPLCONF ip-address if there is no slave-announce-ip option set. */ - if (server.repl_state == REPL_STATE_SEND_IP && - server.slave_announce_ip == NULL) - { - server.repl_state = REPL_STATE_SEND_CAPA; - } - - /* Set the slave ip, so that Master's INFO command can list the - * slave IP address port correctly in case of port forwarding or NAT. */ - if (server.repl_state == REPL_STATE_SEND_IP) { - err = sendCommand(conn,"REPLCONF", - "ip-address",server.slave_announce_ip, NULL); - if (err) goto write_error; - sdsfree(err); - server.repl_state = REPL_STATE_RECEIVE_IP; + server.repl_state = REPL_STATE_RECEIVE_IP_REPLY; return; } + if (server.repl_state == REPL_STATE_RECEIVE_IP_REPLY && !server.slave_announce_ip) + server.repl_state = REPL_STATE_RECEIVE_CAPA_REPLY; + /* Receive REPLCONF ip-address reply. */ - if (server.repl_state == REPL_STATE_RECEIVE_IP) { + if (server.repl_state == REPL_STATE_RECEIVE_IP_REPLY) { err = receiveSynchronousResponse(conn); /* Ignore the error if any, not all the Redis versions support * REPLCONF listening-port. */ @@ -2285,26 +2295,12 @@ void syncWithMaster(connection *conn) { "REPLCONF ip-address: %s", err); } sdsfree(err); - server.repl_state = REPL_STATE_SEND_CAPA; - } - - /* Inform the master of our (slave) capabilities. - * - * EOF: supports EOF-style RDB transfer for diskless replication. - * PSYNC2: supports PSYNC v2, so understands +CONTINUE . - * - * The master will ignore capabilities it does not understand. */ - if (server.repl_state == REPL_STATE_SEND_CAPA) { - err = sendCommand(conn,"REPLCONF", - "capa","eof","capa","psync2",NULL); - if (err) goto write_error; - sdsfree(err); - server.repl_state = REPL_STATE_RECEIVE_CAPA; + server.repl_state = REPL_STATE_RECEIVE_CAPA_REPLY; return; } /* Receive CAPA reply. */ - if (server.repl_state == REPL_STATE_RECEIVE_CAPA) { + if (server.repl_state == REPL_STATE_RECEIVE_CAPA_REPLY) { err = receiveSynchronousResponse(conn); /* Ignore the error if any, not all the Redis versions support * REPLCONF capa. */ @@ -2313,6 +2309,7 @@ void syncWithMaster(connection *conn) { "REPLCONF capa: %s", err); } sdsfree(err); + err = NULL; server.repl_state = REPL_STATE_SEND_PSYNC; } @@ -2326,12 +2323,12 @@ void syncWithMaster(connection *conn) { err = sdsnew("Write error sending the PSYNC command."); goto write_error; } - server.repl_state = REPL_STATE_RECEIVE_PSYNC; + server.repl_state = REPL_STATE_RECEIVE_PSYNC_REPLY; return; } /* If reached this point, we should be in REPL_STATE_RECEIVE_PSYNC. */ - if (server.repl_state != REPL_STATE_RECEIVE_PSYNC) { + if (server.repl_state != REPL_STATE_RECEIVE_PSYNC_REPLY) { serverLog(LL_WARNING,"syncWithMaster(): state machine error, " "state should be RECEIVE_PSYNC but is %d", server.repl_state); diff --git a/src/server.h b/src/server.h index 887f7752e..a19b5ad9e 100644 --- a/src/server.h +++ b/src/server.h @@ -299,24 +299,23 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT]; /* Slave replication state. Used in server.repl_state for slaves to remember * what to do next. */ -#define REPL_STATE_NONE 0 /* No active replication */ -#define REPL_STATE_CONNECT 1 /* Must connect to master */ -#define REPL_STATE_CONNECTING 2 /* Connecting to master */ -/* --- Handshake states, must be ordered --- */ -#define REPL_STATE_RECEIVE_PONG 3 /* Wait for PING reply */ -#define REPL_STATE_SEND_AUTH 4 /* Send AUTH to master */ -#define REPL_STATE_RECEIVE_AUTH 5 /* Wait for AUTH reply */ -#define REPL_STATE_SEND_PORT 6 /* Send REPLCONF listening-port */ -#define REPL_STATE_RECEIVE_PORT 7 /* Wait for REPLCONF reply */ -#define REPL_STATE_SEND_IP 8 /* Send REPLCONF ip-address */ -#define REPL_STATE_RECEIVE_IP 9 /* Wait for REPLCONF reply */ -#define REPL_STATE_SEND_CAPA 10 /* Send REPLCONF capa */ -#define REPL_STATE_RECEIVE_CAPA 11 /* Wait for REPLCONF reply */ -#define REPL_STATE_SEND_PSYNC 12 /* Send PSYNC */ -#define REPL_STATE_RECEIVE_PSYNC 13 /* Wait for PSYNC reply */ -/* --- End of handshake states --- */ -#define REPL_STATE_TRANSFER 14 /* Receiving .rdb from master */ -#define REPL_STATE_CONNECTED 15 /* Connected to master */ +typedef enum { + REPL_STATE_NONE = 0, /* No active replication */ + REPL_STATE_CONNECT, /* Must connect to master */ + REPL_STATE_CONNECTING, /* Connecting to master */ + /* --- Handshake states, must be ordered --- */ + REPL_STATE_RECEIVE_PING_REPLY, /* Wait for PING reply */ + REPL_STATE_SEND_HANDSHAKE, /* Send handshake sequance to master */ + REPL_STATE_RECEIVE_AUTH_REPLY, /* Wait for AUTH reply */ + REPL_STATE_RECEIVE_PORT_REPLY, /* Wait for REPLCONF reply */ + REPL_STATE_RECEIVE_IP_REPLY, /* Wait for REPLCONF reply */ + REPL_STATE_RECEIVE_CAPA_REPLY, /* Wait for REPLCONF reply */ + REPL_STATE_SEND_PSYNC, /* Send PSYNC */ + REPL_STATE_RECEIVE_PSYNC_REPLY, /* Wait for PSYNC reply */ + /* --- End of handshake states --- */ + REPL_STATE_TRANSFER, /* Receiving .rdb from master */ + REPL_STATE_CONNECTED, /* Connected to master */ +} repl_state; /* State of slaves from the POV of the master. Used in client->replstate. * In SEND_BULK and ONLINE state the slave receives new updates -- cgit v1.2.1 From c4b52fc7c9e77fb1f2719d2cb5b9977b90698721 Mon Sep 17 00:00:00 2001 From: huangzhw Date: Thu, 24 Dec 2020 17:58:43 +0800 Subject: cleanup: ziplist prev entry large length use sizeof(uint32_t) instead 4 (#8241) This is just a cleanup, no bugs in the real world. Co-authored-by: Oran Agra --- src/ziplist.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ziplist.c b/src/ziplist.c index 866078613..96ba47e13 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -431,19 +431,21 @@ unsigned int zipStoreEntryEncoding(unsigned char *p, unsigned char encoding, uns /* Encode the length of the previous entry and write it to "p". This only * uses the larger encoding (required in __ziplistCascadeUpdate). */ int zipStorePrevEntryLengthLarge(unsigned char *p, unsigned int len) { + uint32_t u32; if (p != NULL) { p[0] = ZIP_BIG_PREVLEN; - memcpy(p+1,&len,sizeof(len)); + u32 = len; + memcpy(p+1,&u32,sizeof(u32)); memrev32ifbe(p+1); } - return 1+sizeof(len); + return 1 + sizeof(uint32_t); } /* Encode the length of the previous entry and write it to "p". Return the * number of bytes needed to encode this length if "p" is NULL. */ unsigned int zipStorePrevEntryLength(unsigned char *p, unsigned int len) { if (p == NULL) { - return (len < ZIP_BIG_PREVLEN) ? 1 : sizeof(len)+1; + return (len < ZIP_BIG_PREVLEN) ? 1 : sizeof(uint32_t) + 1; } else { if (len < ZIP_BIG_PREVLEN) { p[0] = len; -- cgit v1.2.1 From 4e36925c662bba9c83661caa166d4699bcbef17c Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 19:16:28 +0800 Subject: correction Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index e29256488..6d571aad1 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2758,7 +2758,7 @@ NULL } } -/* HELLO [protocol-version] [AUTH ] [SETNAME ] */ +/* HELLO [ [AUTH ] [SETNAME ] ] */ void helloCommand(client *c) { long long ver = 0; int next_arg = 1; -- cgit v1.2.1 From 955e00fbec2bf9cb2dbe53fceef00941681bfbd4 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 19:23:30 +0800 Subject: ask protover for authentication Signed-off-by: xhe --- src/networking.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/networking.c b/src/networking.c index 6d571aad1..66e91c521 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2769,6 +2769,11 @@ void helloCommand(client *c) { return; } + if (!ver && next_arg < c->argc) { + addReplyError(c,"Authentication needs to provide an protocol version"); + return; + } + for (int j = next_arg; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; -- cgit v1.2.1 From f6711b7da5445f3e4ba3595c9c0153183184078e Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 19:25:30 +0800 Subject: reword Signed-off-by: xhe --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 66e91c521..0760a4669 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2770,7 +2770,7 @@ void helloCommand(client *c) { } if (!ver && next_arg < c->argc) { - addReplyError(c,"Authentication needs to provide an protocol version"); + addReplyError(c,"Need to provide an protocol version for other arguments"); return; } -- cgit v1.2.1 From 78eaf503fdbafe94d1905301ba47294326b278e4 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 20:13:57 +0800 Subject: address comment Signed-off-by: xhe --- src/networking.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/networking.c b/src/networking.c index 0760a4669..c44976d12 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2763,15 +2763,16 @@ void helloCommand(client *c) { long long ver = 0; int next_arg = 1; - if (c->argc >= 2 && getLongLongFromObject(c->argv[next_arg++],&ver) == C_OK && - (ver < 2 || ver > 3)) { - addReplyError(c,"-NOPROTO unsupported protocol version"); - return; - } + if (c->argc >= 2) { + if (getLongLongFromObjectOrReply(c, c->argv[next_arg++], &ver, + "The second argument should the protocol version if provided") != C_OK) { + return; + } - if (!ver && next_arg < c->argc) { - addReplyError(c,"Need to provide an protocol version for other arguments"); - return; + if (ver < 2 || ver > 3) { + addReplyError(c,"-NOPROTO unsupported protocol version"); + return; + } } for (int j = next_arg; j < c->argc; j++) { -- cgit v1.2.1 From 4617960863c2baf3545ca74009b03358fa2363e1 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 24 Dec 2020 14:33:53 +0200 Subject: resolve hung test. --- tests/unit/tracking.tcl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index 1beaf71a9..b315293a0 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -156,6 +156,9 @@ start_server {tags {"tracking"}} { set reply [r HELLO] assert {[lindex $reply 4] eq "proto"} assert {[lindex $reply 5] eq 2} + + # restore RESP3 for next test + r HELLO 3 } test {RESP3 based basic invalidation} { -- cgit v1.2.1 From fae5ceef2ab65797af7d7778143efb8e4d598301 Mon Sep 17 00:00:00 2001 From: xhe Date: Fri, 25 Dec 2020 01:40:06 +0800 Subject: reword Co-authored-by: Itamar Haber --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index c44976d12..187fafbb4 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2765,7 +2765,7 @@ void helloCommand(client *c) { if (c->argc >= 2) { if (getLongLongFromObjectOrReply(c, c->argv[next_arg++], &ver, - "The second argument should the protocol version if provided") != C_OK) { + "Protocol version is not an integer or out of range") != C_OK) { return; } -- cgit v1.2.1 From e6c1aeaf08b14076ccdf26aaef65b76b1597ee77 Mon Sep 17 00:00:00 2001 From: xhe Date: Fri, 25 Dec 2020 10:17:55 +0800 Subject: fix the format Signed-off-by: xhe --- src/networking.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index 187fafbb4..ff71559a4 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2765,9 +2765,9 @@ void helloCommand(client *c) { if (c->argc >= 2) { if (getLongLongFromObjectOrReply(c, c->argv[next_arg++], &ver, - "Protocol version is not an integer or out of range") != C_OK) { + "Protocol version is not an integer or out of range") != C_OK) { return; - } + } if (ver < 2 || ver > 3) { addReplyError(c,"-NOPROTO unsupported protocol version"); -- cgit v1.2.1 From e18068d9d83e6c7298b5501ffabfdb7709984fc0 Mon Sep 17 00:00:00 2001 From: Itamar Haber Date: Fri, 25 Dec 2020 18:27:30 +0200 Subject: Use addReplyErrorObject with shared.syntaxerror (#8248) --- src/db.c | 2 +- src/geo.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/db.c b/src/db.c index 64a473bf3..495300613 100644 --- a/src/db.c +++ b/src/db.c @@ -1208,7 +1208,7 @@ void copyCommand(client *c) { selectDb(c,srcid); /* Back to the source DB */ j++; /* Consume additional arg. */ } else { - addReply(c, shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } diff --git a/src/geo.c b/src/geo.c index 75a88398e..417bf897e 100644 --- a/src/geo.c +++ b/src/geo.c @@ -620,7 +620,7 @@ void georadiusGeneric(client *c, int srcKeyIndex, int flags) { bybox = 1; i += 3; } else { - addReply(c, shared.syntaxerr); + addReplyErrorObject(c,shared.syntaxerr); return; } } -- cgit v1.2.1 From f44186e5755e943f39c1446ad9576ab97de5039d Mon Sep 17 00:00:00 2001 From: Itamar Haber Date: Fri, 25 Dec 2020 21:49:24 +0200 Subject: Adds count to L/RPOP (#8179) Adds: `L/RPOP [count]` Implements no. 2 of the following strategies: 1. Loop on listTypePop - this would result in multiple calls for memory freeing and allocating (see https://github.com/redis/redis/pull/8179/commits/769167a079b0e110d28e4a8099dce1ecd45682b5) 2. Iterate the range to build the reply, then call quickListDelRange - this requires two iterations and **is the current choice** 3. Refactor quicklist to have a pop variant of quickListDelRange - probably optimal but more complex Also: * There's a historical check for NULL after calling listTypePop that was converted to an assert. * This refactors common logic shared between LRANGE and the new form of LPOP/RPOP into addListRangeReply (adds test for b/w compat) * Consequently, it may have made sense to have `LRANGE l -1 -2` and `LRANGE l 9 0` be legit and return a reverse reply. Due to historical reasons that would be, however, a breaking change. * Added minimal comments to existing commands to adhere to the style, make core dev life easier and get commit karma, naturally. --- src/server.c | 4 +- src/server.h | 2 +- src/t_list.c | 156 ++++++++++++++++++++++++++++++++++------------- tests/unit/type/list.tcl | 17 ++++++ 4 files changed, 132 insertions(+), 47 deletions(-) diff --git a/src/server.c b/src/server.c index f28c6e114..ad78abeba 100644 --- a/src/server.c +++ b/src/server.c @@ -288,11 +288,11 @@ struct redisCommand redisCommandTable[] = { "write use-memory @list", 0,NULL,1,1,1,0,0,0}, - {"rpop",rpopCommand,2, + {"rpop",rpopCommand,-2, "write fast @list", 0,NULL,1,1,1,0,0,0}, - {"lpop",lpopCommand,2, + {"lpop",lpopCommand,-2, "write fast @list", 0,NULL,1,1,1,0,0,0}, diff --git a/src/server.h b/src/server.h index a19b5ad9e..1dc761959 100644 --- a/src/server.h +++ b/src/server.h @@ -1849,7 +1849,7 @@ void listTypeConvert(robj *subject, int enc); robj *listTypeDup(robj *o); void unblockClientWaitingData(client *c); void popGenericCommand(client *c, int where); -void listElementsRemoved(client *c, robj *key, int where, robj *o); +void listElementsRemoved(client *c, robj *key, int where, robj *o, long count); /* MULTI/EXEC/WATCH... */ void unwatchAllKeys(client *c); diff --git a/src/t_list.c b/src/t_list.c index c8323c612..b64b37a8a 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -206,7 +206,7 @@ robj *listTypeDup(robj *o) { lobj->encoding = OBJ_ENCODING_QUICKLIST; break; default: - serverPanic("Wrong encoding."); + serverPanic("Unknown list encoding"); break; } return lobj; @@ -216,6 +216,7 @@ robj *listTypeDup(robj *o) { * List Commands *----------------------------------------------------------------------------*/ +/* Implements LPUSH/RPUSH. */ void pushGenericCommand(client *c, int where) { int j, pushed = 0; robj *lobj = lookupKeyWrite(c->db,c->argv[1]); @@ -244,14 +245,17 @@ void pushGenericCommand(client *c, int where) { server.dirty += pushed; } +/* LPUSH [ ...] */ void lpushCommand(client *c) { pushGenericCommand(c,LIST_HEAD); } +/* RPUSH [ ...] */ void rpushCommand(client *c) { pushGenericCommand(c,LIST_TAIL); } +/* Implements LPUSHX/RPUSHX. */ void pushxGenericCommand(client *c, int where) { int j, pushed = 0; robj *subject; @@ -274,14 +278,17 @@ void pushxGenericCommand(client *c, int where) { server.dirty += pushed; } +/* LPUSHX [ ...] */ void lpushxCommand(client *c) { pushxGenericCommand(c,LIST_HEAD); } +/* RPUSH [ ...] */ void rpushxCommand(client *c) { pushxGenericCommand(c,LIST_TAIL); } +/* LINSERT (BEFORE|AFTER) */ void linsertCommand(client *c) { int where; robj *subject; @@ -326,12 +333,14 @@ void linsertCommand(client *c) { addReplyLongLong(c,listTypeLength(subject)); } +/* LLEN */ void llenCommand(client *c) { robj *o = lookupKeyReadOrReply(c,c->argv[1],shared.czero); if (o == NULL || checkType(c,o,OBJ_LIST)) return; addReplyLongLong(c,listTypeLength(o)); } +/* LINDEX */ void lindexCommand(client *c) { robj *o = lookupKeyReadOrReply(c,c->argv[1],shared.null[c->resp]); if (o == NULL || checkType(c,o,OBJ_LIST)) return; @@ -359,6 +368,7 @@ void lindexCommand(client *c) { } } +/* LSET */ void lsetCommand(client *c) { robj *o = lookupKeyWriteOrReply(c,c->argv[1],shared.nokeyerr); if (o == NULL || checkType(c,o,OBJ_LIST)) return; @@ -385,7 +395,53 @@ void lsetCommand(client *c) { } } -void listElementsRemoved(client *c, robj *key, int where, robj *o) { +/* A helper for replying with a list's range between the inclusive start and end + * indexes as multi-bulk, with support for negative indexes. Note that start + * must be less than end or an empty array is returned. When the reverse + * argument is set to a non-zero value, the reply is reversed so that elements + * are returned from end to start. */ +void addListRangeReply(client *c, robj *o, long start, long end, int reverse) { + long rangelen, llen = listTypeLength(o); + + /* Convert negative indexes. */ + if (start < 0) start = llen+start; + if (end < 0) end = llen+end; + if (start < 0) start = 0; + + /* Invariant: start >= 0, so this test will be true when end < 0. + * The range is empty when start > end or start >= length. */ + if (start > end || start >= llen) { + addReply(c,shared.emptyarray); + return; + } + if (end >= llen) end = llen-1; + rangelen = (end-start)+1; + + /* Return the result in form of a multi-bulk reply */ + addReplyArrayLen(c,rangelen); + if (o->encoding == OBJ_ENCODING_QUICKLIST) { + int from = reverse ? end : start; + int direction = reverse ? LIST_HEAD : LIST_TAIL; + listTypeIterator *iter = listTypeInitIterator(o,from,direction); + + while(rangelen--) { + listTypeEntry entry; + listTypeNext(iter, &entry); + quicklistEntry *qe = &entry.entry; + if (qe->value) { + addReplyBulkCBuffer(c,qe->value,qe->sz); + } else { + addReplyBulkLongLong(c,qe->longval); + } + } + listTypeReleaseIterator(iter); + } else { + serverPanic("Unknown list encoding"); + } +} + +/* A housekeeping helper for list elements popping tasks. */ +void listElementsRemoved(client *c, robj *key, int where, robj *o, long count) { char *event = (where == LIST_HEAD) ? "lpop" : "rpop"; notifyKeyspaceEvent(NOTIFY_LIST, event, key, c->db->id); @@ -394,78 +450,84 @@ void listElementsRemoved(client *c, robj *key, int where, robj *o) { dbDelete(c->db, key); } signalModifiedKey(c, c->db, key); - server.dirty++; + server.dirty += count; } +/* Implements the generic list pop operation for LPOP/RPOP. + * The where argument specifies which end of the list is operated on. An + * optional count may be provided as the third argument of the client's + * command. */ void popGenericCommand(client *c, int where) { + long count = 0; + robj *value; + + if (c->argc > 3) { + addReplyErrorFormat(c,"wrong number of arguments for '%s' command", + c->cmd->name); + return; + } else if (c->argc == 3) { + /* Parse the optional count argument. */ + if (getPositiveLongFromObjectOrReply(c,c->argv[2],&count,NULL) != C_OK) + return; + if (count == 0) { + /* Fast exit path. */ + addReplyNullArray(c); + return; + } + } + robj *o = lookupKeyWriteOrReply(c, c->argv[1], shared.null[c->resp]); if (o == NULL || checkType(c, o, OBJ_LIST)) return; - robj *value = listTypePop(o, where); - if (value == NULL) { - addReplyNull(c); - } else { + if (!count) { + /* Pop a single element. This is POP's original behavior that replies + * with a bulk string. */ + value = listTypePop(o,where); + serverAssert(value != NULL); addReplyBulk(c,value); decrRefCount(value); - listElementsRemoved(c,c->argv[1],where,o); + listElementsRemoved(c,c->argv[1],where,o,1); + } else { + /* Pop a range of elements. An addition to the original POP command, + * which replies with a multi-bulk. */ + long llen = listTypeLength(o); + long rangelen = (count > llen) ? llen : count; + long rangestart = (where == LIST_HEAD) ? 0 : -rangelen; + long rangeend = (where == LIST_HEAD) ? rangelen - 1 : -1; + int reverse = (where == LIST_HEAD) ? 0 : 1; + + addListRangeReply(c,o,rangestart,rangeend,reverse); + quicklistDelRange(o->ptr,rangestart,rangelen); + listElementsRemoved(c,c->argv[1],where,o,rangelen); } } +/* LPOP [count] */ void lpopCommand(client *c) { popGenericCommand(c,LIST_HEAD); } +/* RPOP [count] */ void rpopCommand(client *c) { popGenericCommand(c,LIST_TAIL); } +/* LRANGE */ void lrangeCommand(client *c) { robj *o; - long start, end, llen, rangelen; + long start, end; if ((getLongFromObjectOrReply(c, c->argv[2], &start, NULL) != C_OK) || (getLongFromObjectOrReply(c, c->argv[3], &end, NULL) != C_OK)) return; if ((o = lookupKeyReadOrReply(c,c->argv[1],shared.emptyarray)) == NULL || checkType(c,o,OBJ_LIST)) return; - llen = listTypeLength(o); - - /* convert negative indexes */ - if (start < 0) start = llen+start; - if (end < 0) end = llen+end; - if (start < 0) start = 0; - - /* Invariant: start >= 0, so this test will be true when end < 0. - * The range is empty when start > end or start >= length. */ - if (start > end || start >= llen) { - addReply(c,shared.emptyarray); - return; - } - if (end >= llen) end = llen-1; - rangelen = (end-start)+1; - - /* Return the result in form of a multi-bulk reply */ - addReplyArrayLen(c,rangelen); - if (o->encoding == OBJ_ENCODING_QUICKLIST) { - listTypeIterator *iter = listTypeInitIterator(o, start, LIST_TAIL); - while(rangelen--) { - listTypeEntry entry; - listTypeNext(iter, &entry); - quicklistEntry *qe = &entry.entry; - if (qe->value) { - addReplyBulkCBuffer(c,qe->value,qe->sz); - } else { - addReplyBulkLongLong(c,qe->longval); - } - } - listTypeReleaseIterator(iter); - } else { - serverPanic("List encoding is not QUICKLIST!"); - } + addListRangeReply(c,o,start,end,0); } +/* LTRIM */ void ltrimCommand(client *c) { robj *o; long start, end, llen, ltrim, rtrim; @@ -629,6 +691,7 @@ void lposCommand(client *c) { } } +/* LREM */ void lremCommand(client *c) { robj *subject, *obj; obj = c->argv[3]; @@ -756,6 +819,7 @@ void lmoveGenericCommand(client *c, int wherefrom, int whereto) { } } +/* LMOVE (LEFT|RIGHT) (LEFT|RIGHT) */ void lmoveCommand(client *c) { int wherefrom, whereto; if (getListPositionFromObjectOrReply(c,c->argv[3],&wherefrom) @@ -888,7 +952,7 @@ void blockingPopGenericCommand(client *c, int where) { addReplyBulk(c,c->argv[j]); addReplyBulk(c,value); decrRefCount(value); - listElementsRemoved(c,c->argv[j],where,o); + listElementsRemoved(c,c->argv[j],where,o,1); /* Replicate it as an [LR]POP instead of B[LR]POP. */ rewriteClientCommandVector(c,2, @@ -912,10 +976,12 @@ void blockingPopGenericCommand(client *c, int where) { blockForKeys(c,BLOCKED_LIST,c->argv + 1,c->argc - 2,timeout,NULL,&pos,NULL); } +/* BLPOP [ ...] */ void blpopCommand(client *c) { blockingPopGenericCommand(c,LIST_HEAD); } +/* BLPOP [ ...] */ void brpopCommand(client *c) { blockingPopGenericCommand(c,LIST_TAIL); } @@ -942,6 +1008,7 @@ void blmoveGenericCommand(client *c, int wherefrom, int whereto, mstime_t timeou } } +/* BLMOVE (LEFT|RIGHT) (LEFT|RIGHT) */ void blmoveCommand(client *c) { mstime_t timeout; int wherefrom, whereto; @@ -954,6 +1021,7 @@ void blmoveCommand(client *c) { blmoveGenericCommand(c,wherefrom,whereto,timeout); } +/* BRPOPLPUSH */ void brpoplpushCommand(client *c) { mstime_t timeout; if (getTimeoutFromObjectOrReply(c,c->argv[3],&timeout,UNIT_SECONDS) diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index 61ca23377..9be5dd93b 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -123,6 +123,17 @@ start_server { test {R/LPOP against empty list} { r lpop non-existing-list } {} + + test {R/LPOP with the optional count argument} { + assert_equal 7 [r lpush listcount aa bb cc dd ee ff gg] + assert_equal {} [r lpop listcount 0] + assert_equal {gg} [r lpop listcount 1] + assert_equal {ff ee} [r lpop listcount 2] + assert_equal {aa bb} [r rpop listcount 2] + assert_equal {cc} [r rpop listcount 1] + assert_equal {dd} [r rpop listcount 123] + assert_error "*ERR*range*" {r lpop forbarqaz -123} + } test {Variadic RPUSH/LPUSH} { r del mylist @@ -947,6 +958,12 @@ start_server { assert_equal {} [r lrange nosuchkey 0 1] } + test {LRANGE with start > end yields an empty array for backward compatibility} { + create_list mylist "1 2 3" + assert_equal {} [r lrange mylist 1 0] + assert_equal {} [r lrange mylist -1 -2] + } + foreach {type large} [array get largevalue] { proc trim_list {type min max} { upvar 1 large large -- cgit v1.2.1 From 299f9ebffa5d0a79245a6e989cde775daf30d6a8 Mon Sep 17 00:00:00 2001 From: "zhaozhao.zz" <276441700@qq.com> Date: Sun, 27 Dec 2020 19:14:39 +0800 Subject: Tracking: add CLIENT TRACKINGINFO subcommand (#7309) Add CLIENT TRACKINGINFO subcommand Co-authored-by: Oran Agra --- src/networking.c | 64 ++++++++++++++++++++++++++++++++++++++- tests/unit/tracking.tcl | 79 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index ef1210b99..4f72e144a 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2384,9 +2384,10 @@ void clientCommand(client *c) { "REPLY (on|off|skip) -- Control the replies sent to the current connection.", "SETNAME -- Assign the name to the current connection.", "UNBLOCK [TIMEOUT|ERROR] -- Unblock the specified blocked client.", -"TRACKING (on|off) [REDIRECT ] [BCAST] [PREFIX first] [PREFIX second] [OPTIN] [OPTOUT]... -- Enable client keys tracking for client side caching.", +"TRACKING (on|off) [REDIRECT ] [BCAST] [PREFIX first] [PREFIX second] [OPTIN] [OPTOUT] [NOLOOP]... -- Enable client keys tracking for client side caching.", "CACHING (yes|no) -- Enable/Disable tracking of the keys for next command in OPTIN/OPTOUT mode.", "GETREDIR -- Return the client ID we are redirecting to when tracking is enabled.", +"TRACKINGINFO -- Return information about current client's tracking status.", NULL }; addReplyHelp(c, help); @@ -2753,6 +2754,67 @@ NULL } else { addReplyLongLong(c,-1); } + } else if (!strcasecmp(c->argv[1]->ptr,"trackinginfo") && c->argc == 2) { + addReplyMapLen(c,3); + + /* Flags */ + addReplyBulkCString(c,"flags"); + void *arraylen_ptr = addReplyDeferredLen(c); + int numflags = 0; + addReplyBulkCString(c,c->flags & CLIENT_TRACKING ? "on" : "off"); + numflags++; + if (c->flags & CLIENT_TRACKING_BCAST) { + addReplyBulkCString(c,"bcast"); + numflags++; + } + if (c->flags & CLIENT_TRACKING_OPTIN) { + addReplyBulkCString(c,"optin"); + numflags++; + if (c->flags & CLIENT_TRACKING_CACHING) { + addReplyBulkCString(c,"caching-yes"); + numflags++; + } + } + if (c->flags & CLIENT_TRACKING_OPTOUT) { + addReplyBulkCString(c,"optout"); + numflags++; + if (c->flags & CLIENT_TRACKING_CACHING) { + addReplyBulkCString(c,"caching-no"); + numflags++; + } + } + if (c->flags & CLIENT_TRACKING_NOLOOP) { + addReplyBulkCString(c,"noloop"); + numflags++; + } + if (c->flags & CLIENT_TRACKING_BROKEN_REDIR) { + addReplyBulkCString(c,"broken_redirect"); + numflags++; + } + setDeferredSetLen(c,arraylen_ptr,numflags); + + /* Redirect */ + addReplyBulkCString(c,"redirect"); + if (c->flags & CLIENT_TRACKING) { + addReplyLongLong(c,c->client_tracking_redirection); + } else { + addReplyLongLong(c,-1); + } + + /* Prefixes */ + addReplyBulkCString(c,"prefixes"); + if (c->client_tracking_prefixes) { + addReplyArrayLen(c,raxSize(c->client_tracking_prefixes)); + raxIterator ri; + raxStart(&ri,c->client_tracking_prefixes); + raxSeek(&ri,"^",NULL,0); + while(raxNext(&ri)) { + addReplyBulkCBuffer(c,ri.key,ri.key_len); + } + raxStop(&ri); + } else { + addReplyArrayLen(c,0); + } } else { addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try CLIENT HELP", (char*)c->argv[1]->ptr); } diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index fc2800791..555c4e75e 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -398,6 +398,85 @@ start_server {tags {"tracking"}} { assert {$total_prefixes == 1} } + test {CLIENT TRACKINGINFO provides reasonable results when tracking off} { + r CLIENT TRACKING off + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {off} $flags + set redirect [dict get $res redirect] + assert_equal {-1} $redirect + set prefixes [dict get $res prefixes] + assert_equal {} $prefixes + } + + test {CLIENT TRACKINGINFO provides reasonable results when tracking on} { + r CLIENT TRACKING on + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on} $flags + set redirect [dict get $res redirect] + assert_equal {0} $redirect + set prefixes [dict get $res prefixes] + assert_equal {} $prefixes + } + + test {CLIENT TRACKINGINFO provides reasonable results when tracking on with options} { + r CLIENT TRACKING on REDIRECT $redir_id noloop + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on noloop} $flags + set redirect [dict get $res redirect] + assert_equal $redir_id $redirect + set prefixes [dict get $res prefixes] + assert_equal {} $prefixes + } + + test {CLIENT TRACKINGINFO provides reasonable results when tracking optin} { + r CLIENT TRACKING off + r CLIENT TRACKING on optin + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on optin} $flags + set redirect [dict get $res redirect] + assert_equal {0} $redirect + set prefixes [dict get $res prefixes] + assert_equal {} $prefixes + + r CLIENT CACHING yes + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on optin caching-yes} $flags + } + + test {CLIENT TRACKINGINFO provides reasonable results when tracking optout} { + r CLIENT TRACKING off + r CLIENT TRACKING on optout + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on optout} $flags + set redirect [dict get $res redirect] + assert_equal {0} $redirect + set prefixes [dict get $res prefixes] + assert_equal {} $prefixes + + r CLIENT CACHING no + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on optout caching-no} $flags + } + + test {CLIENT TRACKINGINFO provides reasonable results when tracking bcast mode} { + r CLIENT TRACKING off + r CLIENT TRACKING on BCAST PREFIX foo PREFIX bar + set res [r client trackinginfo] + set flags [dict get $res flags] + assert_equal {on bcast} $flags + set redirect [dict get $res redirect] + assert_equal {0} $redirect + set prefixes [lsort [dict get $res prefixes]] + assert_equal {bar foo} $prefixes + } + $rd_redirection close $rd close } -- cgit v1.2.1 From 049cf8cdf4e9e0abecf137dc1e3362089439f414 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 27 Dec 2020 21:40:12 +0200 Subject: Fix memory leaks in error replies due to recent change (#8249) Recently efaf09ee4 started using addReplyErrorSds in place of addReplySds the later takes ownership of the string but the former did not. This introduced memory leaks when a script returns an error to redis, and also in clusterRedirectClient (two new usages of addReplyErrorSds which was mostly unused till now. This commit chagnes two thanks. 1. change addReplyErrorSds to take ownership of the error string. 2. scripting.c doesn't actually need to use addReplyErrorSds, it's a perfect match for addReplyErrorFormat (replaces newlines with spaces) --- src/networking.c | 2 ++ src/scripting.c | 5 +---- src/server.c | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/networking.c b/src/networking.c index 24a601630..34e8b8481 100644 --- a/src/networking.c +++ b/src/networking.c @@ -458,9 +458,11 @@ void addReplyError(client *c, const char *err) { } /* See addReplyErrorLength for expectations from the input string. */ +/* As a side effect the SDS string is freed. */ void addReplyErrorSds(client *c, sds err) { addReplyErrorLength(c,err,sdslen(err)); afterErrorReply(c,err,sdslen(err)); + sdsfree(err); } /* See addReplyErrorLength for expectations from the formatted string. diff --git a/src/scripting.c b/src/scripting.c index 2ac8268ed..e75892046 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -366,10 +366,7 @@ void luaReplyToRedisReply(client *c, lua_State *lua) { lua_gettable(lua,-2); t = lua_type(lua,-1); if (t == LUA_TSTRING) { - sds err = sdsnew(lua_tostring(lua,-1)); - sdsmapchars(err,"\r\n"," ",2); - addReplyErrorSds(c,sdscatprintf(sdsempty(),"-%s",err)); - sdsfree(err); + addReplyErrorFormat(c,"-%s",lua_tostring(lua,-1)); lua_pop(lua,2); return; } diff --git a/src/server.c b/src/server.c index 90f669bdb..257a39f71 100644 --- a/src/server.c +++ b/src/server.c @@ -3689,10 +3689,11 @@ void rejectCommandFormat(client *c, const char *fmt, ...) { sdsmapchars(s, "\r\n", " ", 2); if (c->cmd && c->cmd->proc == execCommand) { execCommandAbort(c, s); + sdsfree(s); } else { + /* The following frees 's'. */ addReplyErrorSds(c, s); } - sdsfree(s); } /* Returns 1 for commands that may have key names in their arguments, but have -- cgit v1.2.1 From 1f5a73a530915f6f6326047effc796218af22cf6 Mon Sep 17 00:00:00 2001 From: sundb Date: Wed, 30 Dec 2020 14:37:37 +0800 Subject: Merge pushGenericCommand and pushxGenericCommand (#8255) Merge pushGenericCommand and pushxGenericCommand to remove redundancy and simplify code. --- src/t_list.c | 73 ++++++++++++++++++++++-------------------------------------- 1 file changed, 26 insertions(+), 47 deletions(-) diff --git a/src/t_list.c b/src/t_list.c index b64b37a8a..f019a7ec0 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -216,76 +216,55 @@ robj *listTypeDup(robj *o) { * List Commands *----------------------------------------------------------------------------*/ -/* Implements LPUSH/RPUSH. */ -void pushGenericCommand(client *c, int where) { - int j, pushed = 0; - robj *lobj = lookupKeyWrite(c->db,c->argv[1]); +/* Implements LPUSH/RPUSH/LPUSHX/RPUSHX. + * 'xx': push if key exists. */ +void pushGenericCommand(client *c, int where, int xx) { + int j; - if (checkType(c,lobj,OBJ_LIST)) { - return; + robj *lobj = lookupKeyWrite(c->db, c->argv[1]); + if (checkType(c,lobj,OBJ_LIST)) return; + if (!lobj) { + if (xx) { + addReply(c, shared.czero); + return; + } + + lobj = createQuicklistObject(); + quicklistSetOptions(lobj->ptr, server.list_max_ziplist_size, + server.list_compress_depth); + dbAdd(c->db,c->argv[1],lobj); } for (j = 2; j < c->argc; j++) { - if (!lobj) { - lobj = createQuicklistObject(); - quicklistSetOptions(lobj->ptr, server.list_max_ziplist_size, - server.list_compress_depth); - dbAdd(c->db,c->argv[1],lobj); - } listTypePush(lobj,c->argv[j],where); - pushed++; + server.dirty++; } - addReplyLongLong(c, (lobj ? listTypeLength(lobj) : 0)); - if (pushed) { - char *event = (where == LIST_HEAD) ? "lpush" : "rpush"; - signalModifiedKey(c,c->db,c->argv[1]); - notifyKeyspaceEvent(NOTIFY_LIST,event,c->argv[1],c->db->id); - } - server.dirty += pushed; + addReplyLongLong(c, listTypeLength(lobj)); + + char *event = (where == LIST_HEAD) ? "lpush" : "rpush"; + signalModifiedKey(c,c->db,c->argv[1]); + notifyKeyspaceEvent(NOTIFY_LIST,event,c->argv[1],c->db->id); } /* LPUSH [ ...] */ void lpushCommand(client *c) { - pushGenericCommand(c,LIST_HEAD); + pushGenericCommand(c,LIST_HEAD,0); } /* RPUSH [ ...] */ void rpushCommand(client *c) { - pushGenericCommand(c,LIST_TAIL); -} - -/* Implements LPUSHX/RPUSHX. */ -void pushxGenericCommand(client *c, int where) { - int j, pushed = 0; - robj *subject; - - if ((subject = lookupKeyWriteOrReply(c,c->argv[1],shared.czero)) == NULL || - checkType(c,subject,OBJ_LIST)) return; - - for (j = 2; j < c->argc; j++) { - listTypePush(subject,c->argv[j],where); - pushed++; - } - - addReplyLongLong(c,listTypeLength(subject)); - - if (pushed) { - char *event = (where == LIST_HEAD) ? "lpush" : "rpush"; - signalModifiedKey(c,c->db,c->argv[1]); - notifyKeyspaceEvent(NOTIFY_LIST,event,c->argv[1],c->db->id); - } - server.dirty += pushed; + pushGenericCommand(c,LIST_TAIL,0); } /* LPUSHX [ ...] */ void lpushxCommand(client *c) { - pushxGenericCommand(c,LIST_HEAD); + pushGenericCommand(c,LIST_HEAD,1); } /* RPUSH [ ...] */ void rpushxCommand(client *c) { - pushxGenericCommand(c,LIST_TAIL); + pushGenericCommand(c,LIST_TAIL,1); } /* LINSERT (BEFORE|AFTER) */ -- cgit v1.2.1 From 90b9f08e5d1657e7bfffe43f31f6663bf469ee75 Mon Sep 17 00:00:00 2001 From: filipe oliveira Date: Thu, 31 Dec 2020 14:53:43 +0000 Subject: Add errorstats info section, Add failed_calls and rejected_calls to commandstats (#8217) This Commit pushes forward the observability on overall error statistics and command statistics within redis-server: It extends INFO COMMANDSTATS to have - failed_calls in - so we can keep track of errors that happen from the command itself, broken by command. - rejected_calls - so we can keep track of errors that were triggered outside the commmand processing per se Adds a new section to INFO, named ERRORSTATS that enables keeping track of the different errors that occur within redis ( within processCommand and call ) based on the reply Error Prefix ( The first word after the "-", up to the first space ). This commit also fixes RM_ReplyWithError so that it can be correctly identified as an error reply. --- src/config.c | 1 + src/module.c | 24 +++---- src/networking.c | 18 +++++ src/server.c | 63 ++++++++++++++++- src/server.h | 10 ++- tests/cluster/tests/18-info.tcl | 36 ++++++++++ tests/support/util.tcl | 6 ++ tests/test_helper.tcl | 1 + tests/unit/info.tcl | 153 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 294 insertions(+), 18 deletions(-) create mode 100644 tests/cluster/tests/18-info.tcl create mode 100644 tests/unit/info.tcl diff --git a/src/config.c b/src/config.c index a92b41624..9d078bfe3 100644 --- a/src/config.c +++ b/src/config.c @@ -2560,6 +2560,7 @@ NULL } else if (!strcasecmp(c->argv[1]->ptr,"resetstat") && c->argc == 2) { resetServerStats(); resetCommandTableStats(); + resetErrorTableStats(); addReply(c,shared.ok); } else if (!strcasecmp(c->argv[1]->ptr,"rewrite") && c->argc == 2) { if (server.configfile == NULL) { diff --git a/src/module.c b/src/module.c index 11f5f4489..913b4de5d 100644 --- a/src/module.c +++ b/src/module.c @@ -1368,18 +1368,6 @@ int RM_ReplyWithLongLong(RedisModuleCtx *ctx, long long ll) { return REDISMODULE_OK; } -/* Reply with an error or simple string (status message). Used to implement - * ReplyWithSimpleString() and ReplyWithError(). - * The function always returns REDISMODULE_OK. */ -int replyWithStatus(RedisModuleCtx *ctx, const char *msg, char *prefix) { - client *c = moduleGetReplyClient(ctx); - if (c == NULL) return REDISMODULE_OK; - addReplyProto(c,prefix,strlen(prefix)); - addReplyProto(c,msg,strlen(msg)); - addReplyProto(c,"\r\n",2); - return REDISMODULE_OK; -} - /* Reply with the error 'err'. * * Note that 'err' must contain all the error, including @@ -1395,7 +1383,10 @@ int replyWithStatus(RedisModuleCtx *ctx, const char *msg, char *prefix) { * The function always returns REDISMODULE_OK. */ int RM_ReplyWithError(RedisModuleCtx *ctx, const char *err) { - return replyWithStatus(ctx,err,"-"); + client *c = moduleGetReplyClient(ctx); + if (c == NULL) return REDISMODULE_OK; + addReplyErrorFormat(c,"-%s",err); + return REDISMODULE_OK; } /* Reply with a simple string (+... \r\n in RESP protocol). This replies @@ -1404,7 +1395,12 @@ int RM_ReplyWithError(RedisModuleCtx *ctx, const char *err) { * * The function always returns REDISMODULE_OK. */ int RM_ReplyWithSimpleString(RedisModuleCtx *ctx, const char *msg) { - return replyWithStatus(ctx,msg,"+"); + client *c = moduleGetReplyClient(ctx); + if (c == NULL) return REDISMODULE_OK; + addReplyProto(c,"+",1); + addReplyProto(c,msg,strlen(msg)); + addReplyProto(c,"\r\n",2); + return REDISMODULE_OK; } /* Reply with an array type of 'len' elements. However 'len' other calls diff --git a/src/networking.c b/src/networking.c index 34e8b8481..7957cc82f 100644 --- a/src/networking.c +++ b/src/networking.c @@ -405,6 +405,24 @@ void addReplyErrorLength(client *c, const char *s, size_t len) { /* Do some actions after an error reply was sent (Log if needed, updates stats, etc.) */ void afterErrorReply(client *c, const char *s, size_t len) { + /* Increment the global error counter */ + server.stat_total_error_replies++; + /* Increment the error stats + * If the string already starts with "-..." then the error prefix + * is provided by the caller ( we limit the search to 32 chars). Otherwise we use "-ERR". */ + if (s[0] != '-') { + incrementErrorCount("ERR", 3); + } else { + char *spaceloc = memchr(s, ' ', len < 32 ? len : 32); + if (spaceloc) { + const size_t errEndPos = (size_t)(spaceloc - s); + incrementErrorCount(s+1, errEndPos-1); + } else { + /* Fallback to ERR if we can't retrieve the error prefix */ + incrementErrorCount("ERR", 3); + } + } + /* Sometimes it could be normal that a slave replies to a master with * an error and this function gets called. Actually the error will never * be sent because addReply*() against master clients has no effect... diff --git a/src/server.c b/src/server.c index 257a39f71..dbcaa767d 100644 --- a/src/server.c +++ b/src/server.c @@ -2952,6 +2952,7 @@ void resetServerStats(void) { atomicSet(server.stat_net_input_bytes, 0); atomicSet(server.stat_net_output_bytes, 0); server.stat_unexpected_error_replies = 0; + server.stat_total_error_replies = 0; server.stat_dump_payload_sanitizations = 0; server.aof_delayed_fsync = 0; server.blocked_last_cron = 0; @@ -2985,6 +2986,7 @@ void initServer(void) { server.in_fork_child = CHILD_TYPE_NONE; server.main_thread_id = pthread_self(); server.current_client = NULL; + server.errors = raxNew(); server.fixed_time_expire = 0; server.clients = listCreate(); server.clients_index = raxNew(); @@ -3291,11 +3293,18 @@ void resetCommandTableStats(void) { c = (struct redisCommand *) dictGetVal(de); c->microseconds = 0; c->calls = 0; + c->rejected_calls = 0; + c->failed_calls = 0; } dictReleaseIterator(di); } +void resetErrorTableStats(void) { + raxFree(server.errors); + server.errors = raxNew(); +} + /* ========================== Redis OP Array API ============================ */ void redisOpArrayInit(redisOpArray *oa) { @@ -3490,6 +3499,7 @@ void call(client *c, int flags) { ustime_t start, duration; int client_old_flags = c->flags; struct redisCommand *real_cmd = c->cmd; + static long long prev_err_count; server.fixed_time_expire++; @@ -3510,6 +3520,7 @@ void call(client *c, int flags) { /* Call the command. */ dirty = server.dirty; + prev_err_count = server.stat_total_error_replies; updateCachedTime(0); start = server.ustime; c->cmd->proc(c); @@ -3517,6 +3528,14 @@ void call(client *c, int flags) { dirty = server.dirty-dirty; if (dirty < 0) dirty = 0; + /* Update failed command calls if required. + * We leverage a static variable (prev_err_count) to retain + * the counter across nested function calls and avoid logging + * the same error twice. */ + if ((server.stat_total_error_replies - prev_err_count) > 0) { + real_cmd->failed_calls++; + } + /* After executing command, we will close the client after writing entire * reply if it is set 'CLIENT_CLOSE_AFTER_COMMAND' flag. */ if (c->flags & CLIENT_CLOSE_AFTER_COMMAND) { @@ -3655,6 +3674,7 @@ void call(client *c, int flags) { server.fixed_time_expire--; server.stat_numcommands++; + prev_err_count = server.stat_total_error_replies; /* Record peak memory after each command and before the eviction that runs * before the next command. */ @@ -3670,6 +3690,7 @@ void call(client *c, int flags) { * Note: 'reply' is expected to end with \r\n */ void rejectCommand(client *c, robj *reply) { flagTransaction(c); + if (c->cmd) c->cmd->rejected_calls++; if (c->cmd && c->cmd->proc == execCommand) { execCommandAbort(c, reply->ptr); } else { @@ -3679,6 +3700,7 @@ void rejectCommand(client *c, robj *reply) { } void rejectCommandFormat(client *c, const char *fmt, ...) { + if (c->cmd) c->cmd->rejected_calls++; flagTransaction(c); va_list ap; va_start(ap,fmt); @@ -3805,6 +3827,7 @@ int processCommand(client *c) { flagTransaction(c); } clusterRedirectClient(c,n,hashslot,error_code); + c->cmd->rejected_calls++; return C_OK; } } @@ -3962,9 +3985,22 @@ int processCommand(client *c) { if (listLength(server.ready_keys)) handleClientsBlockedOnKeys(); } + return C_OK; } +/* ====================== Error lookup and execution ===================== */ + +void incrementErrorCount(const char *fullerr, size_t namelen) { + struct redisError *error = raxFind(server.errors,(unsigned char*)fullerr,namelen); + if (error == raxNotFound) { + error = zmalloc(sizeof(*error)); + error->count = 0; + raxInsert(server.errors,(unsigned char*)fullerr,namelen,error,NULL); + } + error->count++; +} + /*================================== Shutdown =============================== */ /* Close listening sockets. Also unlink the unix domain socket if @@ -4681,6 +4717,7 @@ sds genRedisInfoString(const char *section) { "tracking_total_items:%lld\r\n" "tracking_total_prefixes:%lld\r\n" "unexpected_error_replies:%lld\r\n" + "total_error_replies:%lld\r\n" "dump_payload_sanitizations:%lld\r\n" "total_reads_processed:%lld\r\n" "total_writes_processed:%lld\r\n" @@ -4718,6 +4755,7 @@ sds genRedisInfoString(const char *section) { (unsigned long long) trackingGetTotalItems(), (unsigned long long) trackingGetTotalPrefixes(), server.stat_unexpected_error_replies, + server.stat_total_error_replies, server.stat_dump_payload_sanitizations, stat_total_reads_processed, stat_total_writes_processed, @@ -4908,14 +4946,33 @@ sds genRedisInfoString(const char *section) { di = dictGetSafeIterator(server.commands); while((de = dictNext(di)) != NULL) { c = (struct redisCommand *) dictGetVal(de); - if (!c->calls) continue; + if (!c->calls && !c->failed_calls && !c->rejected_calls) + continue; info = sdscatprintf(info, - "cmdstat_%s:calls=%lld,usec=%lld,usec_per_call=%.2f\r\n", + "cmdstat_%s:calls=%lld,usec=%lld,usec_per_call=%.2f" + ",rejected_calls=%lld,failed_calls=%lld\r\n", c->name, c->calls, c->microseconds, - (c->calls == 0) ? 0 : ((float)c->microseconds/c->calls)); + (c->calls == 0) ? 0 : ((float)c->microseconds/c->calls), + c->rejected_calls, c->failed_calls); } dictReleaseIterator(di); } + /* Error statistics */ + if (allsections || defsections || !strcasecmp(section,"errorstats")) { + if (sections++) info = sdscat(info,"\r\n"); + info = sdscat(info, "# Errorstats\r\n"); + raxIterator ri; + raxStart(&ri,server.errors); + raxSeek(&ri,"^",NULL,0); + struct redisError *e; + while(raxNext(&ri)) { + e = (struct redisError *) ri.data; + info = sdscatprintf(info, + "errorstat_%.*s:count=%lld\r\n", + (int)ri.key_len, ri.key, e->count); + } + raxStop(&ri); + } /* Cluster */ if (allsections || defsections || !strcasecmp(section,"cluster")) { diff --git a/src/server.h b/src/server.h index 1dc761959..5f6b21ec4 100644 --- a/src/server.h +++ b/src/server.h @@ -1122,6 +1122,7 @@ struct redisServer { dict *commands; /* Command table */ dict *orig_commands; /* Command table before command renaming. */ aeEventLoop *el; + rax *errors; /* Errors table */ redisAtomic unsigned int lruclock; /* Clock for LRU eviction */ volatile sig_atomic_t shutdown_asap; /* SHUTDOWN needed ASAP */ int activerehashing; /* Incremental rehash in serverCron() */ @@ -1231,6 +1232,7 @@ struct redisServer { size_t stat_module_cow_bytes; /* Copy on write bytes during module fork. */ uint64_t stat_clients_type_memory[CLIENT_TYPE_COUNT];/* Mem usage by type */ long long stat_unexpected_error_replies; /* Number of unexpected (aof-loading, replica to master, etc.) error replies */ + long long stat_total_error_replies; /* Total number of issued error replies ( command + rejected errors ) */ long long stat_dump_payload_sanitizations; /* Number deep dump payloads integrity validations. */ long long stat_io_reads_processed; /* Number of read events processed by IO / Main threads */ long long stat_io_writes_processed; /* Number of write events processed by IO / Main threads */ @@ -1579,7 +1581,7 @@ struct redisCommand { int firstkey; /* The first argument that's a key (0 = no keys) */ int lastkey; /* The last argument that's a key */ int keystep; /* The step between first and last key */ - long long microseconds, calls; + long long microseconds, calls, rejected_calls, failed_calls; int id; /* Command ID. This is a progressive ID starting from 0 that is assigned at runtime, and is used in order to check ACLs. A connection is able to execute a given command if @@ -1587,6 +1589,10 @@ struct redisCommand { bit set in the bitmap of allowed commands. */ }; +struct redisError { + long long count; +}; + struct redisFunctionSym { char *name; unsigned long pointer; @@ -2132,7 +2138,9 @@ void updateDictResizePolicy(void); int htNeedsResize(dict *dict); void populateCommandTable(void); void resetCommandTableStats(void); +void resetErrorTableStats(void); void adjustOpenFilesLimit(void); +void incrementErrorCount(const char *fullerr, size_t namelen); void closeListeningSockets(int unlink_unix_socket); void updateCachedTime(int update_daylight_info); void resetServerStats(void); diff --git a/tests/cluster/tests/18-info.tcl b/tests/cluster/tests/18-info.tcl new file mode 100644 index 000000000..83dbf833f --- /dev/null +++ b/tests/cluster/tests/18-info.tcl @@ -0,0 +1,36 @@ +# Check cluster info stats + +source "../tests/includes/init-tests.tcl" + +test "Create a primary with a replica" { + create_cluster 2 0 +} + +test "Cluster should start ok" { + assert_cluster_state ok +} + +set primary1 [Rn 0] +set primary2 [Rn 1] + +proc cmdstat {instace cmd} { + return [cmdrstat $cmd $instace] +} + +proc errorstat {instace cmd} { + return [errorrstat $cmd $instace] +} + +test "errorstats: rejected call due to MOVED Redirection" { + $primary1 config resetstat + $primary2 config resetstat + assert_match {} [errorstat $primary1 MOVED] + assert_match {} [errorstat $primary2 MOVED] + # we know that the primary 2 will have a MOVED reply + catch {$primary1 set key{0x0000} b} replyP1 + catch {$primary2 set key{0x0000} b} replyP2 + assert_match {OK} $replyP1 + assert_match {} [errorstat $primary1 MOVED] + assert_match {*count=1*} [errorstat $primary2 MOVED] + assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat $primary2 set] +} diff --git a/tests/support/util.tcl b/tests/support/util.tcl index e4b70ed20..86f2753c2 100644 --- a/tests/support/util.tcl +++ b/tests/support/util.tcl @@ -561,6 +561,12 @@ proc cmdrstat {cmd r} { } } +proc errorrstat {cmd r} { + if {[regexp "\r\nerrorstat_$cmd:(.*?)\r\n" [$r info errorstats] _ value]} { + set _ $value + } +} + proc generate_fuzzy_traffic_on_key {key duration} { # Commands per type, blocking commands removed # TODO: extract these from help.h or elsewhere, and improve to include other types diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 3b8dc16da..4bef921ff 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -18,6 +18,7 @@ set ::all_tests { unit/protocol unit/keyspace unit/scan + unit/info unit/type/string unit/type/incr unit/type/list diff --git a/tests/unit/info.tcl b/tests/unit/info.tcl new file mode 100644 index 000000000..5a44c0647 --- /dev/null +++ b/tests/unit/info.tcl @@ -0,0 +1,153 @@ +proc cmdstat {cmd} { + return [cmdrstat $cmd r] +} + +proc errorstat {cmd} { + return [errorrstat $cmd r] +} + +start_server {tags {"info"}} { + start_server {} { + + test {errorstats: failed call authentication error} { + r config resetstat + assert_match {} [errorstat ERR] + assert_equal [s total_error_replies] 0 + catch {r auth k} e + assert_match {ERR AUTH*} $e + assert_match {*count=1*} [errorstat ERR] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=1} [cmdstat auth] + assert_equal [s total_error_replies] 1 + r config resetstat + assert_match {} [errorstat ERR] + } + + test {errorstats: failed call within MULTI/EXEC} { + r config resetstat + assert_match {} [errorstat ERR] + assert_equal [s total_error_replies] 0 + r multi + r set a b + r auth a + catch {r exec} e + assert_match {ERR AUTH*} $e + assert_match {*count=1*} [errorstat ERR] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=0} [cmdstat set] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=1} [cmdstat auth] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=0} [cmdstat exec] + assert_equal [s total_error_replies] 1 + + # MULTI/EXEC command errors should still be pinpointed to him + catch {r exec} e + assert_match {ERR EXEC without MULTI} $e + assert_match {*calls=2,*,rejected_calls=0,failed_calls=1} [cmdstat exec] + assert_match {*count=2*} [errorstat ERR] + assert_equal [s total_error_replies] 2 + } + + test {errorstats: failed call within LUA} { + r config resetstat + assert_match {} [errorstat ERR] + assert_equal [s total_error_replies] 0 + catch {r eval {redis.pcall('XGROUP', 'CREATECONSUMER', 's1', 'mygroup', 'consumer') return } 0} e + assert_match {*count=1*} [errorstat ERR] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=1} [cmdstat xgroup] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=0} [cmdstat eval] + + # EVAL command errors should still be pinpointed to him + catch {r eval a} e + assert_match {ERR wrong*} $e + assert_match {*calls=1,*,rejected_calls=1,failed_calls=0} [cmdstat eval] + assert_match {*count=2*} [errorstat ERR] + assert_equal [s total_error_replies] 2 + } + + test {errorstats: failed call NOGROUP error} { + r config resetstat + assert_match {} [errorstat NOGROUP] + r del mystream + r XADD mystream * f v + catch {r XGROUP CREATECONSUMER mystream mygroup consumer} e + assert_match {NOGROUP*} $e + assert_match {*count=1*} [errorstat NOGROUP] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=1} [cmdstat xgroup] + r config resetstat + assert_match {} [errorstat NOGROUP] + } + + test {errorstats: rejected call unknown command} { + r config resetstat + assert_equal [s total_error_replies] 0 + assert_match {} [errorstat ERR] + catch {r asdf} e + assert_match {ERR unknown*} $e + assert_match {*count=1*} [errorstat ERR] + assert_equal [s total_error_replies] 1 + r config resetstat + assert_match {} [errorstat ERR] + } + + test {errorstats: rejected call within MULTI/EXEC} { + r config resetstat + assert_equal [s total_error_replies] 0 + assert_match {} [errorstat ERR] + r multi + catch {r set} e + assert_match {ERR wrong number of arguments*} $e + catch {r exec} e + assert_match {EXECABORT*} $e + assert_match {*count=1*} [errorstat ERR] + assert_equal [s total_error_replies] 1 + assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat set] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=0} [cmdstat multi] + assert_match {*calls=1,*,rejected_calls=0,failed_calls=0} [cmdstat exec] + assert_equal [s total_error_replies] 1 + r config resetstat + assert_match {} [errorstat ERR] + } + + test {errorstats: rejected call due to wrong arity} { + r config resetstat + assert_equal [s total_error_replies] 0 + assert_match {} [errorstat ERR] + catch {r set k} e + assert_match {ERR wrong number of arguments*} $e + assert_match {*count=1*} [errorstat ERR] + assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat set] + # ensure that after a rejected command, valid ones are counted properly + r set k1 v1 + r set k2 v2 + assert_match {calls=2,*,rejected_calls=1,failed_calls=0} [cmdstat set] + assert_equal [s total_error_replies] 1 + } + + test {errorstats: rejected call by OOM error} { + r config resetstat + assert_equal [s total_error_replies] 0 + assert_match {} [errorstat OOM] + r config set maxmemory 1 + catch {r set a b} e + assert_match {OOM*} $e + assert_match {*count=1*} [errorstat OOM] + assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat set] + assert_equal [s total_error_replies] 1 + r config resetstat + assert_match {} [errorstat OOM] + } + + test {errorstats: rejected call by authorization error} { + r config resetstat + assert_equal [s total_error_replies] 0 + assert_match {} [errorstat NOPERM] + r ACL SETUSER alice on >p1pp0 ~cached:* +get +info +config + r auth alice p1pp0 + catch {r set a b} e + assert_match {NOPERM*} $e + assert_match {*count=1*} [errorstat NOPERM] + assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat set] + assert_equal [s total_error_replies] 1 + r config resetstat + assert_match {} [errorstat NOPERM] + } + } +} -- cgit v1.2.1 From 152b5d46c4a76f2d16031ef794092bfc8d322f8a Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Fri, 1 Jan 2021 10:23:30 +0200 Subject: Crash log would crash half way on commands with no arguments (#8260) The crash log attempts to print the current client info, and when it does that it attempts to check if the first argument happens to be a key but it did so for commands with no arguments too, which caused the crash log to crash half way and not reach its end. --- src/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/debug.c b/src/debug.c index f95aaba0d..de6e8c13a 100644 --- a/src/debug.c +++ b/src/debug.c @@ -1569,7 +1569,7 @@ void logCurrentClient(void) { } /* Check if the first argument, usually a key, is found inside the * selected DB, and if so print info about the associated object. */ - if (cc->argc >= 1) { + if (cc->argc > 1) { robj *val, *key; dictEntry *de; -- cgit v1.2.1 From 71fbe6e800a10dabc81b1aec96bcca3b74af7bf4 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sat, 2 Jan 2021 08:37:19 +0200 Subject: Fix leak in new errorstats commit, and a flaky test (#8278) --- src/server.c | 2 +- tests/cluster/tests/18-info.tcl | 23 ++++++++++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/server.c b/src/server.c index dbcaa767d..49ce319c4 100644 --- a/src/server.c +++ b/src/server.c @@ -3301,7 +3301,7 @@ void resetCommandTableStats(void) { } void resetErrorTableStats(void) { - raxFree(server.errors); + raxFreeWithCallback(server.errors, zfree); server.errors = raxNew(); } diff --git a/tests/cluster/tests/18-info.tcl b/tests/cluster/tests/18-info.tcl index 83dbf833f..978d9d1da 100644 --- a/tests/cluster/tests/18-info.tcl +++ b/tests/cluster/tests/18-info.tcl @@ -26,11 +26,20 @@ test "errorstats: rejected call due to MOVED Redirection" { $primary2 config resetstat assert_match {} [errorstat $primary1 MOVED] assert_match {} [errorstat $primary2 MOVED] - # we know that the primary 2 will have a MOVED reply - catch {$primary1 set key{0x0000} b} replyP1 - catch {$primary2 set key{0x0000} b} replyP2 - assert_match {OK} $replyP1 - assert_match {} [errorstat $primary1 MOVED] - assert_match {*count=1*} [errorstat $primary2 MOVED] - assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat $primary2 set] + # we know that one will have a MOVED reply and one will succeed + catch {$primary1 set key b} replyP1 + catch {$primary2 set key b} replyP2 + # sort servers so we know which one failed + if {$replyP1 eq {OK}} { + assert_match {MOVED*} $replyP2 + set pok $primary1 + set perr $primary2 + } else { + assert_match {MOVED*} $replyP1 + set pok $primary2 + set perr $primary1 + } + assert_match {} [errorstat $pok MOVED] + assert_match {*count=1*} [errorstat $perr MOVED] + assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat $perr set] } -- cgit v1.2.1 From 41b2ed2bbc0671e43101feecc48cac26a5e312cb Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 3 Jan 2021 11:56:26 +0200 Subject: fix crash in redis-cli after making cluster backup (#8267) getRDB is "designed" to work in two modes: one for redis-cli --rdb and one for redis-cli --cluster backup. in the later case it uses the hiredis connection from the cluster nodes and it used to free it without nullifying the context, so a later attempt to free the context would crash. I suppose the reason it seems to want to free the hiredis context ASAP is that it wants to disconnect the replica link, so that replication buffers will not be accumulated. --- src/redis-cli.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/redis-cli.c b/src/redis-cli.c index ff29de748..31d2360c9 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -7132,7 +7132,9 @@ static void getRDB(clusterManagerNode *node) { } else { fprintf(stderr,"Transfer finished with success.\n"); } - redisFree(s); /* Close the file descriptor ASAP as fsync() may take time. */ + redisFree(s); /* Close the connection ASAP as fsync() may take time. */ + if (node) + node->context = NULL; fsync(fd); close(fd); fprintf(stderr,"Transfer finished with success.\n"); -- cgit v1.2.1 From 7896debe6c16ec212c2b8ce169227988ccd9a2d8 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 3 Jan 2021 16:09:29 +0200 Subject: Fix rare assertion as a result of: active defrag while loading (#8281) In #7726 (part of 6.2), we added a mechanism for whileBlockedCron, this mechanism has an assertion to make sure the timestamp in whileBlockedCron was always set correctly before the blocking operation starts. I now found (thanks to our CI) two bugs in that area: 1) CONFIG RESETSTAT (if it was allowed during loading) would have cleared this var 2) the call stopLoading (which calls whileBlockedCron) was made too early, while the rio is still in use, in which case the update_cksum (rdbLoadProgressCallback) may still be called and whileBlockedCron can assert. --- src/replication.c | 4 +++- src/server.c | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/replication.c b/src/replication.c index 520c43fa4..20eb83e72 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1690,7 +1690,6 @@ void readSyncBulkPayload(connection *conn) { * gets promoted. */ return; } - stopLoading(1); /* RDB loading succeeded if we reach this point. */ if (server.repl_diskless_load == REPL_DISKLESS_LOAD_SWAPDB) { @@ -1705,6 +1704,7 @@ void readSyncBulkPayload(connection *conn) { if (!rioRead(&rdb,buf,CONFIG_RUN_ID_SIZE) || memcmp(buf,eofmark,CONFIG_RUN_ID_SIZE) != 0) { + stopLoading(0); serverLog(LL_WARNING,"Replication stream EOF marker is broken"); cancelReplicationHandshake(1); rioFreeConn(&rdb, NULL); @@ -1712,6 +1712,8 @@ void readSyncBulkPayload(connection *conn) { } } + stopLoading(1); + /* Cleanup and restore the socket to the original state to continue * with the normal replication. */ rioFreeConn(&rdb, NULL); diff --git a/src/server.c b/src/server.c index 49ce319c4..7ecfa8d1b 100644 --- a/src/server.c +++ b/src/server.c @@ -2955,7 +2955,6 @@ void resetServerStats(void) { server.stat_total_error_replies = 0; server.stat_dump_payload_sanitizations = 0; server.aof_delayed_fsync = 0; - server.blocked_last_cron = 0; } /* Make the thread killable at any time, so that kill threads functions @@ -3004,6 +3003,7 @@ void initServer(void) { server.clients_paused = 0; server.events_processed_while_blocked = 0; server.system_memory_size = zmalloc_get_memory_size(); + server.blocked_last_cron = 0; if ((server.tls_port || server.tls_replication || server.tls_cluster) && tlsConfigure(&server.tls_ctx_config) == C_ERR) { -- cgit v1.2.1 From 91690a29202b8ff3fbc9305273463fd5a575e572 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 3 Jan 2021 16:14:52 +0200 Subject: Fix uninitialized variable in new errorstats commit (#8280) --- src/module.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/module.c b/src/module.c index 913b4de5d..abad38e78 100644 --- a/src/module.c +++ b/src/module.c @@ -851,6 +851,8 @@ int RM_CreateCommand(RedisModuleCtx *ctx, const char *name, RedisModuleCmdFunc c cp->rediscmd->keystep = keystep; cp->rediscmd->microseconds = 0; cp->rediscmd->calls = 0; + cp->rediscmd->rejected_calls = 0; + cp->rediscmd->failed_calls = 0; dictAdd(server.commands,sdsdup(cmdname),cp->rediscmd); dictAdd(server.orig_commands,sdsdup(cmdname),cp->rediscmd); cp->rediscmd->id = ACLGetCommandID(cmdname); /* ID used for ACL. */ -- cgit v1.2.1 From 33fb6170531435b891faf0ed7b21a95ac65dd0a1 Mon Sep 17 00:00:00 2001 From: kukey Date: Sun, 3 Jan 2021 23:13:37 +0800 Subject: GEOADD - add [CH] [NX|XX] options (#8227) New command flags similar to what SADD already has. Co-authored-by: huangwei03 Co-authored-by: Itamar Haber Co-authored-by: Oran Agra --- src/geo.c | 42 ++++++++++++++++++++++++++++-------------- tests/unit/geo.tcl | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/src/geo.c b/src/geo.c index 417bf897e..e9c8bf5cb 100644 --- a/src/geo.c +++ b/src/geo.c @@ -428,31 +428,45 @@ static int sort_gp_desc(const void *a, const void *b) { * Commands * ==================================================================== */ -/* GEOADD key long lat name [long2 lat2 name2 ... longN latN nameN] */ +/* GEOADD key [CH] [NX|XX] long lat name [long2 lat2 name2 ... longN latN nameN] */ void geoaddCommand(client *c) { - /* Check arguments number for sanity. */ - if ((c->argc - 2) % 3 != 0) { + int xx = 0, nx = 0, longidx = 2; + int i; + + /* Parse options. At the end 'longidx' is set to the argument position + * of the longitude of the first element. */ + while (longidx < c->argc) { + char *opt = c->argv[longidx]->ptr; + if (!strcasecmp(opt,"nx")) nx = 1; + else if (!strcasecmp(opt,"xx")) xx = 1; + else if (!strcasecmp(opt,"ch")) {} + else break; + longidx++; + } + + if ((c->argc - longidx) % 3 || (xx && nx)) { /* Need an odd number of arguments if we got this far... */ - addReplyError(c, "syntax error. Try GEOADD key [x1] [y1] [name1] " - "[x2] [y2] [name2] ... "); + addReplyErrorObject(c,shared.syntaxerr); return; } - int elements = (c->argc - 2) / 3; - int argc = 2+elements*2; /* ZADD key score ele ... */ + /* Set up the vector for calling ZADD. */ + int elements = (c->argc - longidx) / 3; + int argc = longidx+elements*2; /* ZADD key [CH] [NX|XX] score ele ... */ robj **argv = zcalloc(argc*sizeof(robj*)); argv[0] = createRawStringObject("zadd",4); - argv[1] = c->argv[1]; /* key */ - incrRefCount(argv[1]); + for (i = 1; i < longidx; i++) { + argv[i] = c->argv[i]; + incrRefCount(argv[i]); + } /* Create the argument vector to call ZADD in order to add all * the score,value pairs to the requested zset, where score is actually * an encoded version of lat,long. */ - int i; for (i = 0; i < elements; i++) { double xy[2]; - if (extractLongLatOrReply(c, (c->argv+2)+(i*3),xy) == C_ERR) { + if (extractLongLatOrReply(c, (c->argv+longidx)+(i*3),xy) == C_ERR) { for (i = 0; i < argc; i++) if (argv[i]) decrRefCount(argv[i]); zfree(argv); @@ -464,9 +478,9 @@ void geoaddCommand(client *c) { geohashEncodeWGS84(xy[0], xy[1], GEO_STEP_MAX, &hash); GeoHashFix52Bits bits = geohashAlign52Bits(hash); robj *score = createObject(OBJ_STRING, sdsfromlonglong(bits)); - robj *val = c->argv[2 + i * 3 + 2]; - argv[2+i*2] = score; - argv[3+i*2] = val; + robj *val = c->argv[longidx + i * 3 + 2]; + argv[longidx+i*2] = score; + argv[longidx+1+i*2] = val; incrRefCount(val); } diff --git a/tests/unit/geo.tcl b/tests/unit/geo.tcl index 119d2bd64..6c1f2ef11 100644 --- a/tests/unit/geo.tcl +++ b/tests/unit/geo.tcl @@ -74,6 +74,49 @@ start_server {tags {"geo"}} { r geoadd nyc -73.9454966 40.747533 "lic market" } {0} + test {GEOADD update with CH option} { + assert_equal 1 [r geoadd nyc CH 40.747533 -73.9454966 "lic market"] + lassign [lindex [r geopos nyc "lic market"] 0] x1 y1 + assert {abs($x1) - 40.747 < 0.001} + assert {abs($y1) - 73.945 < 0.001} + } {} + + test {GEOADD update with NX option} { + assert_equal 0 [r geoadd nyc NX -73.9454966 40.747533 "lic market"] + lassign [lindex [r geopos nyc "lic market"] 0] x1 y1 + assert {abs($x1) - 40.747 < 0.001} + assert {abs($y1) - 73.945 < 0.001} + } {} + + test {GEOADD update with XX option} { + assert_equal 0 [r geoadd nyc XX -83.9454966 40.747533 "lic market"] + lassign [lindex [r geopos nyc "lic market"] 0] x1 y1 + assert {abs($x1) - 83.945 < 0.001} + assert {abs($y1) - 40.747 < 0.001} + } {} + + test {GEOADD update with CH NX option} { + r geoadd nyc CH NX -73.9454966 40.747533 "lic market" + } {0} + + test {GEOADD update with CH XX option} { + r geoadd nyc CH XX -73.9454966 40.747533 "lic market" + } {1} + + test {GEOADD update with XX NX option will return syntax error} { + catch { + r geoadd nyc xx nx -73.9454966 40.747533 "lic market" + } err + set err + } {ERR*syntax*} + + test {GEOADD update with invalid option} { + catch { + r geoadd nyc ch xx foo -73.9454966 40.747533 "lic market" + } err + set err + } {ERR*syntax*} + test {GEOADD invalid coordinates} { catch { r geoadd nyc -73.9454966 40.747533 "lic market" \ -- cgit v1.2.1 From 08ad6abd04c5aafe5471fa754000e512ae6b0f05 Mon Sep 17 00:00:00 2001 From: huangzhw Date: Mon, 4 Jan 2021 16:28:47 +0800 Subject: sort Command lookupKeyRead and lookupKeyWrite are used on the opposite (#8283) This is a recent problem, introduced by 7471743 (redis 6.0) The implications are: The sole difference between LookupKeyRead and LookupKeyWrite is for command executed on a replica, which are not received from its master client. (for the master, and for the master client on the replica, these two functions behave the same)! Since SORT is a write command, this bug only implicates a writable-replica. And these are its implications: - SORT STORE will behave as it did before the above mentioned commit (like before redis 6.0). on a writable-replica an already logically expired the key would have appeared missing. (store dest key would be deleted, instead of being populated with the data from the already logically expired key) - SORT (the non store variant, which in theory could have been executed on read-only-replica if it weren't for the write flag), will (in redis 6.0) have a new bug and return the data from the already logically expired key instead of empty response. --- src/sort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sort.c b/src/sort.c index 44637720b..3b67cc639 100644 --- a/src/sort.c +++ b/src/sort.c @@ -270,7 +270,7 @@ void sortCommand(client *c) { } /* Lookup the key to sort. It must be of the right types */ - if (storekey) + if (!storekey) sortval = lookupKeyRead(c->db,c->argv[1]); else sortval = lookupKeyWrite(c->db,c->argv[1]); -- cgit v1.2.1 From ecd53518700f5b2be19c18e7a19f87c9f25a7ff5 Mon Sep 17 00:00:00 2001 From: "Meir Shpilraien (Spielrein)" Date: Mon, 4 Jan 2021 13:42:17 +0200 Subject: Fix assertion on loading AOF with timed out script. (#8284) If AOF file contains a long Lua script that timed out, then the `evalCommand` calls `blockingOperationEnds` which sets `server.blocked_last_cron` to 0. later on, the AOF `whileBlockedCron` function asserts that this value is not 0. The fix allows nesting call to `blockingOperationStarts` and `blockingOperationEnds`. The issue was first introduce in this commit: 9ef8d2f67 (Redis 6.2 RC1) --- src/server.c | 11 ++++++++--- src/server.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/server.c b/src/server.c index 7ecfa8d1b..1fee70789 100644 --- a/src/server.c +++ b/src/server.c @@ -2187,12 +2187,16 @@ int serverCron(struct aeEventLoop *eventLoop, long long id, void *clientData) { void blockingOperationStarts() { - updateCachedTime(0); - server.blocked_last_cron = server.mstime; + if(!server.blocking_op_nesting++){ + updateCachedTime(0); + server.blocked_last_cron = server.mstime; + } } void blockingOperationEnds() { - server.blocked_last_cron = 0; + if(!(--server.blocking_op_nesting)){ + server.blocked_last_cron = 0; + } } /* This function fill in the role of serverCron during RDB or AOF loading, and @@ -3004,6 +3008,7 @@ void initServer(void) { server.events_processed_while_blocked = 0; server.system_memory_size = zmalloc_get_memory_size(); server.blocked_last_cron = 0; + server.blocking_op_nesting = 0; if ((server.tls_port || server.tls_replication || server.tls_cluster) && tlsConfigure(&server.tls_ctx_config) == C_ERR) { diff --git a/src/server.h b/src/server.h index 5f6b21ec4..aaee5a35d 100644 --- a/src/server.h +++ b/src/server.h @@ -1468,6 +1468,7 @@ struct redisServer { int daylight_active; /* Currently in daylight saving time. */ mstime_t mstime; /* 'unixtime' in milliseconds. */ ustime_t ustime; /* 'unixtime' in microseconds. */ + size_t blocking_op_nesting; /* Nesting level of blocking operation, used to reset blocked_last_cron. */ long long blocked_last_cron; /* Indicate the mstime of the last time we did cron jobs from a blocking operation */ /* Pubsub */ dict *pubsub_channels; /* Map channels to list of subscribed clients */ -- cgit v1.2.1 From 10f94b0ab12f9315939dcccf39d64b9388c0c7fa Mon Sep 17 00:00:00 2001 From: Yang Bodong Date: Mon, 4 Jan 2021 20:48:28 +0800 Subject: Swapdb should make transaction fail if there is any client watching keys (#8239) This PR not only fixes the problem that swapdb does not make the transaction fail, but also optimizes the FLUSHALL and FLUSHDB command to set the CLIENT_DIRTY_CAS flag to avoid unnecessary traversal of clients. FLUSHDB was changed to first iterate on all watched keys, and then on the clients watching each key. Instead of iterating though all clients, and for each iterate on watched keys. Co-authored-by: Oran Agra --- src/db.c | 20 ++++++++++++++++++-- src/multi.c | 47 ++++++++++++++++++++++++++--------------------- src/server.h | 2 +- tests/unit/multi.tcl | 23 +++++++++++++++++++++++ 4 files changed, 68 insertions(+), 24 deletions(-) diff --git a/src/db.c b/src/db.c index 495300613..62ffda101 100644 --- a/src/db.c +++ b/src/db.c @@ -573,7 +573,18 @@ void signalModifiedKey(client *c, redisDb *db, robj *key) { } void signalFlushedDb(int dbid, int async) { - touchWatchedKeysOnFlush(dbid); + int startdb, enddb; + if (dbid == -1) { + startdb = 0; + enddb = server.dbnum-1; + } else { + startdb = enddb = dbid; + } + + for (int j = startdb; j <= enddb; j++) { + touchAllWatchedKeysInDb(&server.db[j], NULL); + } + trackingInvalidateKeysOnFlush(async); } @@ -1331,9 +1342,14 @@ int dbSwapDatabases(long id1, long id2) { * However normally we only do this check for efficiency reasons * in dbAdd() when a list is created. So here we need to rescan * the list of clients blocked on lists and signal lists as ready - * if needed. */ + * if needed. + * + * Also the swapdb should make transaction fail if there is any + * client watching keys */ scanDatabaseForReadyLists(db1); + touchAllWatchedKeysInDb(db1, db2); scanDatabaseForReadyLists(db2); + touchAllWatchedKeysInDb(db2, db1); return C_OK; } diff --git a/src/multi.c b/src/multi.c index af0b0c612..d88c5f1b8 100644 --- a/src/multi.c +++ b/src/multi.c @@ -374,31 +374,36 @@ void touchWatchedKey(redisDb *db, robj *key) { } } -/* On FLUSHDB or FLUSHALL all the watched keys that are present before the - * flush but will be deleted as effect of the flushing operation should - * be touched. "dbid" is the DB that's getting the flush. -1 if it is - * a FLUSHALL operation (all the DBs flushed). */ -void touchWatchedKeysOnFlush(int dbid) { - listIter li1, li2; +/* Set CLIENT_DIRTY_CAS to all clients of DB when DB is dirty. + * It may happen in the following situations: + * FLUSHDB, FLUSHALL, SWAPDB + * + * replaced_with: for SWAPDB, the WATCH should be invalidated if + * the key exists in either of them, and skipped only if it + * doesn't exist in both. */ +void touchAllWatchedKeysInDb(redisDb *emptied, redisDb *replaced_with) { + listIter li; listNode *ln; - - /* For every client, check all the waited keys */ - listRewind(server.clients,&li1); - while((ln = listNext(&li1))) { - client *c = listNodeValue(ln); - listRewind(c->watched_keys,&li2); - while((ln = listNext(&li2))) { - watchedKey *wk = listNodeValue(ln); - - /* For every watched key matching the specified DB, if the - * key exists, mark the client as dirty, as the key will be - * removed. */ - if (dbid == -1 || wk->db->id == dbid) { - if (dictFind(wk->db->dict, wk->key->ptr) != NULL) - c->flags |= CLIENT_DIRTY_CAS; + dictEntry *de; + + if (dictSize(emptied->watched_keys) == 0) return; + + dictIterator *di = dictGetSafeIterator(emptied->watched_keys); + while((de = dictNext(di)) != NULL) { + robj *key = dictGetKey(de); + list *clients = dictGetVal(de); + if (!clients) continue; + listRewind(clients,&li); + while((ln = listNext(&li))) { + client *c = listNodeValue(ln); + if (dictFind(emptied->dict, key->ptr)) { + c->flags |= CLIENT_DIRTY_CAS; + } else if (replaced_with && dictFind(replaced_with->dict, key->ptr)) { + c->flags |= CLIENT_DIRTY_CAS; } } } + dictReleaseIterator(di); } void watchCommand(client *c) { diff --git a/src/server.h b/src/server.h index aaee5a35d..d79bf5b23 100644 --- a/src/server.h +++ b/src/server.h @@ -1864,7 +1864,7 @@ void initClientMultiState(client *c); void freeClientMultiState(client *c); void queueMultiCommand(client *c); void touchWatchedKey(redisDb *db, robj *key); -void touchWatchedKeysOnFlush(int dbid); +void touchAllWatchedKeysInDb(redisDb *emptied, redisDb *replaced_with); void discardTransaction(client *c); void flagTransaction(client *c); void execCommandAbort(client *c, sds error); diff --git a/tests/unit/multi.tcl b/tests/unit/multi.tcl index 25e417055..e22b6d43d 100644 --- a/tests/unit/multi.tcl +++ b/tests/unit/multi.tcl @@ -196,6 +196,29 @@ start_server {tags {"multi"}} { r exec } {PONG} + test {SWAPDB is able to touch the watched keys that exist} { + r flushall + r select 0 + r set x 30 + r watch x ;# make sure x (set to 30) doesn't change (SWAPDB will "delete" it) + r swapdb 0 1 + r multi + r ping + r exec + } {} + + test {SWAPDB is able to touch the watched keys that do not exist} { + r flushall + r select 1 + r set x 30 + r select 0 + r watch x ;# make sure the key x (currently missing) doesn't change (SWAPDB will create it) + r swapdb 0 1 + r multi + r ping + r exec + } {} + test {WATCH is able to remember the DB a key belongs to} { r select 5 r set x 30 -- cgit v1.2.1 From 9dcdc7e79a25968fcdfde09c7ca72a2012a1febf Mon Sep 17 00:00:00 2001 From: Itamar Haber Date: Mon, 4 Jan 2021 17:02:57 +0200 Subject: HELP subcommand, continued (#5531) * man-like consistent long formatting * Uppercases commands, subcommands and options * Adds 'HELP' to HELP for all * Lexicographical order * Uses value notation and other .md likeness * Moves const char *help to top * Keeps it under 80 chars * Misc help typos, consistent conjuctioning (i.e return and not returns) * Uses addReplySubcommandSyntaxError(c) all over Signed-off-by: Itamar Haber --- src/acl.c | 44 ++++++++++++++-------- src/cluster.c | 65 +++++++++++++++++++++----------- src/config.c | 13 +++++-- src/debug.c | 111 +++++++++++++++++++++++++++++++++++++++---------------- src/latency.c | 24 +++++++----- src/module.c | 15 ++++++-- src/networking.c | 70 +++++++++++++++++++++++------------ src/object.c | 36 ++++++++++++------ src/pubsub.c | 10 +++-- src/scripting.c | 21 +++++++---- src/sentinel.c | 58 +++++++++++++++++++---------- src/server.c | 15 +++++--- src/slowlog.c | 14 ++++--- src/t_stream.c | 55 ++++++++++++++------------- 14 files changed, 363 insertions(+), 188 deletions(-) diff --git a/src/acl.c b/src/acl.c index 606b61cb7..14d023cc3 100644 --- a/src/acl.c +++ b/src/acl.c @@ -174,15 +174,15 @@ sds ACLHashPassword(unsigned char *cleartext, size_t len) { return sdsnewlen(hex,HASH_PASSWORD_LEN); } -/* Given a hash and the hash length, returns C_OK if it is a valid password +/* Given a hash and the hash length, returns C_OK if it is a valid password * hash, or C_ERR otherwise. */ int ACLCheckPasswordHash(unsigned char *hash, int hashlen) { if (hashlen != HASH_PASSWORD_LEN) { - return C_ERR; + return C_ERR; } - + /* Password hashes can only be characters that represent - * hexadecimal values, which are numbers and lowercase + * hexadecimal values, which are numbers and lowercase * characters 'a' through 'f'. */ for(int i = 0; i < HASH_PASSWORD_LEN; i++) { char c = hash[i]; @@ -2184,18 +2184,30 @@ void aclCommand(client *c) { } } else if (c->argc == 2 && !strcasecmp(sub,"help")) { const char *help[] = { -"LOAD -- Reload users from the ACL file.", -"SAVE -- Save the current config to the ACL file.", -"LIST -- Show user details in config file format.", -"USERS -- List all the registered usernames.", -"SETUSER [attribs ...] -- Create or modify a user.", -"GETUSER -- Get the user details.", -"DELUSER [...] -- Delete a list of users.", -"CAT -- List available categories.", -"CAT -- List commands inside category.", -"GENPASS [] -- Generate a secure user password.", -"WHOAMI -- Return the current connection username.", -"LOG [ | RESET] -- Show the ACL log entries.", +"CAT []", +" List all commands that belong to , or all command categories", +" when no category is specified.", +"DELUSER [ ...]", +" Delete a list of users.", +"GETUSER ", +" Get the user's details.", +"GENPASS []", +" Generate a secure 256-bit user password. The optional `bits` argument can", +" be used to specify a different size.", +"LIST", +" Show users details in config file format.", +"LOAD", +" Reload users from the ACL file.", +"LOG [ | RESET]", +" Show the ACL log entries.", +"SAVE", +" Save the current config to the ACL file.", +"SETUSER [ ...]", +" Create or modify a user with the specified attributes.", +"USERS", +" List all the registered usernames.", +"WHOAMI", +" Return the current connection username.", NULL }; addReplyHelp(c,help); diff --git a/src/cluster.c b/src/cluster.c index 29bdb2109..d9f1a66d7 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -4357,28 +4357,49 @@ void clusterCommand(client *c) { if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) { const char *help[] = { -"ADDSLOTS [slot ...] -- Assign slots to current node.", -"BUMPEPOCH -- Advance the cluster config epoch.", -"COUNT-failure-reports -- Return number of failure reports for .", -"COUNTKEYSINSLOT - Return the number of keys in .", -"DELSLOTS [slot ...] -- Delete slots information from current node.", -"FAILOVER [force|takeover] -- Promote current replica node to being a master.", -"FORGET -- Remove a node from the cluster.", -"GETKEYSINSLOT -- Return key names stored by current node in a slot.", -"FLUSHSLOTS -- Delete current node own slots information.", -"INFO - Return information about the cluster.", -"KEYSLOT -- Return the hash slot for .", -"MEET [bus-port] -- Connect nodes into a working cluster.", -"MYID -- Return the node id.", -"NODES -- Return cluster configuration seen by node. Output format:", -" ... ", -"REPLICATE -- Configure current node as replica to .", -"RESET [hard|soft] -- Reset current node (default: soft).", -"SET-config-epoch - Set config epoch of current node.", -"SETSLOT (importing|migrating|stable|node ) -- Set slot state.", -"REPLICAS -- Return replicas.", -"SAVECONFIG - Force saving cluster configuration on disk.", -"SLOTS -- Return information about slots range mappings. Each range is made of:", +"ADDSLOTS [ ...]", +" Assign slots to current node.", +"BUMPEPOCH", +" Advance the cluster config epoch.", +"COUNT-FAILURE-REPORTS ", +" Return number of failure reports for .", +"COUNTKEYSINSLOT ", +" Return the number of keys in .", +"DELSLOTS [ ...]", +" Delete slots information from current node.", +"FAILOVER [FORCE|TAKEOVER]", +" Promote current replica node to being a master.", +"FORGET ", +" Remove a node from the cluster.", +"GETKEYSINSLOT ", +" Return key names stored by current node in a slot.", +"FLUSHSLOTS", +" Delete current node own slots information.", +"INFO", +" Return information about the cluster.", +"KEYSLOT ", +" Return the hash slot for .", +"MEET []", +" Connect nodes into a working cluster.", +"MYID", +" Return the node id.", +"NODES", +" Return cluster configuration seen by node. Output format:", +" ...", +"REPLICATE ", +" Configure current node as replica to .", +"RESET [HARD|SOFT]", +" Reset current node (default: soft).", +"SET-CONFIG-EPOCH ", +" Set config epoch of current node.", +"SETSLOT (IMPORTING|MIGRATING|STABLE|NODE )", +" Set slot state.", +"REPLICAS ", +" Return replicas.", +"SAVECONFIG", +" Force saving cluster configuration on disk.", +"SLOTS", +" Return information about slots range mappings. Each range is made of:", " start, end, master and replicas IP addresses, ports and ids", NULL }; diff --git a/src/config.c b/src/config.c index 9d078bfe3..d804ff0ef 100644 --- a/src/config.c +++ b/src/config.c @@ -2546,12 +2546,17 @@ void configCommand(client *c) { if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) { const char *help[] = { -"GET -- Return parameters matching the glob-like and their values.", -"SET -- Set parameter to value.", -"RESETSTAT -- Reset statistics reported by INFO.", -"REWRITE -- Rewrite the configuration file.", +"GET ", +" Return parameters matching the glob-like and their values.", +"SET ", +" Set the configuration to .", +"RESETSTAT", +" Reset statistics reported by the INFO command.", +"REWRITE", +" Rewrite the configuration file.", NULL }; + addReplyHelp(c, help); } else if (!strcasecmp(c->argv[1]->ptr,"set") && c->argc == 4) { configSetCommand(c); diff --git a/src/debug.c b/src/debug.c index de6e8c13a..a725e5a30 100644 --- a/src/debug.c +++ b/src/debug.c @@ -381,39 +381,88 @@ void mallctl_string(client *c, robj **argv, int argc) { void debugCommand(client *c) { if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) { const char *help[] = { -"ASSERT -- Crash by assertion failed.", -"CHANGE-REPL-ID -- Change the replication IDs of the instance. Dangerous, should be used only for testing the replication subsystem.", -"CRASH-AND-RECOVER -- Hard crash and restart after delay.", -"DIGEST -- Output a hex signature representing the current DB content.", -"DIGEST-VALUE ... -- Output a hex signature of the values of all the specified keys.", -"DEBUG PROTOCOL [string|integer|double|bignum|null|array|set|map|attrib|push|verbatim|true|false]", -"ERROR -- Return a Redis protocol error with as message. Useful for clients unit tests to simulate Redis errors.", -"LOG -- write message to the server log.", -"LEAK -- Create a memory leak of the input string.", -"HTSTATS -- Return hash table statistics of the specified Redis database.", -"HTSTATS-KEY -- Like htstats but for the hash table stored as key's value.", -"LOADAOF -- Flush the AOF buffers on disk and reload the AOF in memory.", -"LUA-ALWAYS-REPLICATE-COMMANDS <0|1> -- Setting it to 1 makes Lua replication defaulting to replicating single commands, without the script having to enable effects replication.", -"OBJECT -- Show low level info about key and associated value.", -"OOM -- Crash the server simulating an out-of-memory error.", -"PANIC -- Crash the server simulating a panic.", -"POPULATE [prefix] [size] -- Create string keys named key:. If a prefix is specified is used instead of the 'key' prefix.", -"RELOAD [MERGE] [NOFLUSH] [NOSAVE] -- Save the RDB on disk and reload it back in memory. By default it will save the RDB file and load it back. With the NOFLUSH option the current database is not removed before loading the new one, but conflicts in keys will kill the server with an exception. When MERGE is used, conflicting keys will be loaded (the key in the loaded RDB file will win). When NOSAVE is used, the server will not save the current dataset in the RDB file before loading. Use DEBUG RELOAD NOSAVE when you want just to load the RDB file you placed in the Redis working directory in order to replace the current dataset in memory. Use DEBUG RELOAD NOSAVE NOFLUSH MERGE when you want to add what is in the current RDB file placed in the Redis current directory, with the current memory content. Use DEBUG RELOAD when you want to verify Redis is able to persist the current dataset in the RDB file, flush the memory content, and load it back.", -"RESTART -- Graceful restart: save config, db, restart.", -"SDSLEN -- Show low level SDS string info representing key and value.", -"SEGFAULT -- Crash the server with sigsegv.", -"SET-ACTIVE-EXPIRE <0|1> -- Setting it to 0 disables expiring keys in background when they are not accessed (otherwise the Redis behavior). Setting it to 1 reenables back the default.", -"SET-SKIP-CHECKSUM-VALIDATION <0|1> -- Enables or disables checksum checks for rdb or RESTORE payload.", -"AOF-FLUSH-SLEEP -- Server will sleep before flushing the AOF, this is used for testing", -"SLEEP -- Stop the server for . Decimals allowed.", -"STRUCTSIZE -- Return the size of different Redis core C structures.", -"ZIPLIST -- Show low level info about the ziplist encoding.", -"STRINGMATCH-TEST -- Run a fuzz tester against the stringmatchlen() function.", -"CONFIG-REWRITE-FORCE-ALL -- Like CONFIG REWRITE but writes all configuration options, including keywords not listed in original configuration file or default values.", +"AOF-FLUSH-SLEEP ", +" Server will sleep before flushing the AOF, this is used for testing.", +"ASSERT", +" Crash by assertion failed.", +"CHANGE-REPL-ID" +" Change the replication IDs of the instance.", +" Dangerous: should be used only for testing the replication subsystem.", +"CONFIG-REWRITE-FORCE-ALL", +" Like CONFIG REWRITE but writes all configuration options, including", +" keywords not listed in original configuration file or default values.", +"CRASH-AND-RECOVER ", +" Hard crash and restart after a delay.", +"DIGEST", +" Output a hex signature representing the current DB content.", +"DIGEST-VALUE [ ...]", +" Output a hex signature of the values of all the specified keys.", +"ERROR ", +" Return a Redis protocol error with as message. Useful for clients", +" unit tests to simulate Redis errors.", +"LOG ", +" Write to the server log.", +"HTSTATS ", +" Return hash table statistics of the specified Redis database.", +"HTSTATS-KEY ", +" Like HTSTATS but for the hash table stored at 's value.", +"LOADAOF", +" Flush the AOF buffers on disk and reload the AOF in memory.", +"LUA-ALWAYS-REPLICATE-COMMANDS <0|1>", +" Setting it to 1 makes Lua replication defaulting to replicating single", +" commands, without the script having to enable effects replication.", #ifdef USE_JEMALLOC -"MALLCTL [] -- Get or set a malloc tunning integer.", -"MALLCTL-STR [] -- Get or set a malloc tunning string.", +"MALLCTL []", +" Get or set a malloc tuning integer.", +"MALLCTL-STR []", +" Get or set a malloc tuning string.", #endif +"OBJECT ", +" Show low level info about `key` and associated value.", +"OOM", +" Crash the server simulating an out-of-memory error.", +"PANIC", +" Crash the server simulating a panic.", +"POPULATE [] []", +" Create string keys named key:. If is specified then", +" it is used instead of the 'key' prefix.", +"DEBUG PROTOCOL ", +" Reply with a test value of the specified type. can be: string,", +" integer, double, bignum, null, array, set, map, attrib, push, verbatim,", +" true, false.", +"RELOAD [option ...]", +" Save the RDB on disk and reload it back to memory. Valid