From abd44c8393d33363357c98c947e51ae0aff7f262 Mon Sep 17 00:00:00 2001
From: Oran Agra <oran@redislabs.com>
Date: Tue, 22 Jun 2021 07:35:59 +0300
Subject: [PATCH] Fix race in client side tracking (#9116)

The `Tracking gets notification of expired keys` test in tracking.tcl
used to hung in valgrind CI quite a lot.

It turns out the reason is that with valgrind and a busy machine, the
server cron active expire cycle could easily run in the same event loop
as the command that created `mykey`, so that when they key got expired,
there were two change events to broadcast, one that set the key and one
that expired it, but since we used raxTryInsert, the client that was
associated with the "last" change was the one that created the key, so
the NOLOOP filtered that event.

This commit adds a test that reproduces the problem by using lazy expire
in a multi-exec which makes sure the key expires in the same event loop
as the one that added it.

(cherry picked from commit 9b564b525d8ce88295ec14ffdc3bede7e5f5c33e)
---
 src/tracking.c          |  2 +-
 tests/unit/tracking.tcl | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/tracking.c b/src/tracking.c
index a11e4b7d7..a57f062c6 100644
--- a/src/tracking.c
+++ b/src/tracking.c
@@ -326,7 +326,7 @@ void trackingRememberKeyToBroadcast(client *c, char *keyname, size_t keylen) {
          * tree. This way we know who was the client that did the last
          * change to the key, and can avoid sending the notification in the
          * case the client is in NOLOOP mode. */
-        raxTryInsert(bs->keys,(unsigned char*)keyname,keylen,c,NULL);
+        raxInsert(bs->keys,(unsigned char*)keyname,keylen,c,NULL);
     }
     raxStop(&ri);
 }
diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl
index 4c75b6f48..217a057dd 100644
--- a/tests/unit/tracking.tcl
+++ b/tests/unit/tracking.tcl
@@ -132,6 +132,22 @@ start_server {tags {"tracking network"}} {
         assert {$keys eq {mykey}}
     }
 
+    test {Tracking gets notification of lazy expired keys} {
+        r CLIENT TRACKING off
+        r CLIENT TRACKING on BCAST REDIRECT $redir_id NOLOOP
+        # Use multi-exec to expose a race where the key gets an two invalidations
+        # in the same event loop, once by the client so filtered by NOLOOP, and
+        # the second one by the lazy expire
+        r MULTI
+        r SET mykey{t} myval px 1
+        r SET mykeyotherkey{t} myval ; # We should not get it
+        r DEBUG SLEEP 0.1
+        r GET mykey{t}
+        r EXEC
+        set keys [lsort [lindex [$rd_redirection read] 2]]
+        assert {$keys eq {mykey{t}}}
+    } {} {needs:debug}
+
     test {HELLO 3 reply is correct} {
         set reply [r HELLO 3]
         assert_equal [dict get $reply proto] 3