From 33f943b4cddb0bf90b436ccd47d12363e13d0fce Mon Sep 17 00:00:00 2001
From: Matt Stancliff <matt@genges.com>
Date: Mon, 7 Apr 2014 21:22:30 -0400
Subject: [PATCH] Fix blocking operations from missing new lists

Behrad Zari discovered [1] and Josiah reported [2]: if you block
and wait for a list to exist, but the list creates from
a non-push command, the blocked client never gets notified.

This commit adds notification of blocked clients into
the DB layer and away from individual commands.

Lists can be created by [LR]PUSH, SORT..STORE, RENAME, MOVE,
and RESTORE.  Previously, blocked client notifications were
only triggered by [LR]PUSH.  Your client would never get
notified if a list were created by SORT..STORE or RENAME or
a RESTORE, etc.

Blocked client notification now happens in one unified place:
  - dbAdd() triggers notification when adding a list to the DB

Two new tests are added that fail prior to this commit.

All test pass.

Fixes #1668

[1]: https://groups.google.com/forum/#!topic/redis-db/k4oWfMkN1NU
[2]: #1668
---
 src/db.c                 |  1 +
 src/redis.h              |  1 +
 src/t_list.c             | 16 +++++-----------
 tests/unit/type/list.tcl | 23 +++++++++++++++++++++++
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/src/db.c b/src/db.c
index f849e5247..291f3c2b8 100644
--- a/src/db.c
+++ b/src/db.c
@@ -95,6 +95,7 @@ void dbAdd(redisDb *db, robj *key, robj *val) {
     int retval = dictAdd(db->dict, copy, val);
 
     redisAssertWithInfo(NULL,key,retval == REDIS_OK);
+    if (val->type == REDIS_LIST) signalListAsReady(db, key);
     if (server.cluster_enabled) slotToKeyAdd(key);
  }
 
diff --git a/src/redis.h b/src/redis.h
index e6b7ea93b..db06abef8 100644
--- a/src/redis.h
+++ b/src/redis.h
@@ -1035,6 +1035,7 @@ void listTypeConvert(robj *subject, int enc);
 void unblockClientWaitingData(redisClient *c);
 void handleClientsBlockedOnLists(void);
 void popGenericCommand(redisClient *c, int where);
+void signalListAsReady(redisDb *db, robj *key);
 
 /* MULTI/EXEC/WATCH... */
 void unwatchAllKeys(redisClient *c);
diff --git a/src/t_list.c b/src/t_list.c
index 70f5cf164..d03879bdd 100644
--- a/src/t_list.c
+++ b/src/t_list.c
@@ -29,8 +29,6 @@
 
 #include "redis.h"
 
-void signalListAsReady(redisClient *c, robj *key);
-
 /*-----------------------------------------------------------------------------
  * List API
  *----------------------------------------------------------------------------*/
@@ -297,15 +295,12 @@ void listTypeConvert(robj *subject, int enc) {
 void pushGenericCommand(redisClient *c, int where) {
     int j, waiting = 0, pushed = 0;
     robj *lobj = lookupKeyWrite(c->db,c->argv[1]);
-    int may_have_waiting_clients = (lobj == NULL);
 
     if (lobj && lobj->type != REDIS_LIST) {
         addReply(c,shared.wrongtypeerr);
         return;
     }
 
-    if (may_have_waiting_clients) signalListAsReady(c,c->argv[1]);
-
     for (j = 2; j < c->argc; j++) {
         c->argv[j] = tryObjectEncoding(c->argv[j]);
         if (!lobj) {
@@ -709,7 +704,6 @@ void rpoplpushHandlePush(redisClient *c, robj *dstkey, robj *dstobj, robj *value
     if (!dstobj) {
         dstobj = createZiplistObject();
         dbAdd(c->db,dstkey,dstobj);
-        signalListAsReady(c,dstkey);
     }
     signalModifiedKey(c->db,dstkey);
     listTypePush(dstobj,value,REDIS_HEAD);
@@ -849,19 +843,19 @@ void unblockClientWaitingData(redisClient *c) {
  * made by a script or in the context of MULTI/EXEC.
  *
  * The list will be finally processed by handleClientsBlockedOnLists() */
-void signalListAsReady(redisClient *c, robj *key) {
+void signalListAsReady(redisDb *db, robj *key) {
     readyList *rl;
 
     /* No clients blocking for this key? No need to queue it. */
-    if (dictFind(c->db->blocking_keys,key) == NULL) return;
+    if (dictFind(db->blocking_keys,key) == NULL) return;
 
     /* Key was already signaled? No need to queue it again. */
-    if (dictFind(c->db->ready_keys,key) != NULL) return;
+    if (dictFind(db->ready_keys,key) != NULL) return;
 
     /* Ok, we need to queue this key into server.ready_keys. */
     rl = zmalloc(sizeof(*rl));
     rl->key = key;
-    rl->db = c->db;
+    rl->db = db;
     incrRefCount(key);
     listAddNodeTail(server.ready_keys,rl);
 
@@ -869,7 +863,7 @@ void signalListAsReady(redisClient *c, robj *key) {
      * to avoid adding it multiple times into a list with a simple O(1)
      * check. */
     incrRefCount(key);
-    redisAssert(dictAdd(c->db->ready_keys,key,NULL) == DICT_OK);
+    redisAssert(dictAdd(db->ready_keys,key,NULL) == DICT_OK);
 }
 
 /* This is a helper function for handleClientsBlockedOnLists(). It's work
diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl
index e665afc0a..c8e26602b 100644
--- a/tests/unit/type/list.tcl
+++ b/tests/unit/type/list.tcl
@@ -408,6 +408,29 @@ start_server {
       $rd read
     } {}
 
+    test "BLPOP when new key is moved into place" {
+        set rd [redis_deferring_client]
+
+        $rd blpop foo 5
+        r lpush bob abc def hij
+        r rename bob foo
+        $rd read
+    } {foo hij}
+
+    test "BLPOP when result key is created by SORT..STORE" {
+        set rd [redis_deferring_client]
+
+        # zero out list from previous test without explicit delete
+        r lpop foo
+        r lpop foo
+        r lpop foo
+
+        $rd blpop foo 5
+        r lpush notfoo hello hola aguacate konichiwa zanzibar
+        r sort notfoo ALPHA store foo
+        $rd read
+    } {foo aguacate}
+
     foreach {pop} {BLPOP BRPOP} {
         test "$pop: with single empty list argument" {
             set rd [redis_deferring_client]
-- 
GitLab