Redis 2.2.8

This reverts commit 1b905277bb.
Moving all the new stuff in 2.4 branch.

00-RELEASENOTES

@@ -12,6 +12,40 @@ for 2.0.
 CHANGELOG
 ---------
 
+What's new in Redis 2.2.8
+=========================
+
+* A new form of dict.c (hash table implementation) iterator that performs less
+  copy-on-write of pages, introduced in Redis 2.2.7, caused ZINTERSTORE,
+  ZUNIONSTORE, SINTER, SINTERSTORE commands to behave in the wrong way.
+  This bug is now fixed.
+* Print version info before running the test with 'make test'. This is mainly
+  useful for the Continuous Integration system we run.
+* Fix for DEBUG DIGEST, key may expire on lookup, producing the wrong result.
+* Replication with expire test modified to produce no or less false failures.
+* Fixed Z*STORE when dealing with intsets, regression test added.
+
+What's new in Redis 2.2.7
+=========================
+
+* Fixed bug #543-2 (the issue was reopened with a completely different report)
+  that caused Redis to randomly crash on list push performed against lists
+  with other clients blocked with BLPOP (or variants).
+
+What's new in Redis 2.2.6
+=========================
+
+* Fixed bug #543. If you saw Redis instances crashing on List operations
+  (only happening with a non-default max entry size ziplist setting in
+  redis.conf) it was almost certainly this problem.
+* Fixed a bug with replication where SLAVEOF NO ONE caused a slave to close the
+  connection with all its slaves.
+
+What's new in Redis 2.2.5
+=========================
+
+* Fixed a crash occurring when loading an AOF containing the SPOP command.
+
 What's new in Redis 2.2.4
 =========================
 

src/aof.c

@@ -285,9 +285,11 @@ int loadAppendOnlyFile(char *filename) {
         /* The fake client should not have a reply */
         redisAssert(fakeClient->bufpos == 0 && listLength(fakeClient->reply) == 0);
 
-        /* Clean up, ready for the next command */
-        for (j = 0; j < argc; j++) decrRefCount(argv[j]);
-        zfree(argv);
+        /* Clean up. Command code may have changed argv/argc so we use the
+         * argv/argc of the client instead of the local variables. */
+        for (j = 0; j < fakeClient->argc; j++)
+            decrRefCount(fakeClient->argv[j]);
+        zfree(fakeClient->argv);
 
         /* Handle swapping while loading big datasets when VM is on */
         force_swapout = 0;

src/debug.c

@@ -80,7 +80,7 @@ void computeDatasetDigest(unsigned char *final) {
         redisDb *db = server.db+j;
 
         if (dictSize(db->dict) == 0) continue;
-        di = dictGetIterator(db->dict);
+        di = dictGetSafeIterator(db->dict);
 
         /* hash the DB id, so the same dataset moved in a different
          * DB will lead to a different digest */
@@ -101,6 +101,11 @@ void computeDatasetDigest(unsigned char *final) {
 
             /* Make sure the key is loaded if VM is active */
             o = lookupKeyRead(db,keyobj);
+            if (o == NULL) {
+                /* Key expired on lookup? Try the next one. */
+                decrRefCount(keyobj);
+                continue;
+            }
 
             aux = htonl(o->type);
             mixDigest(digest,&aux,sizeof(aux));

src/dict.c

@@ -244,9 +244,9 @@ int dictRehashMilliseconds(dict *d, int ms) {
 }
 
 /* This function performs just a step of rehashing, and only if there are
- * not iterators bound to our hash table. When we have iterators in the middle
- * of a rehashing we can't mess with the two hash tables otherwise some element
- * can be missed or duplicated.
+ * no safe iterators bound to our hash table. When we have iterators in the
+ * middle of a rehashing we can't mess with the two hash tables otherwise
+ * some element can be missed or duplicated.
  *
  * This function is called by common lookup or update operations in the
  * dictionary so that the hash table automatically migrates from H1 to H2
@@ -423,17 +423,26 @@ dictIterator *dictGetIterator(dict *d)
     iter->d = d;
     iter->table = 0;
     iter->index = -1;
+    iter->safe = 0;
     iter->entry = NULL;
     iter->nextEntry = NULL;
     return iter;
 }
 
+dictIterator *dictGetSafeIterator(dict *d) {
+    dictIterator *i = dictGetIterator(d);
+
+    i->safe = 1;
+    return i;
+}
+
 dictEntry *dictNext(dictIterator *iter)
 {
     while (1) {
         if (iter->entry == NULL) {
             dictht *ht = &iter->d->ht[iter->table];
-            if (iter->index == -1 && iter->table == 0) iter->d->iterators++;
+            if (iter->safe && iter->index == -1 && iter->table == 0)
+                iter->d->iterators++;
             iter->index++;
             if (iter->index >= (signed) ht->size) {
                 if (dictIsRehashing(iter->d) && iter->table == 0) {
@@ -460,7 +469,8 @@ dictEntry *dictNext(dictIterator *iter)
 
 void dictReleaseIterator(dictIterator *iter)
 {
-    if (!(iter->index == -1 && iter->table == 0)) iter->d->iterators--;
+    if (iter->safe && !(iter->index == -1 && iter->table == 0))
+        iter->d->iterators--;
     zfree(iter);
 }
 

src/dict.h

@@ -74,10 +74,13 @@ typedef struct dict {
     int iterators; /* number of iterators currently running */
 } dict;
 
+/* If safe is set to 1 this is a safe iteartor, that means, you can call
+ * dictAdd, dictFind, and other functions against the dictionary even while
+ * iterating. Otherwise it is a non safe iterator, and only dictNext()
+ * should be called while iterating. */
 typedef struct dictIterator {
     dict *d;
-    int table;
-    int index;
+    int table, index, safe;
     dictEntry *entry, *nextEntry;
 } dictIterator;
 
@@ -132,6 +135,7 @@ dictEntry * dictFind(dict *d, const void *key);
 void *dictFetchValue(dict *d, const void *key);
 int dictResize(dict *d);
 dictIterator *dictGetIterator(dict *d);
+dictIterator *dictGetSafeIterator(dict *d);
 dictEntry *dictNext(dictIterator *iter);
 void dictReleaseIterator(dictIterator *iter);
 dictEntry *dictGetRandomKey(dict *d);

src/networking.c

@@ -416,7 +416,7 @@ void acceptTcpHandler(aeEventLoop *el, int fd, void *privdata, int mask) {
 
     cfd = anetTcpAccept(server.neterr, fd, cip, &cport);
     if (cfd == AE_ERR) {
-        redisLog(REDIS_VERBOSE,"Accepting client connection: %s", server.neterr);
+        redisLog(REDIS_WARNING,"Accepting client connection: %s", server.neterr);
         return;
     }
     redisLog(REDIS_VERBOSE,"Accepted %s:%d", cip, cport);
@@ -431,7 +431,7 @@ void acceptUnixHandler(aeEventLoop *el, int fd, void *privdata, int mask) {
 
     cfd = anetUnixAccept(server.neterr, fd);
     if (cfd == AE_ERR) {
-        redisLog(REDIS_VERBOSE,"Accepting client connection: %s", server.neterr);
+        redisLog(REDIS_WARNING,"Accepting client connection: %s", server.neterr);
         return;
     }
     redisLog(REDIS_VERBOSE,"Accepted connection to %s", server.unixsocket);
@@ -523,10 +523,16 @@ void freeClient(redisClient *c) {
          * close the connection with all our slaves if we have any, so
          * when we'll resync with the master the other slaves will sync again
          * with us as well. Note that also when the slave is not connected
-         * to the master it will keep refusing connections by other slaves. */
-        while (listLength(server.slaves)) {
-            ln = listFirst(server.slaves);
-            freeClient((redisClient*)ln->value);
+         * to the master it will keep refusing connections by other slaves.
+         *
+         * We do this only if server.masterhost != NULL. If it is NULL this
+         * means the user called SLAVEOF NO ONE and we are freeing our
+         * link with the master, so no need to close link with slaves. */
+        if (server.masterhost != NULL) {
+            while (listLength(server.slaves)) {
+                ln = listFirst(server.slaves);
+                freeClient((redisClient*)ln->value);
+            }
         }
     }
     /* Release memory */

src/t_list.c

@@ -817,7 +817,6 @@ int handleClientsWaitingListPush(redisClient *c, robj *key, robj *ele) {
 
         /* This should remove the first element of the "clients" list. */
         unblockClientWaitingData(receiver);
-        redisAssert(ln != listFirst(clients));
 
         if (dstkey == NULL) {
             /* BRPOP/BLPOP */

src/t_set.c

@@ -433,6 +433,7 @@ void sinterGenericCommand(redisClient *c, robj **setkeys, unsigned long setnum,
     si = setTypeInitIterator(sets[0]);
     while((encoding = setTypeNext(si,&eleobj,&intobj)) != -1) {
         for (j = 1; j < setnum; j++) {
+            if (sets[j] == sets[0]) continue;
             if (encoding == REDIS_ENCODING_INTSET) {
                 /* intset with intset is simple... and fast */
                 if (sets[j]->encoding == REDIS_ENCODING_INTSET &&

src/t_zset.c

@@ -615,6 +615,10 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) {
             if (obj->type == REDIS_ZSET) {
                 src[i].dict = ((zset*)obj->ptr)->dict;
             } else if (obj->type == REDIS_SET) {
+                if (obj->encoding == REDIS_ENCODING_INTSET)
+                    setTypeConvert(obj, REDIS_ENCODING_HT);
+
+                redisAssert(obj->encoding == REDIS_ENCODING_HT);
                 src[i].dict = (obj->ptr);
             } else {
                 zfree(src);
@@ -682,7 +686,18 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) {
 
                 score = src[0].weight * zunionInterDictValue(de);
                 for (j = 1; j < setnum; j++) {
-                    dictEntry *other = dictFind(src[j].dict,dictGetEntryKey(de));
+                    dictEntry *other;
+
+                    /* If it's the same dictionary don't lookup as we are not
+                     * in the context of a safe iterator. It's the same
+                     * dictionary so we are sure the element is inside.
+                     * This happens on SINTERSTORE dest 2 mykey mykey. */
+                    if (src[j].dict == src[0].dict) {
+                        other = de;
+                    } else {
+                        other = dictFind(src[j].dict,dictGetEntryKey(de));
+                    }
+
                     if (other) {
                         value = src[j].weight * zunionInterDictValue(other);
                         zunionInterAggregate(&score,value,aggregate);
@@ -720,10 +735,19 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) {
                 /* because the zsets are sorted by size, its only possible
                  * for sets at larger indices to hold this entry */
                 for (j = (i+1); j < setnum; j++) {
-                    dictEntry *other = dictFind(src[j].dict,dictGetEntryKey(de));
-                    if (other) {
-                        value = src[j].weight * zunionInterDictValue(other);
+                    /* It is not safe to access the zset we are
+                     * iterating, so explicitly check for equal object. */
+                    if (src[j].dict == src[i].dict) {
+                        value = src[i].weight * zunionInterDictValue(de);
                         zunionInterAggregate(&score,value,aggregate);
+                    } else {
+                        dictEntry *other;
+
+                        other = dictFind(src[j].dict,dictGetEntryKey(de));
+                        if (other) {
+                            value = src[j].weight * zunionInterDictValue(other);
+                            zunionInterAggregate(&score,value,aggregate);
+                        }
                     }
                 }
 

src/version.h

@@ -1 +1 @@
-#define REDIS_VERSION "2.2.4"
+#define REDIS_VERSION "2.2.8"

src/ziplist.c

@@ -398,12 +398,17 @@ static unsigned char *__ziplistCascadeUpdate(unsigned char *zl, unsigned char *p
             offset = p-zl;
             extra = rawlensize-next.prevrawlensize;
             zl = ziplistResize(zl,curlen+extra);
-            ZIPLIST_TAIL_OFFSET(zl) += extra;
             p = zl+offset;
 
-            /* Move the tail to the back. */
+            /* Current pointer and offset for next element. */
             np = p+rawlen;
             noffset = np-zl;
+
+            /* Update tail offset when next element is not the tail element. */
+            if ((zl+ZIPLIST_TAIL_OFFSET(zl)) != np)
+                ZIPLIST_TAIL_OFFSET(zl) += extra;
+
+            /* Move the tail to the back. */
             memmove(np+rawlensize,
                 np+next.prevrawlensize,
                 curlen-noffset-next.prevrawlensize-1);
@@ -877,7 +882,7 @@ void pop(unsigned char *zl, int where) {
     }
 }
 
-void randstring(char *target, unsigned int min, unsigned int max) {
+int randstring(char *target, unsigned int min, unsigned int max) {
     int p, len = min+rand()%(max-min+1);
     int minval, maxval;
     switch(rand() % 3) {
@@ -899,10 +904,9 @@ void randstring(char *target, unsigned int min, unsigned int max) {
 
     while(p < len)
         target[p++] = minval+rand()%(maxval-minval+1);
-    return;
+    return len;
 }
 
-
 int main(int argc, char **argv) {
     unsigned char *zl, *p;
     unsigned char *entry;
@@ -1235,6 +1239,7 @@ int main(int argc, char **argv) {
         int i,j,len,where;
         unsigned char *p;
         char buf[1024];
+        int buflen;
         list *ref;
         listNode *refnode;
 
@@ -1243,10 +1248,6 @@ int main(int argc, char **argv) {
         unsigned int slen;
         long long sval;
 
-        /* In the regression for the cascade bug, it was triggered
-         * with a random seed of 2. */
-        srand(2);
-
         for (i = 0; i < 20000; i++) {
             zl = ziplistNew();
             ref = listCreate();
@@ -1256,31 +1257,32 @@ int main(int argc, char **argv) {
             /* Create lists */
             for (j = 0; j < len; j++) {
                 where = (rand() & 1) ? ZIPLIST_HEAD : ZIPLIST_TAIL;
-                switch(rand() % 4) {
-                case 0:
-                    sprintf(buf,"%lld",(0LL + rand()) >> 20);
-                    break;
-                case 1:
-                    sprintf(buf,"%lld",(0LL + rand()));
-                    break;
-                case 2:
-                    sprintf(buf,"%lld",(0LL + rand()) << 20);
-                    break;
-                case 3:
-                    randstring(buf,0,256);
-                break;
-                default:
-                    assert(NULL);
+                if (rand() % 2) {
+                    buflen = randstring(buf,1,sizeof(buf)-1);
+                } else {
+                    switch(rand() % 3) {
+                    case 0:
+                        buflen = sprintf(buf,"%lld",(0LL + rand()) >> 20);
+                        break;
+                    case 1:
+                        buflen = sprintf(buf,"%lld",(0LL + rand()));
+                        break;
+                    case 2:
+                        buflen = sprintf(buf,"%lld",(0LL + rand()) << 20);
+                        break;
+                    default:
+                        assert(NULL);
+                    }
                 }
 
                 /* Add to ziplist */
-                zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), where);
+                zl = ziplistPush(zl, (unsigned char*)buf, buflen, where);
 
                 /* Add to reference list */
                 if (where == ZIPLIST_HEAD) {
-                    listAddNodeHead(ref,sdsnew(buf));
+                    listAddNodeHead(ref,sdsnewlen(buf, buflen));
                 } else if (where == ZIPLIST_TAIL) {
-                    listAddNodeTail(ref,sdsnew(buf));
+                    listAddNodeTail(ref,sdsnewlen(buf, buflen));
                 } else {
                     assert(NULL);
                 }
@@ -1295,12 +1297,13 @@ int main(int argc, char **argv) {
 
                 assert(ziplistGet(p,&sstr,&slen,&sval));
                 if (sstr == NULL) {
-                    sprintf(buf,"%lld",sval);
+                    buflen = sprintf(buf,"%lld",sval);
                 } else {
-                    memcpy(buf,sstr,slen);
-                    buf[slen] = '\0';
+                    buflen = slen;
+                    memcpy(buf,sstr,buflen);
+                    buf[buflen] = '\0';
                 }
-                assert(strcmp(buf,listNodeValue(refnode)) == 0);
+                assert(memcmp(buf,listNodeValue(refnode),buflen) == 0);
             }
             zfree(zl);
             listRelease(ref);

tests/integration/aof.tcl

@@ -31,13 +31,14 @@ tags {"aof"} {
     }
 
     start_server_aof [list dir $server_path] {
-        test {Unfinished MULTI: Server should not have been started} {
-            is_alive $srv
-        } {0}
+        test "Unfinished MULTI: Server should not have been started" {
+            assert_equal 0 [is_alive $srv]
+        }
 
-        test {Unfinished MULTI: Server should have logged an error} {
-            exec cat [dict get $srv stdout] | tail -n1
-        } {*Unexpected end of file reading the append only file*}
+        test "Unfinished MULTI: Server should have logged an error" {
+            set result [exec cat [dict get $srv stdout] | tail -n1]
+            assert_match "*Unexpected end of file reading the append only file*" $result
+        }
     }
 
     ## Test that the server exits when the AOF contains a short read
@@ -47,36 +48,57 @@ tags {"aof"} {
     }
 
     start_server_aof [list dir $server_path] {
-        test {Short read: Server should not have been started} {
-            is_alive $srv
-        } {0}
+        test "Short read: Server should not have been started" {
+            assert_equal 0 [is_alive $srv]
+        }
 
-        test {Short read: Server should have logged an error} {
-            exec cat [dict get $srv stdout] | tail -n1
-        } {*Bad file format reading the append only file*}
+        test "Short read: Server should have logged an error" {
+            set result [exec cat [dict get $srv stdout] | tail -n1]
+            assert_match "*Bad file format reading the append only file*" $result
+        }
     }
 
     ## Test that redis-check-aof indeed sees this AOF is not valid
-    test {Short read: Utility should confirm the AOF is not valid} {
+    test "Short read: Utility should confirm the AOF is not valid" {
         catch {
             exec src/redis-check-aof $aof_path
-        } str
-        set _ $str
-    } {*not valid*}
+        } result
+        assert_match "*not valid*" $result
+    }
 
-    test {Short read: Utility should be able to fix the AOF} {
-        exec echo y | src/redis-check-aof --fix $aof_path
-    } {*Successfully truncated AOF*}
+    test "Short read: Utility should be able to fix the AOF" {
+        set result [exec echo y | src/redis-check-aof --fix $aof_path]
+        assert_match "*Successfully truncated AOF*" $result
+    }
 
     ## Test that the server can be started using the truncated AOF
     start_server_aof [list dir $server_path] {
-        test {Fixed AOF: Server should have been started} {
-            is_alive $srv
-        } {1}
+        test "Fixed AOF: Server should have been started" {
+            assert_equal 1 [is_alive $srv]
+        }
 
-        test {Fixed AOF: Keyspace should contain values that were parsable} {
+        test "Fixed AOF: Keyspace should contain values that were parsable" {
             set client [redis [dict get $srv host] [dict get $srv port]]
-            list [$client get foo] [$client get bar]
-        } {hello {}}
+            assert_equal "hello" [$client get foo]
+            assert_equal "" [$client get bar]
+        }
+    }
+
+    ## Test that SPOP (that modifies the client its argc/argv) is correctly free'd
+    create_aof {
+        append_to_aof [formatCommand sadd set foo]
+        append_to_aof [formatCommand sadd set bar]
+        append_to_aof [formatCommand spop set]
+    }
+
+    start_server_aof [list dir $server_path] {
+        test "AOF+SPOP: Server should have been started" {
+            assert_equal 1 [is_alive $srv]
+        }
+
+        test "AOF+SPOP: Set should have 1 member" {
+            set client [redis [dict get $srv host] [dict get $srv port]]
+            assert_equal 1 [$client scard set]
+        }
     }
 }

tests/integration/replication.tcl

@@ -27,6 +27,8 @@ start_server {tags {"repl"}} {
         test {MASTER and SLAVE consistency with expire} {
             createComplexDataset r 50000 useexpire
             after 4000 ;# Make sure everything expired before taking the digest
+            r keys *   ;# Force DEL syntesizing to slave
+            after 1000 ;# Wait another second. Now everything should be fine.
             if {[r debug digest] ne [r -1 debug digest]} {
                 set csv1 [csvdump r]
                 set csv2 [csvdump {r -1}]

tests/test_helper.tcl

@@ -108,6 +108,7 @@ proc cleanup {} {
 }
 
 proc execute_everything {} {
+    execute_tests "unit/printver"
     execute_tests "unit/auth"
     execute_tests "unit/protocol"
     execute_tests "unit/basic"

tests/unit/printver.tcl

@@ -0,0 +1,6 @@
+start_server {} {
+    set i [r info]
+    regexp {redis_version:(.*?)\r\n} $i - version
+    regexp {redis_git_sha1:(.*?)\r\n} $i - sha1
+    puts "Testing Redis version $version ($sha1)"
+}

tests/unit/type/zset.tcl

@@ -484,6 +484,13 @@ start_server {tags {"zset"}} {
     test {ZINTERSTORE with AGGREGATE MAX} {
         list [r zinterstore zsetc 2 zseta zsetb aggregate max] [r zrange zsetc 0 -1 withscores]
     } {2 {b 2 c 3}}
+
+    test {ZINTERSTORE regression with two sets, intset+hashtable} {
+        r del seta setb setc
+        r sadd set1 a
+        r sadd set2 10
+        r zinterstore set3 2 set1 set2
+    } {0}
     
     foreach cmd {ZUNIONSTORE ZINTERSTORE} {
         test "$cmd with +inf/-inf scores" {