Client should not block multiple times on the same key.

Sending a command like:

BLPOP foo foo foo foo 0

Resulted into a crash before this commit since the client ended being
inserted in the waiting list for this key multiple times.
This resulted into the function handleClientsBlockedOnLists() to fail
because we have code like that:

    if (de) {
        list *clients = dictGetVal(de);
        int numclients = listLength(clients);

        while(numclients--) {
            listNode *clientnode = listFirst(clients);

            /* server clients here... */
        }
    }

The code to serve clients used to remove the served client from the
waiting list, so if a client is blocking multiple times, eventually the
call to listFirst() will return NULL or worse will access random memory
since the list may no longer exist as it is removed by the function
unblockClientWaitingData() if there are no more clients waiting for this
list.

To avoid making the rest of the implementation more complex, this commit
modifies blockForKeys() so that a client will be put just a single time
into the waiting list for a given key.

Since it is Saturday, I hope this fixes issue #801.

diff --git a/src/t_list.c b/src/t_list.c
index 7d7e83cbb702c73d1e5ec0e3d82f9f05e0cc1231..1bd81c2e1c8c2075d49a44fec20ff9fd2dd7a8b8 100644 (file)
--- a/src/t_list.c
+++ b/src/t_list.c
@@ -753,22 +753,33 @@ void rpoplpushCommand(redisClient *c) {
 /* Set a client in blocking mode for the specified key, with the specified
  * timeout */
 void blockForKeys(redisClient *c, robj **keys, int numkeys, time_t timeout, robj *target) {
+    dict *added;
     dictEntry *de;
     list *l;
-    int j;
+    int j, i;
 
     c->bpop.keys = zmalloc(sizeof(robj*)*numkeys);
-    c->bpop.count = numkeys;
     c->bpop.timeout = timeout;
     c->bpop.target = target;
 
-    if (target != NULL) {
-        incrRefCount(target);
-    }
+    if (target != NULL) incrRefCount(target);
+
+    /* Create a dictionary that we use to avoid adding duplicated keys
+     * in case the user calls something like: "BLPOP foo foo foo 0".
+     * The rest of the implementation is simpler if we know there are no
+     * duplications in the key waiting list. */
+    added = dictCreate(&setDictType,NULL);
 
+    i = 0; /* The index for c->bpop.keys[...], we can't use the j loop
+              variable as the list of keys may have duplicated elements. */
     for (j = 0; j < numkeys; j++) {
+        /* Add the key in the "added" dictionary to make sure there are 
+         * no duplicated keys. */
+        if (dictAdd(added,keys[j],NULL) != DICT_OK) continue;
+        incrRefCount(keys[j]);
+
         /* Add the key in the client structure, to map clients -> keys */
-        c->bpop.keys[j] = keys[j];
+        c->bpop.keys[i++] = keys[j];
         incrRefCount(keys[j]);
 
         /* And in the other "side", to map keys -> clients */
@@ -786,9 +797,12 @@ void blockForKeys(redisClient *c, robj **keys, int numkeys, time_t timeout, robj
         }
         listAddNodeTail(l,c);
     }
+    c->bpop.count = i;
+
     /* Mark the client as a blocked client */
     c->flags |= REDIS_BLOCKED;
     server.bpop_blocked_clients++;
+    dictRelease(added);
 }
 
 /* Unblock a client that's waiting in a blocking operation such as BLPOP */