From 64adf015b1ecc36bb64eada1acca3d46f9877a07 Mon Sep 17 00:00:00 2001 From: John Sully Date: Mon, 1 Jun 2020 23:36:01 -0400 Subject: [PATCH 1/8] Undecorated new is OK to use Former-commit-id: 5b885bb1649805f6a2edb8d28edd1447bb6c4843 --- src/new.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/new.h b/src/new.h index fc7ea926e..265d7ad69 100644 --- a/src/new.h +++ b/src/new.h @@ -4,7 +4,6 @@ void *operator new(size_t size, enum MALLOC_CLASS mclass); #ifndef SANITIZE -[[deprecated]] void *operator new(size_t size); void operator delete(void * p) noexcept; From 779023beffa4ddcc2d962b9df949b4f8aac3fc0a Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 5 Jun 2020 21:35:47 -0400 Subject: [PATCH 2/8] Ensure CRON jobs run in a clean environment Former-commit-id: c6dce838b7cc94e115fd73a64dda663f0a2c28c5 --- src/ae.cpp | 4 ++-- src/ae.h | 2 +- src/expire.cpp | 30 ++++++++++++++++++++---------- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/ae.cpp b/src/ae.cpp index ab5eeb163..7387e2a7f 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -285,9 +285,9 @@ int aePostFunction(aeEventLoop *eventLoop, aePostFunctionProc *proc, void *arg) return AE_OK; } -int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynchronous, bool fLock) +int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynchronous, bool fLock, bool fForceQueue) { - if (eventLoop == g_eventLoopThisThread) + if (eventLoop == g_eventLoopThisThread && !fForceQueue) { fn(); return AE_OK; diff --git a/src/ae.h b/src/ae.h index 3f1ddbf06..fdd444d3a 100644 --- a/src/ae.h +++ b/src/ae.h @@ -135,7 +135,7 @@ aeEventLoop *aeCreateEventLoop(int setsize); int aePostFunction(aeEventLoop *eventLoop, aePostFunctionProc *proc, void *arg); #ifdef __cplusplus } // EXTERN C -int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynchronous = false, bool fLock = true); +int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynchronous = false, bool fLock = true, bool fForceQueue = false); extern "C" { #endif void aeDeleteEventLoop(aeEventLoop *eventLoop); diff --git a/src/expire.cpp b/src/expire.cpp index 123f55817..b7f648117 100644 --- a/src/expire.cpp +++ b/src/expire.cpp @@ -82,6 +82,7 @@ void activeExpireCycleExpire(redisDb *db, expireEntry &e, long long now) { robj objKey; initStaticStringObject(objKey, (char*)e.key()); + bool fTtlChanged = false; while (!pfat->FEmpty()) { @@ -128,7 +129,15 @@ void activeExpireCycleExpire(redisDb *db, expireEntry &e, long long now) { break; case OBJ_CRON: - executeCronJobExpireHook(e.key(), val); + { + sds keyCopy = sdsdup(e.key()); + incrRefCount(val); + aePostFunction(g_pserver->rgthreadvar[IDX_EVENT_LOOP_MAIN].el, [keyCopy, val]{ + executeCronJobExpireHook(keyCopy, val); + sdsfree(keyCopy); + decrRefCount(val); + }, false, true /*fLock*/, true /*fForceQueue*/); + } return; case OBJ_LIST: @@ -141,19 +150,20 @@ void activeExpireCycleExpire(redisDb *db, expireEntry &e, long long now) { propagateSubkeyExpire(db, val->type, &objKey, &objSubkey); pfat->popfrontExpireEntry(); + fTtlChanged = true; + } + + if (!pfat->FEmpty() && fTtlChanged) + { + // We need to resort the expire entry since it may no longer be in the correct position + auto itr = db->setexpire->find(e.key()); + expireEntry eT = std::move(e); + db->setexpire->erase(itr); + db->setexpire->insert(eT); } if (deleted) { - if (!pfat->FEmpty()) - { - // We need to resort the expire entry since it may no longer be in the correct position - auto itr = db->setexpire->find(e.key()); - expireEntry eT = std::move(e); - db->setexpire->erase(itr); - db->setexpire->insert(eT); - } - switch (val->type) { case OBJ_SET: From 8f8015bb224dc35e06e19decba353251095bcecf Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 5 Jun 2020 22:18:28 -0400 Subject: [PATCH 3/8] Detect issues like #189 Former-commit-id: 0042586190a3e5b212a015aeb3577695cd3623c5 --- tests/unit/expire.tcl | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/unit/expire.tcl b/tests/unit/expire.tcl index 67b2db4c2..16006d7da 100644 --- a/tests/unit/expire.tcl +++ b/tests/unit/expire.tcl @@ -291,4 +291,26 @@ start_server {tags {"expire"}} { fail "Server reported corrupt subexpire" } } + + test {Stress subkey expires} { + r flushall + set rd [redis_deferring_client] + set rd2 [redis_deferring_client] + $rd2 multi + for {set j 0} {$j < 1000} {incr j} { + for {set k 0} {$k < 1000} {incr k} { + $rd hset "hash_$k" "field_$j" "foo" + $rd expiremember "hash_$k" "field_$j" [expr int(floor(rand() * 1000))] ms + if {rand() < 0.3} { + $rd2 hdel "hash_$k" "field_$j" + } + if {rand() < 0.01} { + $rd del "hash_$k" + } + } + } + $rd2 exec + after 3000 + assert_equal [r dbsize] 0 + } } From ebf5d2826a8c61d8026983209a5e78c7b81cebb4 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 7 Jun 2020 00:40:58 -0400 Subject: [PATCH 4/8] Make lazyfree more reliable Former-commit-id: b6c3d80edbdc4348c071b4108b039f4da5c88ce5 --- tests/unit/lazyfree.tcl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/lazyfree.tcl b/tests/unit/lazyfree.tcl index 8efb3aecd..be3939fd2 100644 --- a/tests/unit/lazyfree.tcl +++ b/tests/unit/lazyfree.tcl @@ -17,7 +17,9 @@ start_server {tags {"lazyfree"}} { fail "Memory is not reclaimed by UNLINK" } } +} +start_server {tags {"lazyfree"}} { test "FLUSHDB ASYNC can reclaim memory in background" { set orig_mem [s used_memory] set args {} From c60bf228b41d6ea1e049c35dd1ed1e5d58d3dc38 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 7 Jun 2020 00:44:11 -0400 Subject: [PATCH 5/8] fix race in test Former-commit-id: 02e821530a402796697e63c68e768b563de0a3cb --- tests/unit/dump.tcl | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/dump.tcl b/tests/unit/dump.tcl index 062d803b5..aa0f6ee11 100644 --- a/tests/unit/dump.tcl +++ b/tests/unit/dump.tcl @@ -245,6 +245,7 @@ start_server {tags {"dump"}} { set rd [redis_deferring_client] $rd debug sleep 1.0 ; # Make second server unable to reply. + after 100 set e {} catch {r -1 migrate $second_host $second_port key 9 500} e assert_match {IOERR*} $e From 7384abfe5610f101340f989004664e9fd1f755c5 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 7 Jun 2020 01:14:57 -0400 Subject: [PATCH 6/8] replication test race Former-commit-id: e1f3cd6ec3bf2319484de04c3796dcfa75e0479c --- tests/integration/replication.tcl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/replication.tcl b/tests/integration/replication.tcl index b67f5428c..68f1ff69c 100644 --- a/tests/integration/replication.tcl +++ b/tests/integration/replication.tcl @@ -160,7 +160,7 @@ start_server {tags {"repl"}} { test {FLUSHALL should replicate} { r -1 flushall - if {$::valgrind} {after 2000} + if {$::valgrind} {after 2000} else {after 500} list [r -1 dbsize] [r 0 dbsize] } {0 0} From 97908941bfbacd184af7f37f8447d0a7ab72d981 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 7 Jun 2020 13:59:41 -0400 Subject: [PATCH 7/8] Stream tests rely on deferring clients, mitigate races caused by them Former-commit-id: 2caf8e4c8095215b189942b7eaec3bf5023f7fcf --- tests/unit/type/stream.tcl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index c2b524d7f..144a207b9 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -176,6 +176,7 @@ start_server { r XADD s2 * old abcd1234 set rd [redis_deferring_client] $rd XREAD BLOCK 20000 STREAMS s1 s2 s3 $ $ $ + after 100 r XADD s2 * new abcd1234 set res [$rd read] assert {[lindex $res 0 0] eq {s2}} @@ -185,6 +186,7 @@ start_server { test {Blocking XREAD waiting old data} { set rd [redis_deferring_client] $rd XREAD BLOCK 20000 STREAMS s1 s2 s3 $ 0-0 $ + after 100 r XADD s2 * foo abcd1234 set res [$rd read] assert {[lindex $res 0 0] eq {s2}} @@ -198,6 +200,7 @@ start_server { r XDEL s1 667 set rd [redis_deferring_client] $rd XREAD BLOCK 10 STREAMS s1 666 + after 100 after 20 assert {[$rd read] == {}} ;# before the fix, client didn't even block, but was served synchronously with {s1 {}} } @@ -206,6 +209,7 @@ start_server { set rd [redis_deferring_client] r del s1 $rd XREAD BLOCK 20000 STREAMS s1 $ + after 100 r multi r XADD s1 * old abcd1234 r DEL s1 @@ -220,6 +224,7 @@ start_server { set rd [redis_deferring_client] r del s1 $rd XREAD BLOCK 20000 STREAMS s1 $ + after 100 r multi r XADD s1 * old abcd1234 r DEL s1 @@ -236,6 +241,7 @@ start_server { r XADD s2 * old abcd1234 set rd [redis_deferring_client] $rd XREAD BLOCK 20000 STREAMS s2 s2 s2 $ $ $ + after 100 r XADD s2 * new abcd1234 set res [$rd read] assert {[lindex $res 0 0] eq {s2}} @@ -246,6 +252,7 @@ start_server { r XADD s2 * old abcd1234 set rd [redis_deferring_client] $rd XREAD BLOCK 20000 STREAMS s2 s2 s2 $ $ $ + after 100 r MULTI r XADD s2 * field one r XADD s2 * field two @@ -353,6 +360,7 @@ start_server { r del x set rd [redis_deferring_client] $rd XREAD BLOCK 0 STREAMS x 1-18446744073709551615 + after 100 r XADD x 1-1 f v r XADD x 1-18446744073709551615 f v r XADD x 2-1 f v From 0120d23a4816576ce651c4e76b56be614febcfa7 Mon Sep 17 00:00:00 2001 From: John Sully Date: Sun, 7 Jun 2020 15:06:50 -0400 Subject: [PATCH 8/8] Solo tests should work with loopn Former-commit-id: bc7ea8fb20c71ace6c35559791e2e8dc6bf4097b --- tests/test_helper.tcl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 625ef9602..52acb1747 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -215,7 +215,7 @@ proc s {args} { # test server, so that the test server will send them again to # clients once the clients are idle. proc run_solo {name code} { - if {$::numclients == 1 || $::loop || $::external} { + if {$::numclients == 1 || $::loop < 0 || $::external} { # run_solo is not supported in these scenarios, just run the code. eval $code return