Update PR #6537: use a fresh time outside call().fix-expires-6537

One problem with the solution proposed so far in #6537 is that key
lookups outside a command execution via call(), still used a cached
time. The cached time needed to be refreshed in multiple places,
especially because of modules callbacks from timers, cluster bus, and
thread safe contexts, that may use RM_Open().

In order to avoid this problem, this commit introduces the ability to
detect if we are inside call(): this way we can use the reference fixed
time only when we are in the context of a command execution or Lua
script, but for the asynchronous lookups, we can still use mstime() to
get a fresh time reference.

3 files changed, 27 insertions, 7 deletions

diff --git a/src/db.c b/src/db.c
index 6c0b190a8..5ea3087fc 100644
--- a/src/db.c
+++ b/src/db.c
@@ -1188,6 +1188,7 @@ void propagateExpire(redisDb *db, robj *key, int lazy) {
 /* Check if the key is expired. */
 int keyIsExpired(redisDb *db, robj *key) {
     mstime_t when = getExpire(db,key);
+    mstime_t now;
 
     if (when < 0) return 0; /* No expire for this key */
 
@@ -1198,13 +1199,27 @@ int keyIsExpired(redisDb *db, robj *key) {
      * blocked to when the Lua script started. This way a key can expire
      * only the first time it is accessed and not in the middle of the
      * script execution, making propagation to slaves / AOF consistent.
-     * See issue #1525 on Github for more information.
-     *
-     * Outside the Lua script execution, we use the cached time server.mstime
-     * that is updated before commands executions in call(). */
-    mstime_t now = server.lua_caller ? server.lua_time_start :
-                                       server.mstime;
+     * See issue #1525 on Github for more information. */
+    if (server.lua_caller) {
+        now = server.lua_time_start;
+    }
+    /* If we are in the middle of a command execution, we still want to use
+     * a reference time that does not change: in that case we just use the
+     * cached time, that we update before each call in the call() function.
+     * This way we avoid that commands such as RPOPLPUSH or similar, that
+     * may re-open the same key multiple times, can invalidate an already
+     * open object in a next call, if the next call will see the key expired,
+     * while the first did not. */
+    else if (server.call_depth > 0) {
+        now = server.mstime;
+    }
+    /* For the other cases, we want to use the most fresh time we have. */
+    else {
+        now = mstime();
+    }
 
+    /* The key expired if the current (virtual or real) time is greater
+     * than the expire time of the key. */
     return now > when;
 }
 
diff --git a/src/server.c b/src/server.c
index 2e5d8193b..acf8eebbc 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2780,6 +2780,7 @@ void initServer(void) {
     server.hz = server.config_hz;
     server.pid = getpid();
     server.current_client = NULL;
+    server.call_depth = 0;
     server.clients = listCreate();
     server.clients_index = raxNew();
     server.clients_to_close = listCreate();
@@ -3252,6 +3253,8 @@ void call(client *c, int flags) {
     int client_old_flags = c->flags;
     struct redisCommand *real_cmd = c->cmd;
 
+    server.call_depth++;
+
     /* Sent the command to clients in MONITOR mode, only if the commands are
      * not generated from reading an AOF. */
     if (listLength(server.monitors) &&
@@ -3377,6 +3380,7 @@ void call(client *c, int flags) {
             trackingRememberKeys(caller);
     }
 
+    server.call_depth--;
     server.stat_numcommands++;
 }
 
diff --git a/src/server.h b/src/server.h
index 24dbc079c..4a497c47c 100644
--- a/src/server.h
+++ b/src/server.h
@@ -1134,7 +1134,8 @@ struct redisServer {
     list *clients_pending_write; /* There is to write or install handler. */
     list *clients_pending_read;  /* Client has pending read socket buffers. */
     list *slaves, *monitors;    /* List of slaves and MONITORs */
-    client *current_client; /* Current client, only used on crash report */
+    client *current_client;     /* Current client executing the command. */
+    long call_depth;            /* call() re-entering count. */
     rax *clients_index;         /* Active clients dictionary by client ID. */
     int clients_paused;         /* True if clients are currently paused */
     mstime_t clients_pause_end_time; /* Time when we undo clients_paused */