From 3c2ea1ea9516724a361f38ffd2fdfb13aa0b041a Mon Sep 17 00:00:00 2001 From: Binbin Date: Tue, 20 Feb 2024 17:12:19 +0800 Subject: [PATCH] Fix wathced client test timing issue caused by late close (#13062) There is a timing issue in the test, close may arrive late, or in freeClientAsync we will free the client in async way, which will lead to errors in watching_clients statistics, since we will only unwatch all keys when we truly freeClient. Add a wait here to avoid this problem. Also fixed some outdated comments i saw. The test was introduced in #12966. --- src/networking.c | 2 +- tests/support/util.tcl | 8 ++++++++ tests/test_helper.tcl | 2 +- tests/unit/info.tcl | 5 +++-- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/networking.c b/src/networking.c index d36d64dee..5d0167fc1 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1722,7 +1722,7 @@ void freeClient(client *c) { zfree(c); } -/* Schedule a client to free it at a safe time in the serverCron() function. +/* Schedule a client to free it at a safe time in the beforeSleep() function. * This function is useful when we need to terminate a client but we are in * a context where calling freeClient() is not possible, because the client * should be valid for the continuation of the flow of the program. */ diff --git a/tests/support/util.tcl b/tests/support/util.tcl index 9b9ea0ac1..0270e9222 100644 --- a/tests/support/util.tcl +++ b/tests/support/util.tcl @@ -925,6 +925,14 @@ proc wait_for_blocked_clients_count {count {maxtries 100} {delay 10} {idx 0}} { } } +proc wait_for_watched_clients_count {count {maxtries 100} {delay 10} {idx 0}} { + wait_for_condition $maxtries $delay { + [s $idx watching_clients] == $count + } else { + fail "Timeout waiting for watched clients" + } +} + proc read_from_aof {fp} { # Input fp is a blocking binary file descriptor of an opened AOF file. if {[gets $fp count] == -1} return "" diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 6623d059e..ea000b9d4 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -284,7 +284,7 @@ proc redis_client {args} { set args [lrange $args 1 end] } - # create client that defers reading reply + # create client that won't defers reading reply set client [redis [srv $level "host"] [srv $level "port"] 0 $::tls] # select the right db and read the response (OK), or at least ping diff --git a/tests/unit/info.tcl b/tests/unit/info.tcl index 1852717ab..cb4397cc8 100644 --- a/tests/unit/info.tcl +++ b/tests/unit/info.tcl @@ -458,9 +458,10 @@ start_server {tags {"info" "external:skip"}} { # unwatch without watch has no effect r unwatch assert_equal [s watching_clients] 1 - # after disconnect + # after disconnect, since close may arrive later, or the client may + # be freed asynchronously, we use a wait_for_condition $r2 close - assert_equal [s watching_clients] 0 + wait_for_watched_clients_count 0 } } }