Client tracking tracking-redir-broken push len is 2 not 3 by huangzhw · Pull Request #8456 · redis/redis

huangzhw · 2021-02-07T01:31:31Z

When redis responds with tracking-redir-broken push message (RESP3),
it was responding with a broken protocol: an array of 3 elements, but only
pushes 2 elements.

Some bugs in the test make this pass. Read the push reply
will consume an extra reply, because the reply length is 3, but there
are only two elements, so the next reply will be treated as third
element.

This fix is simple, but I can't pass the test. It takes me a lot of time to figure out what's wrong with the test.

204         set res -1
205         for {set i 0} {$i <= $MAX_TRIES && $res < 0} {incr i} {
206             set res [lsearch -exact [r PING] "tracking-redir-broken"]
207         }

We should set res -1 explicitly, because res currently is key1, this test will always pass.
In for loop condition $res < 0, can't equal 0.

Some bugs in the test exactly make this pass. Read the push reply will consume an extra reply, because the reply length is 3, but there are only two elements, so the next reply will be treated as third element. So the test is corrected too.

oranagra

@huangzhw thanks for catching that.
i think the tests can be further improved.
if you agree, let's use this opportunity

huangzhw · 2021-02-16T06:04:15Z

@oranagra done.

oranagra

LGTM.
I'd like to ask @nitaicaro to take a look if he can.

oranagra · 2021-02-16T14:26:04Z

    }

    test {Invalidations of previous keys can be redirected after switching to RESP3} {
+        r HELLO 2


is this a bugfix for the test?
it meant to swtich protocol, but because other tests were added before it, it didn't get to test what it meant to?

Before this test, r is RESP3. For this test, I think it should switch from 2 to 3.

oranagra · 2021-02-16T14:27:54Z

        assert {$res >= 0}
-    }
+        # Consume PING reply
+        assert_equal PONG [r read]


oranagra · 2021-02-16T14:30:37Z

        # Reinstantiating after QUIT
        set rd_redirection [redis_deferring_client]
        $rd_redirection CLIENT ID
        set redir_id [$rd_redirection read]
        $rd_redirection SUBSCRIBE __redis__:invalidate
        $rd_redirection read ; # Consume the SUBSCRIBE reply


can you fill me in as to why did you conclude this part belongs to the previous test?

Previous test close connection, I think we'd better recover in one test, so next test will not rely on previous one. If we change the test order or insert some test after this, it will be OK.

oranagra · 2021-02-16T14:41:58Z

+        r CLIENT TRACKING off
+        r HELLO 3
+        r CLIENT TRACKING on
        $rd_sg SET key1 1
-        $rd GET key1
-        $rd read ; # Consume the GET reply
-        $rd CLIENT TRACKING off 
-        $rd read ; # Consume the TRACKING reply
-        $rd CLIENT TRACKING on BCAST
-        $rd read ; # Consume the TRACKING reply
+        r GET key1
+        r CLIENT TRACKING off 
+        r CLIENT TRACKING on BCAST
        $rd_sg INCR key1
-        $rd PING
-        set inv_msg [$rd read]
-        set ping_reply [$rd read]
+        set inv_msg [r PING]
+        set ping_reply [r read]


so this test just didn't need to use a differed client..
btw, i recently learned we can change the same client between these two modes:

r deferred 1 r deferred 0

cool. The test is a little hard for me, I never learned tcl.

oranagra · 2021-02-16T14:42:38Z

    }

    test {Tracking gets notification on tracking table key eviction} {
-        $rd_redirection HELLO 2


can you explain why you removed this?

In test {Able to redirect to a RESP3 client}, I make$rd_redirection RESP2. For the principle, change and recovery in the same test.
Besides, when execute $rd_redirection HELLO 2, it doesn't read the reply.

When redis responds with tracking-redir-broken push message (RESP3), it was responding with a broken protocol: an array of 3 elements, but only pushes 2 elements. Some bugs in the test make this pass. Read the push reply will consume an extra reply, because the reply length is 3, but there are only two elements, so the next reply will be treated as third element. So the test is corrected too. Other changes: * checkPrefixCollisionsOrReply success should return 1 instead of -1, this bug didn't have any implications. * improve client tracking tests to validate more of the response it reads.

When redis responds with tracking-redir-broken push message (RESP3), it was responding with a broken protocol: an array of 3 elements, but only pushes 2 elements. (cherry picked from commit f687ac0)

When redis responds with tracking-redir-broken push message (RESP3), it was responding with a broken protocol: an array of 3 elements, but only pushes 2 elements. Some bugs in the test make this pass. Read the push reply will consume an extra reply, because the reply length is 3, but there are only two elements, so the next reply will be treated as third element. So the test is corrected too. Other changes: * checkPrefixCollisionsOrReply success should return 1 instead of -1, this bug didn't have any implications. * improve client tracking tests to validate more of the response it reads.

…-overlap checkPrefixCollisionsOrReply documents a boolean contract: 1 on no collision, 0 on collision (with the error reply already sent). The sole caller in networking.c relies on this: if (!checkPrefixCollisionsOrReply(c, prefix, numprefix)) { zfree(prefix); return; } When a collision between two user-provided prefixes is detected in the inner j-loop, the function returns the outer loop index i instead of 0. At i=0 this happens to be falsy and the caller behaves correctly, which hid the bug from casual testing. For any i>=1 the non-zero value is truthy, the caller falls through to enableTracking(), and the client receives both the error reply and +OK while the overlapping prefixes get registered: > CLIENT TRACKING ON BCAST PREFIX BAZ PREFIX FOOBAR PREFIX FOO -ERR Prefix 'FOOBAR' overlaps with another provided prefix 'FOO'... +OK History ------- The bug has been present since the function was introduced in redis#8176, which added both the function and the boolean-style caller in the same commit. The original contract was three-valued (-1 success, 0 existing-collision, i self-collision) but the caller used !retval, so at i>=1 the self-collision path was silently a pass. redis#8456 later changed the success return from -1 to 1 and tightened the docstring to a boolean contract, but did not touch the `return i` path below. That cleanup codified the boolean contract while leaving the non-boolean exit alive, making the inherited bug easier to spot by reading the function but no easier to catch at runtime. Nothing in the tree consumes the returned index value -- it has always been dead information. This change returns 0 in that path so both error exits match the documented contract. Test ---- Adds tests/unit/tracking.tcl case that triggers a self-collision at i>=1 (BAZ FOOBAR FOO), asserts the error reply, and asserts CLIENT TRACKINGINFO reports `flags off` -- i.e. tracking was not wrongly enabled. Verified the test fails on master without the fix and passes with it. Full unit/tracking suite green. Signed-off-by: Raj Danday <rajkripal.danday@gmail.com>

…-overlap Caller is `if (!checkPrefixCollisionsOrReply(...))` — expects 0 on collision, non-zero on success. The self-collision branch returns the outer loop index `i` instead of 0. At i=0 it's falsy so the caller bails correctly. At i>=1 it's truthy, caller proceeds to enableTracking(), client gets both the error reply AND +OK, overlapping prefixes get registered. Repro: CLIENT TRACKING ON BCAST PREFIX BAZ PREFIX FOOBAR PREFIX FOO -ERR Prefix 'FOOBAR' overlaps with 'FOO' +OK Been there since redis#8176. redis#8456 flipped the success return to match a boolean docstring but left `return i` alone. The index value isn't read anywhere — always dead info. Return 0 and add a regression test. Signed-off-by: Raj Danday <rajkripal.danday@gmail.com>

Caller is `if (!checkPrefixCollisionsOrReply(...))` — expects 0 on collision, non-zero on success. The self-collision branch returns the outer loop index `i` instead of 0. At i=0 it's falsy so the caller bails correctly. At i>=1 it's truthy, caller proceeds to enableTracking(), client gets both the error reply AND +OK, overlapping prefixes get registered. Repro: CLIENT TRACKING ON BCAST PREFIX BAZ PREFIX FOOBAR PREFIX FOO -ERR Prefix 'FOOBAR' overlaps with 'FOO' +OK Been there since redis#8176. redis#8456 flipped the success return to match a boolean docstring but left `return i` alone. The index value isn't read anywhere — always dead info. Return 0 and add a regression test. Signed-off-by: Raj Danday <rajkripal.danday@gmail.com>

oranagra reviewed Feb 7, 2021

View reviewed changes

Comment thread tests/unit/tracking.tcl Outdated

Comment thread tests/unit/tracking.tcl Outdated

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 7, 2021

huangzhw added 3 commits February 16, 2021 13:42

checkPrefixCollisionsOrReply success should return 1 instead of -1

2f4c054

tests/support/redis.tcl add RESP3 SET parse support

13d42a0

improve client tracking test

6ca595c

oranagra reviewed Feb 16, 2021

View reviewed changes

oranagra approved these changes Feb 21, 2021

View reviewed changes

oranagra merged commit f687ac0 into redis:unstable Feb 21, 2021

huangzhw deleted the tracking branch February 21, 2021 08:01

This was referenced Feb 22, 2021

Redis 6.2.0 GA #8525

Merged

Release 6.0.11 #8527

Merged

rajkripal mentioned this pull request Apr 19, 2026

tracking: fix self-overlap returning non-zero loop index #15073

Merged

Uh oh!

Conversation

huangzhw commented Feb 7, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

huangzhw commented Feb 16, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

huangzhw commented Feb 7, 2021 •

edited by oranagra

Loading