valkey-cli doesn't auto exit from subscribed mode on reaching zero subscription #1432

Nikhil-Manglore · 2024-12-12T22:05:52Z

Resolves issue with valkey-cli not auto exiting from subscribed mode on reaching zero pub/sub subscription (previously filled on Redis) redis/redis#12592

Signed-off-by: Nikhil Manglore <[email protected]>

codecov · 2024-12-12T23:43:58Z

Codecov Report

Attention: Patch coverage is 88.63636% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.71%. Comparing base (b3b4bdc) to head (6743126).

Files with missing lines	Patch %	Lines
src/valkey-cli.c	88.63%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1432      +/-   ##
============================================
- Coverage     70.83%   70.71%   -0.13%     
============================================
  Files           120      120              
  Lines         64911    64955      +44     
============================================
- Hits          45982    45932      -50     
- Misses        18929    19023      +94

Files with missing lines	Coverage Δ
src/valkey-cli.c	`54.25% <88.63%> (-1.24%)`	⬇️

... and 15 files with indirect coverage changes

zuiderkwast

Hello! Welcome to Valkey!

I remember this issue. :)

src/valkey-cli.c

Signed-off-by: Nikhil Manglore <[email protected]>

zuiderkwast

Logic looks correct, but I have two comments.

I think it would be good to add a test for valkey-cli that uses SUBSCRIBE, PSUBSCRIBE, SSUBSCRIBE in the same session, an exits pubsub mode when all three are unsubscribed.

zuiderkwast · 2024-12-18T22:28:54Z

src/valkey-cli.c

+    /* Handle pubsub mode */
+    if (config.pubsub_mode) {
+        if (isPubsubPush(reply)) {
+            if (reply->elements >= 3) {
+                char *cmd = reply->element[0]->str;
+                int count = reply->element[2]->integer;
+
+                if (strcmp(cmd, "subscribe") == 0 || strcmp(cmd, "unsubscribe") == 0 ||
+                    strcmp(cmd, "psubscribe") == 0 || strcmp(cmd, "punsubscribe") == 0) {
+                    config.pubsub_unsharded_count = count;
+                } else if (strcmp(cmd, "ssubscribe") == 0 || strcmp(cmd, "sunsubscribe") == 0) {
+                    config.pubsub_sharded_count = count;
+                }
+
+                if (config.pubsub_unsharded_count == 0 && config.pubsub_sharded_count == 0) {
+                    config.pubsub_mode = 0;
+                    cliRefreshPrompt();
+                }
+            }
+        }
+    }


The logic looks right to me. I think we should move it though.

You have put this logic in cliReadReply, but I think we should put it where cliReadReply is called, or just a few lines below that. That's where most of the client state is handled per command and it's also where the logic is that sets config.pubsub_mode = 1;, so it seems more logical to put this in the same place. The reply can be accessed as config.last_reply.

Somewhere here:

/* Read response, possibly skipping pubsub/push messages. */ while (1) { if (cliReadReply(output_raw) != REDIS_OK) { zfree(argvlen); return REDIS_ERR; } fflush(stdout); if (config.pubsub_mode || num_expected_pubsub_push > 0) { if (isPubsubPush(config.last_reply)) { if (num_expected_pubsub_push > 0 && !strcasecmp(config.last_reply->element[0]->str, command)) { /* This pushed message confirms the * [p|s][un]subscribe command. */ if (is_subscribe && !config.pubsub_mode) { config.pubsub_mode = 1; cliRefreshPrompt(); }

tests/unit/pubsub.tcl

Signed-off-by: Nikhil Manglore <[email protected]>

zuiderkwast

Good, thanks. I have some new comments. :)

src/valkey-cli.c

tests/integration/valkey-cli.tcl

zuiderkwast · 2024-12-19T12:33:25Z

tests/integration/valkey-cli.tcl

+        # Verify that we've exited pubsub mode
+        assert_equal "PONG" [run_command $fd "PING"]


This only verifies that the server believes that the client is not in pubsub mode. It would work even without these cli fixes.

What we need to test is that the client know that we have exited pubsub mode. Maybe we can check that the prompt is back to normal 127.0.0.1:6379> instead of 127.0.0.1:6379(subscribed mode)>? I don't know if this test framework can do this. I hope so.

Thanks for the suggestions, I pushed the updated code. Let me know if you want this check handled another way.

Signed-off-by: Nikhil Manglore <[email protected]>

zuiderkwast

It's getting better! I'm not sure if check for "(subscribed mode)" in the prompt works as intended though.

Maybe we need to test this in some other way. If you can't solve it, then I'll try to think of something.

zuiderkwast · 2024-12-20T10:23:59Z

src/valkey-cli.c

+                        if (config.pubsub_unsharded_count == 0 && config.pubsub_sharded_count == 0) {
+                            config.pubsub_mode = 0;
+                            cliRefreshPrompt();
+                        } else {
+                            config.pubsub_mode = 1;
+                        }


There is already a check that sets pubsub_mode = 1 below. It becomes redundant with this change. We should only do this in one place.

I think it's better to do it here, but we also need to call cliRefreshPrompt and we should do it only if we're not already in pubsub mode. I suggest like this:

Suggested change

if (config.pubsub_unsharded_count == 0 && config.pubsub_sharded_count == 0) {

config.pubsub_mode = 0;

cliRefreshPrompt();

} else {

config.pubsub_mode = 1;

}

if (config.pubsub_unsharded_count == 0 && config.pubsub_sharded_count == 0) {

if (config.pubsub_mode) {

config.pubsub_mode = 0;

cliRefreshPrompt();

}

} else if (!config.pubsub_mode) {

config.pubsub_mode = 1;

cliRefreshPrompt();

}

And this code below can be deleted:

if (is_subscribe && !config.pubsub_mode) { config.pubsub_mode = 1; cliRefreshPrompt(); }

I was manually testing the code and it seems that when we just use a unsubscribe command to unsubscribe to all channels the count isn't updated properly. So although we are able to unsubscribe to all channels we won't be able to automatically exit subscribe mode. I will continue to look into it.

When unsubscribing all channels, the server sends multiple unsubscribe messages and the last one should have count 0. So we should be able to detect this.

I think now that this code only detects the first unsubscribe message. The rest are received in cliWaitForMessagesOrStdin.

This code:

} else if (FD_ISSET(context->fd, &readfds)) { /* Message from the server */ if (cliReadReply(0) != REDIS_OK) { cliPrintContextError(); exit(1); } fflush(stdout);

It also calls cliReadReply. Maybe your first solution where the code was inside cliReadReply could actually detect the unsubscribe all channels case. We can put it back there if you want. But can we also do the check to set subscribed node to 1 here? It's good to have the code for pubsub_mode = 1 and pubsub_mode = 0 in the same place.

Sorry for the delay. I reverted to the previous commit where the code was in cliReadReply and I still seem to get the same issue even though I'm sure it was working last week when I was manually testing. I put a print statement to debug and here is the output

127.0.0.1:6379> subscribe ch1 ch2 ch3 1) "subscribe" 2) "ch1" 3) (integer) 1 1) "subscribe" 2) "ch2" 3) (integer) 2 1) "subscribe" 2) "ch3" 3) (integer) 3 127.0.0.1:6379(subscribed mode)> unsubscribe 1) "unsubscribe" 2) "ch3" 3) (integer) 2 Command: unsubscribe, Count: 2 1) "unsubscribe" 2) "ch2" 3) (integer) 1 1) "unsubscribe" 2) "ch1" 3) (integer) 0

So it seems to only get the reply of the first unsubscribe instead of all of them

I will continue to look into this issue but am currently unsure why this is occurring

@Nikhil-Manglore How is the deep dive going here?

I just fixed the issue by adding code to check for multiple unsubscribe messages in the method cliWaitForMessagesOrStdin(). I will push that code now and begin fixing the test cases.

zuiderkwast · 2024-12-20T10:35:59Z

tests/integration/valkey-cli.tcl

+        # Verify that we've exited pubsub mode
+        set response [run_command $fd "PING"]
+
+        if {[string first "(subscribed mode)" $response] >= 0} {
+            return -code error "Client is still in pubsub mode"
+        }


Interesting! Does this work though? Does the response really contain the prompt?

We should add the opposite check when we enter subscribed mode, to see that the prompt does include "(subscribed mode)" when we are in subscribed mode. In this way, we also know that this kind of check works.

I've done some source code reading. The prompt is only printed when STDIN is a TTY. When we run the cli from the tests, the prompt is not printed.

(The code for this is in the command line editor linenoise (deps/linenoise/linenoise.c), function linenoise(), which calls linenoiseNoTTY which doesn't print the prompt.)

So, to test if valkey-cli thinks that it's in pubsub mode or not, we need to do something else. Either we can modify linenoise to add some way to make it behave like an interactive terminal even it it's not running in a terminal. That seems complicated though. I have another idea.

There is some prepared work in valkey-cli for "internal commands", starting with ":".

not connected> :banana unknown valkey-cli internal command ':banana'

The only such commands right now are :set nohints and :set hints to disable or enable syntax hints.

We could add an internal command to be able to check if we're in pubsub mode. I think for example :get prompt (to print the config.prompt) would be simple enough and useful for testing. WDYT?

I have pushed the updated code. I was able to implement all functionality while being able to refactor the code as we had initially discussed. I also was able to implement your suggestion into the test cases and they work.

Also I don't think we can delete this code because otherwise the client never enters subscribe mode

if (is_subscribe && !config.pubsub_mode) { config.pubsub_mode = 1; cliRefreshPrompt(); } ```

…mplementing test cases Signed-off-by: Nikhil Manglore <[email protected]>

…y into subscribe-mode-bug

hpatro · 2025-01-03T21:57:04Z

src/valkey-cli.c

+                                } else {
+                                    config.pubsub_mode = 1;
+                                    cliRefreshPrompt();
+                                }


If we are already in pubsub_mode, do we need to perform this ?

hpatro · 2025-01-03T22:09:30Z

src/valkey-cli.c

+                        if (strcmp(cmd, "unsubscribe") == 0) {
+                            config.pubsub_unsharded_count = count;
+                        } else if (strcmp(cmd, "sunsubscribe") == 0) {
+                            config.pubsub_sharded_count = count;
+                        }


How are we dealing with punsubscribe here?

hpatro · 2025-01-03T22:15:01Z

tests/integration/valkey-cli.tcl

@@ -608,6 +608,67 @@ if {!$::tls} { ;# fake_redis_node doesn't support TLS
        assert_equal "a\n1\nb\n2\nc\n3" [exec {*}$cmdline ZRANGE new_zset 0 -1 WITHSCORES]
    }

+    test "valkey-cli pubsub mode with multiple subscription types" {


Could we split them up into smaller test cases?

hpatro · 2025-01-03T22:17:01Z

src/valkey-cli.c

@@ -2246,7 +2249,32 @@ static void cliWaitForMessagesOrStdin(void) {
                sds out = cliFormatReply(reply, config.output, 0);
                fwrite(out, sdslen(out), 1, stdout);
                fflush(stdout);
+
+                // Handle unsubscribe responses
+                if (isPubsubPush(reply) && reply->elements >= 3) {


This code block seems like duplication of the code below. Could you check if we can place it in one place and handle all scenarios.

Fixed issue with exiting subscribe mode

c21bf9d

Signed-off-by: Nikhil Manglore <[email protected]>

Nikhil-Manglore force-pushed the subscribe-mode-bug branch from a7fb910 to c21bf9d Compare December 12, 2024 22:41

zuiderkwast reviewed Dec 12, 2024

View reviewed changes

src/valkey-cli.c Outdated Show resolved Hide resolved

src/valkey-cli.c Outdated Show resolved Hide resolved

fixed issue with the counters when exit subscribe mode

cb71718

Signed-off-by: Nikhil Manglore <[email protected]>

zuiderkwast reviewed Dec 18, 2024

View reviewed changes

Nikhil-Manglore added 2 commits December 19, 2024 00:17

refactored code in valkey-cli.cand fixed the test cases

926a713

Signed-off-by: Nikhil Manglore <[email protected]>

fixed typo in the comments

2278fce

Signed-off-by: Nikhil Manglore <[email protected]>

zuiderkwast reviewed Dec 19, 2024

View reviewed changes

Refactored code again and added new tests to the test case

0c4a177

Signed-off-by: Nikhil Manglore <[email protected]>

zuiderkwast reviewed Dec 20, 2024

View reviewed changes

Nikhil-Manglore and others added 6 commits December 20, 2024 12:05

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

398c482

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

a047778

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

791a80c

Merge branch 'valkey-io:unstable' into subscribe-mode-bug

d6c863e

Fixed all issues with subscribe mode, refactored code, and finished i…

79aff57

…mplementing test cases Signed-off-by: Nikhil Manglore <[email protected]>

Merge branch 'subscribe-mode-bug' of github.com:Nikhil-Manglore/valke…

6743126

…y into subscribe-mode-bug

hpatro reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

valkey-cli doesn't auto exit from subscribed mode on reaching zero subscription #1432

valkey-cli doesn't auto exit from subscribed mode on reaching zero subscription #1432

Nikhil-Manglore commented Dec 12, 2024 •

edited by hpatro

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

zuiderkwast left a comment

zuiderkwast left a comment

zuiderkwast Dec 18, 2024 •

edited

Loading

zuiderkwast left a comment

zuiderkwast Dec 19, 2024

Nikhil-Manglore Dec 19, 2024

zuiderkwast left a comment

zuiderkwast Dec 20, 2024

Nikhil-Manglore Dec 23, 2024

zuiderkwast Dec 24, 2024

Nikhil-Manglore Dec 27, 2024 •

edited

Loading

hpatro Jan 2, 2025

Nikhil-Manglore Jan 2, 2025

zuiderkwast Dec 20, 2024

zuiderkwast Dec 21, 2024

Nikhil-Manglore Jan 3, 2025 •

edited

Loading

hpatro Jan 3, 2025

hpatro Jan 3, 2025

hpatro Jan 3, 2025

hpatro Jan 3, 2025

		# Verify that we've exited pubsub mode
		assert_equal "PONG" [run_command $fd "PING"]

valkey-cli doesn't auto exit from subscribed mode on reaching zero subscription #1432

Are you sure you want to change the base?

valkey-cli doesn't auto exit from subscribed mode on reaching zero subscription #1432

Conversation

Nikhil-Manglore commented Dec 12, 2024 • edited by hpatro Loading

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nikhil-Manglore Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nikhil-Manglore Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nikhil-Manglore commented Dec 12, 2024 •

edited by hpatro

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

zuiderkwast Dec 18, 2024 •

edited

Loading

Nikhil-Manglore Dec 27, 2024 •

edited

Loading

Nikhil-Manglore Jan 3, 2025 •

edited

Loading