Skip to content

Commit

Permalink
prevent client tracking from causing feedback loop in performEvictions (
Browse files Browse the repository at this point in the history
#8100)

When client tracking is enabled signalModifiedKey can increase memory usage,
this can cause the loop in performEvictions to keep running since it was measuring
the memory usage impact of signalModifiedKey.

The section that measures the memory impact of the eviction should be just on dbDelete,
excluding keyspace notification, client tracking, and propagation to AOF and replicas.

This resolves part of the problem described in #8069
p.s. fix took 1 minute, test took about 3 hours to write.
  • Loading branch information
oranagra committed Dec 6, 2020
1 parent 1df5bb5 commit c4fdf09
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/evict.c
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,8 @@ int performEvictions(void) {
* we are freeing removing the key, but we can't account for
* that otherwise we would never exit the loop.
*
* Same for CSC invalidation messages generated by signalModifiedKey.
*
* AOF and Output buffer memory will be freed eventually so
* we only care about memory used by the key space. */
delta = (long long) zmalloc_used_memory();
Expand All @@ -637,12 +639,12 @@ int performEvictions(void) {
dbAsyncDelete(db,keyobj);
else
dbSyncDelete(db,keyobj);
signalModifiedKey(NULL,db,keyobj);
latencyEndMonitor(eviction_latency);
latencyAddSampleIfNeeded("eviction-del",eviction_latency);
delta -= (long long) zmalloc_used_memory();
mem_freed += delta;
server.stat_evictedkeys++;
signalModifiedKey(NULL,db,keyobj);
notifyKeyspaceEvent(NOTIFY_EVICTED, "evicted",
keyobj, db->id);
decrRefCount(keyobj);
Expand Down
52 changes: 52 additions & 0 deletions tests/unit/maxmemory.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -260,3 +260,55 @@ start_server {tags {"maxmemory"}} {
r dbsize
} {4098}
}

start_server {tags {"maxmemory"}} {
test {client tracking don't cause eviction feedback loop} {
r config set maxmemory 0
r config set maxmemory-policy allkeys-lru
r config set maxmemory-eviction-tenacity 100

# 10 clients listening on tracking messages
set clients {}
for {set j 0} {$j < 10} {incr j} {
lappend clients [redis_deferring_client]
}
foreach rd $clients {
$rd HELLO 3
$rd read ; # Consume the HELLO reply
$rd CLIENT TRACKING on
$rd read ; # Consume the CLIENT reply
}

# populate 300 keys, with long key name and short value
for {set j 0} {$j < 300} {incr j} {
set key $j[string repeat x 1000]
r set $key x

# for each key, enable caching for this key
foreach rd $clients {
$rd get $key
$rd read
}
}

# set the memory limit which will cause a few keys to be evicted
# we need to make sure to evict keynames of a total size of more than
# 16kb since the (PROTO_REPLY_CHUNK_BYTES), only after that the
# invalidation messages have a chance to trigger further eviction.
set used [s used_memory]
set limit [expr {$used - 40000}]
r config set maxmemory $limit

# make sure some eviction happened
set evicted [s evicted_keys]
if {$::verbose} { puts "evicted: $evicted" }
assert_range $evicted 10 50
foreach rd $clients {
$rd read ;# make sure we have some invalidation message waiting
$rd close
}

# make sure we didn't drain the database
assert_range [r dbsize] 200 300
}
}

0 comments on commit c4fdf09

Please sign in to comment.