From e9ae03787e0a2e0484914737f82bfe216f8e9d52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Tue, 22 Feb 2022 11:09:46 +0100 Subject: [PATCH] Delete key doesn't dirty client who watched stale key (#10256) When WATCH is called on a key that's already logically expired, avoid discarding the transaction when the keys is actually deleted. When WATCH is called, a flag is stored if the key is already expired at the time of watch. The expired key is not deleted, only checked. When a key is "touched", if it is deleted and it was already expired when a client watched it, the client is not marked as dirty. Co-authored-by: Oran Agra Co-authored-by: zhaozhao.zz --- src/db.c | 22 ++++++------ src/multi.c | 59 +++++++++++++++++++++++++----- tests/unit/multi.tcl | 85 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 20 deletions(-) diff --git a/src/db.c b/src/db.c index c2477d678..d28349664 100644 --- a/src/db.c +++ b/src/db.c @@ -1340,6 +1340,11 @@ int dbSwapDatabases(int id1, int id2) { redisDb aux = server.db[id1]; redisDb *db1 = &server.db[id1], *db2 = &server.db[id2]; + /* Swapdb should make transaction fail if there is any + * client watching keys */ + touchAllWatchedKeysInDb(db1, db2); + touchAllWatchedKeysInDb(db2, db1); + /* Swap hash tables. Note that we don't swap blocking_keys, * ready_keys and watched_keys, since we want clients to * remain in the same DB they were. */ @@ -1361,14 +1366,9 @@ int dbSwapDatabases(int id1, int 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. - * - * Also the swapdb should make transaction fail if there is any - * client watching keys */ + * if needed. */ scanDatabaseForReadyLists(db1); - touchAllWatchedKeysInDb(db1, db2); scanDatabaseForReadyLists(db2); - touchAllWatchedKeysInDb(db2, db1); return C_OK; } @@ -1387,6 +1387,10 @@ void swapMainDbWithTempDb(redisDb *tempDb) { redisDb aux = server.db[i]; redisDb *activedb = &server.db[i], *newdb = &tempDb[i]; + /* Swapping databases should make transaction fail if there is any + * client watching keys. */ + touchAllWatchedKeysInDb(activedb, newdb); + /* Swap hash tables. Note that we don't swap blocking_keys, * ready_keys and watched_keys, since clients * remain in the same DB they were. */ @@ -1408,12 +1412,8 @@ void swapMainDbWithTempDb(redisDb *tempDb) { * 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. - * - * Also the swapdb should make transaction fail if there is any - * client watching keys. */ + * if needed. */ scanDatabaseForReadyLists(activedb); - touchAllWatchedKeysInDb(activedb, newdb); } trackingInvalidateKeysOnFlush(1); diff --git a/src/multi.c b/src/multi.c index a98195672..42426a2d6 100644 --- a/src/multi.c +++ b/src/multi.c @@ -257,10 +257,13 @@ void execCommand(client *c) { /* In the client->watched_keys list we need to use watchedKey structures * as in order to identify a key in Redis we need both the key name and the - * DB */ + * DB. This struct is also referenced from db->watched_keys dict, where the + * values are lists of watchedKey pointers. */ typedef struct watchedKey { robj *key; redisDb *db; + client *client; + unsigned expired:1; /* Flag that we're watching an already expired key. */ } watchedKey; /* Watch for the specified key */ @@ -284,13 +287,15 @@ void watchForKey(client *c, robj *key) { dictAdd(c->db->watched_keys,key,clients); incrRefCount(key); } - listAddNodeTail(clients,c); /* Add the new key to the list of keys watched by this client */ wk = zmalloc(sizeof(*wk)); wk->key = key; + wk->client = c; wk->db = c->db; + wk->expired = keyIsExpired(c->db, key); incrRefCount(key); listAddNodeTail(c->watched_keys,wk); + listAddNodeTail(clients,wk); } /* Unwatch all the keys watched by this client. To clean the EXEC dirty @@ -305,12 +310,12 @@ void unwatchAllKeys(client *c) { list *clients; watchedKey *wk; - /* Lookup the watched key -> clients list and remove the client + /* Lookup the watched key -> clients list and remove the client's wk * from the list */ wk = listNodeValue(ln); clients = dictFetchValue(wk->db->watched_keys, wk->key); serverAssertWithInfo(c,NULL,clients != NULL); - listDelNode(clients,listSearchKey(clients,c)); + listDelNode(clients,listSearchKey(clients,wk)); /* Kill the entry at all if this was the only client */ if (listLength(clients) == 0) dictDelete(wk->db->watched_keys, wk->key); @@ -321,8 +326,8 @@ void unwatchAllKeys(client *c) { } } -/* iterates over the watched_keys list and - * look for an expired key . */ +/* Iterates over the watched_keys list and looks for an expired key. Keys which + * were expired already when WATCH was called are ignored. */ int isWatchedKeyExpired(client *c) { listIter li; listNode *ln; @@ -331,6 +336,7 @@ int isWatchedKeyExpired(client *c) { listRewind(c->watched_keys,&li); while ((ln = listNext(&li))) { wk = listNodeValue(ln); + if (wk->expired) continue; /* was expired when WATCH was called */ if (keyIsExpired(wk->db, wk->key)) return 1; } @@ -352,13 +358,31 @@ void touchWatchedKey(redisDb *db, robj *key) { /* Check if we are already watching for this key */ listRewind(clients,&li); while((ln = listNext(&li))) { - client *c = listNodeValue(ln); + watchedKey *wk = listNodeValue(ln); + client *c = wk->client; + + if (wk->expired) { + /* The key was already expired when WATCH was called. */ + if (db == wk->db && + equalStringObjects(key, wk->key) && + dictFind(db->dict, key->ptr) == NULL) + { + /* Already expired key is deleted, so logically no change. Clear + * the flag. Deleted keys are not flagged as expired. */ + wk->expired = 0; + goto skip_client; + } + break; + } c->flags |= CLIENT_DIRTY_CAS; /* As the client is marked as dirty, there is no point in getting here * again in case that key (or others) are modified again (or keep the * memory overhead till EXEC). */ unwatchAllKeys(c); + + skip_client: + continue; } } @@ -379,14 +403,31 @@ void touchAllWatchedKeysInDb(redisDb *emptied, redisDb *replaced_with) { dictIterator *di = dictGetSafeIterator(emptied->watched_keys); while((de = dictNext(di)) != NULL) { robj *key = dictGetKey(de); - if (dictFind(emptied->dict, key->ptr) || + int exists_in_emptied = dictFind(emptied->dict, key->ptr) != NULL; + if (exists_in_emptied || (replaced_with && dictFind(replaced_with->dict, key->ptr))) { list *clients = dictGetVal(de); if (!clients) continue; listRewind(clients,&li); while((ln = listNext(&li))) { - client *c = listNodeValue(ln); + watchedKey *wk = listNodeValue(ln); + if (wk->expired) { + if (!replaced_with || !dictFind(replaced_with->dict, key->ptr)) { + /* Expired key now deleted. No logical change. Clear the + * flag. Deleted keys are not flagged as expired. */ + wk->expired = 0; + continue; + } else if (keyIsExpired(replaced_with, key)) { + /* Expired key remains expired. */ + continue; + } + } else if (!exists_in_emptied && keyIsExpired(replaced_with, key)) { + /* Non-existing key is replaced with an expired key. */ + wk->expired = 1; + continue; + } + client *c = wk->client; c->flags |= CLIENT_DIRTY_CAS; /* As the client is marked as dirty, there is no point in getting here * again for others keys (or keep the memory overhead till EXEC). */ diff --git a/tests/unit/multi.tcl b/tests/unit/multi.tcl index 8a0b731d5..70e33b74b 100644 --- a/tests/unit/multi.tcl +++ b/tests/unit/multi.tcl @@ -147,6 +147,45 @@ start_server {tags {"multi"}} { r debug set-active-expire 1 } {OK} {needs:debug} + test {WATCH stale keys should not fail EXEC} { + r del x + r debug set-active-expire 0 + r set x foo px 1 + after 2 + r watch x + r multi + r ping + assert_equal {PONG} [r exec] + r debug set-active-expire 1 + } {OK} {needs:debug} + + test {Delete WATCHed stale keys should not fail EXEC} { + r del x + r debug set-active-expire 0 + r set x foo px 1 + after 2 + r watch x + # EXISTS triggers lazy expiry/deletion + assert_equal 0 [r exists x] + r multi + r ping + assert_equal {PONG} [r exec] + r debug set-active-expire 1 + } {OK} {needs:debug} + + test {FLUSHDB while watching stale keys should not fail EXEC} { + r del x + r debug set-active-expire 0 + r set x foo px 1 + after 2 + r watch x + r flushdb + r multi + r ping + assert_equal {PONG} [r exec] + r debug set-active-expire 1 + } {OK} {needs:debug} + test {After successful EXEC key is no longer watched} { r set x 30 r watch x @@ -245,6 +284,52 @@ start_server {tags {"multi"}} { r exec } {} {singledb:skip} + test {SWAPDB does not touch watched stale keys} { + r flushall + r select 1 + r debug set-active-expire 0 + r set x foo px 1 + after 2 + r watch x + r swapdb 0 1 ; # expired key replaced with no key => no change + r multi + r ping + assert_equal {PONG} [r exec] + r debug set-active-expire 1 + } {OK} {singledb:skip needs:debug} + + test {SWAPDB does not touch non-existing key replaced with stale key} { + r flushall + r select 0 + r debug set-active-expire 0 + r set x foo px 1 + after 2 + r select 1 + r watch x + r swapdb 0 1 ; # no key replaced with expired key => no change + r multi + r ping + assert_equal {PONG} [r exec] + r debug set-active-expire 1 + } {OK} {singledb:skip needs:debug} + + test {SWAPDB does not touch stale key replaced with another stale key} { + r flushall + r debug set-active-expire 0 + r select 1 + r set x foo px 1 + r select 0 + r set x bar px 1 + after 2 + r select 1 + r watch x + r swapdb 0 1 ; # no key replaced with expired key => no change + r multi + r ping + assert_equal {PONG} [r exec] + r debug set-active-expire 1 + } {OK} {singledb:skip needs:debug} + test {WATCH is able to remember the DB a key belongs to} { r select 5 r set x 30