redis-cli: Accept commands in subscribed mode by zuiderkwast · Pull Request #11873 · redis/redis

zuiderkwast · 2023-03-02T10:43:31Z

The message "Reading messages... (press Ctrl-C to quit)" is replaced by "Reading messages... (press Ctrl-C to quit or any key to type command)".

This allows users to subscribe to more channels, to try out UNSUBSCRIBE and to combine pubsub with other features such as push messages from client tracking.

The "Reading messages" info message is displayed in the bottom of the output in a distinct style and moves downward as more messages appear. When any key is pressed, the info message is replaced by the prompt with for entering commands. After entering a command and the reply is displayed, the "Reading messages" info messages appears again. This is added to the repl loop in redis-cli and in the corresponding place for non-interactive mode.

An indication "(subscribed mode)" is included in the prompt when entering commands in subscribed mode.

Also:

Fixes a problem that UNSUBSCRIBE hanged when used with RESP3 and push callback, without first entering subscribe mode. It hanged because UNSUBSCRIBE gets one or more push replies but no in-band reply.
Exit subscribed mode after RESET.

The message "Reading messages... (press Ctrl-C to quit)" is replaced by "Reading messages... (press Ctrl-C to quit or enter to type command)". When enter is pressed, a prompt for entering commands appear. After entering one command and the reply is displayed, you get the "Reading messages..." again. This is added to the repl loop in redis-cli and in the corresponding place for non-interactive mode. This allows users to subscribe to more channels, to try out UNSUBSCRIBE and to combine pubsub with other features such as push messages from client tracking. An indication "(subscribed mode)" is included in the prompt when entering commands in subscribed mode. Also: * Fix problem that UNSUBSCRIBE hanged when used with RESP3 and push callback, without first entering subscribe mode. It hanged because UNSUBSCRIBE gets one or more push replies but no in-band reply. * Fix problem that UNSUBSCRIBE with 2+ channels with RESP2 without first entering subscribed mode caused the request-response pairs to get out of sync. The responses for UNSUBSCRIBE ch1 ch2; PING results appeared as one unsubscribe message for ch1 as a response to UNSUBSCRIBE and the unsubscribe message for ch2 as a response to PING. (Pong appeared when the next command after that is entered.) * Exit subscribed mode after RESET. * Set the config.resp3 variable after HELLO, based on its reply.

bjosv · 2023-03-03T12:29:25Z

Very nice!
As an early tester (and probably lack of insights) I've seen that every second PING response after a subscribe is not printed, like below. Not sure if this is a legacy problem, and it's the same with RESP2.

./src/redis-cli -3 -p 7000
127.0.0.1:7000> subscribe mychan
Reading messages... (press Ctrl-C to quit or enter to type command)
1) "subscribe"
2) "mychan"
3) (integer) 1
PING                                                                  <--- No PONG printed after this
127.0.0.1:7000(subscribed mode)> PING
PONG
Reading messages... (press Ctrl-C to quit or enter to type command)
PING                                                                  <--- No PONG printed after this
127.0.0.1:7000(subscribed mode)>

zuiderkwast · 2023-03-03T13:26:47Z

@bjosv You can only type commands at the prompt. You need to press enter first to make the prompt appear. Linenoise (the readline clone) consumes all pending input. That's why.

zuiderkwast · 2023-03-06T18:31:49Z

@bjosv I changed "press enter" to "press any key" to avoid the confusion you experienced. I hope it's more intuitive now.

Added tests for TTY and non-TTY.

zuiderkwast · 2023-03-06T18:40:50Z

Some screenshots:

bjosv

I seems that my terminal is not showing the "Reading messages.. .." anymore.

After some investigations my TERM=xterm-256color using solarized dark don't like
the color \033[1;90m ("Bold, bright color.").

Maybe the black color wont work for other dark mode users either.

zuiderkwast · 2023-03-08T11:10:16Z

@bjosv The color 90 "bright black (gray)" is the same color as already used for command hints. I guess you don't see hints either then?

bjosv · 2023-03-08T13:01:08Z

Nope, seems that I'm missing out of some good stuff 😳
Well, you last change works fine for me even not seeing the text, it's intuitive.
Update: This is an issue on my side, I seem to have a weird palette..

oranagra · 2023-03-09T10:26:23Z

wow.. that's a lot of CLI code (hope i'll find time to review it soon).
btw, maybe one of you is brave enough to review the redis-cli portion of #10515? and then i can merge it.

oranagra

LGTM.
hope i'm not missing any corner cases.
please run some use cases (interactive, non-interactive, piped from stdin) via valgrind.
@yossigo please take a quick look and state if you see any problem with it.

src/redis-cli.c

…-while-subscribed

yossigo · 2023-03-14T14:59:22Z

This looks really nice! @zuiderkwast Did you consider actually using the first keypress that breaks out of the receive loop as input? Intuitively I just started typing but then noticed the first character is "lost".

zuiderkwast · 2023-03-14T15:16:54Z

This looks really nice! @zuiderkwast Did you consider actually using the first keypress that breaks out of the receive loop as input? Intuitively I just started typing but then noticed the first character is "lost".

Thanks for trying it out. :-) Yes, using the "any key" as input would be ideal, but it is consumed by the readline lib (linenoise). I would need to look into modify it in some way then, but I didn't go that far.

zuiderkwast · 2023-03-14T15:53:09Z

@yossigo The "any key" can be used as input if we change the following in linenoise. I don't know if it has any drawbacks. Probably it will just make input available that otherwise would be lost if the user is typing too fast. Is it worth it? WDYT?

diff --git a/deps/linenoise/linenoise.c b/deps/linenoise/linenoise.c
index 1b01622c5..c6c496e34 100644
--- a/deps/linenoise/linenoise.c
+++ b/deps/linenoise/linenoise.c
@@ -257,7 +257,7 @@ static int enableRawMode(int fd) {
     raw.c_cc[VMIN] = 1; raw.c_cc[VTIME] = 0; /* 1 byte, no timer */
 
     /* put terminal in raw mode after flushing */
-    if (tcsetattr(fd,TCSAFLUSH,&raw) < 0) goto fatal;
+    if (tcsetattr(fd,TCSANOW,&raw) < 0) goto fatal;
     rawmode = 1;
     return 0;

oranagra · 2023-03-15T08:50:45Z

i've tested it and i see few things that could maybe improved:

after hitting a key and getting a prompt, if i type enter i get a new line without a prompt, another enter gives me a prompt again, and a 3rd one give me the "reading messages" line again.
if i start typing and don't hit enter, i won't get any push message until i hit enter (even if i delete what i started typing, i won't get the push until i post a command or hit enter).
if some push was received while i type, when i do hit enter, it's hard to distinguish between the post message and the command response (they look similar)

zuiderkwast · 2023-03-15T09:19:35Z

i've tested it and i see few things that could maybe improved:

after hitting a key and getting a prompt, if i type enter i get a new line without a prompt, another enter gives me a prompt again, and a 3rd one give me the "reading messages" line again.

This must have been after the last commit (Use the "any key" as input). The enter goes to the prompt, which goes back to "reading messages..." ~~I don't really know what to do in this case, without breaking the abstraction of linenoise. Any ideas?~~ Solved: It can be detected in linenoise, because it is buffered as NL (10) before entering raw mode, which is distinct from 13 (CR) we get after entering raw mode.

if i start typing and don't hit enter, i won't get any push message until i hit enter (even if i delete what i started typing, i won't get the push until i post a command or hit enter).

That's expected. When the prompt is visible, linenoise is doing a blocking read() on stdin. It doesn't use select() to detect input on the socket here, since this is a separate library which is only concerned about line editing. Do you think I should break the abstraction of this lib and read push messages during line editing, or inject some way to do it, using ifdefs or something like that?

if some push was received while i type, when i do hit enter, it's hard to distinguish between the post message and the command response (they look similar)

Right. This is easier to solve. How about using a different color for push messages? Cyan, magenta...

oranagra · 2023-03-15T09:43:54Z

by "solved" you mean a future commit? i do think i tested the latest.
maybe we can leave that aside, but we do need it to be clear to the user that he's currently not watching for pushes (e.g. if he deleted what he started to type), and what he should do in order to listen again.
we don't use colors for other server outputs. maybe add some indentation or some > prefix? (we better do that only in interactive mode)

zuiderkwast · 2023-03-15T11:04:10Z

by "solved" you mean a future commit? i do think i tested the latest.

Exactly :-) Sorry for the confusion.

maybe we can leave that aside, but we do need it to be clear to the user that he's currently not watching for pushes (e.g. if he deleted what he started to type), and what he should do in order to listen again.

Isn't it clear enough that when the "Reading messages..." is displayed, push messages are displayed continuously, but when the prompt is displayed, it can't be assumed that they are?

we don't use colors for other server outputs. maybe add some indentation or some > prefix? (we better do that only in interactive mode)

OK, for RESP3 we can use a prefix for all push messages. I think that's a good idea.

For RESP2 though, the user can't really tell what's a "push" and what's a reply. There is no difference. That's why only a few commands are allowed in RESP2 pubsub mode. The user can disambiguate by looking at the replies.

oranagra · 2023-03-15T12:07:50Z

src/redis-cli.c

            char numsep;
            if (r->type == REDIS_REPLY_SET) numsep = '~';
            else if (r->type == REDIS_REPLY_MAP) numsep = '#';
+            else if (r->type == REDIS_REPLY_PUSH) numsep = '>';


aren't we introducing a breaking change (for scripts that work with redis-cli)?
i thought to do that only in interactive mode, but actually that would be confusing (someone experimenting with interactive in order to know how to write his script).
@yossigo please advise.

This output is only for TTYs, so script shouldn't be affected unless they're trying to simulate a TTY (like we do in the TTY tests).

It seems redis-cli --no-raw in scripts is affected, so it's breaking change for this use case.

All other types have their own prefix, so it's odd that push doesn't. Push shares the prefix with array (1), 2) etc.).

OTOH, this is not coupled with this PR so I can move it to a separate PR.

I spoke to Yossi, let's drop this > change, and add it in 8.0

oranagra · 2023-03-15T12:15:16Z

Isn't it clear enough that when the "Reading messages..." is displayed, push messages are displayed continuously, but when the prompt is displayed, it can't be assumed that they are?

i suppose you're right. testing the new last version and knowing that seems that it works right.

looking for the > i now realize that i tested pubsub and not push, so the > didn't help me, and the responses are still mixed.

127.0.0.1:6379(subscribed mode)> ping
1) "message"
2) "asdf"
3) "asdfasdf111asdf"
1) "pong"
2) ""

another interesting thing i just noticed, that while in that subscribed mode, i'm getting another line (2) ""), can you explain it?

zuiderkwast · 2023-03-15T12:21:08Z

looking for the > i now realize that i tested pubsub and not push, so the > didn't help me, and the responses are still mixed.

Well, try HELLO 3 and then SUBSCRIBE x y z, then you'll see the >.

another interesting thing i just noticed, that while in that subscribed mode, i'm getting another line (2) ""), can you explain it?

It is explained on the page https://redis.io/commands/ping/: "If the client is subscribed to a channel or a pattern, it will instead return a multi-bulk with a "pong" in the first position and an empty bulk in the second position, unless an argument is provided in which case it returns a copy of the argument."

comment out the `>` push prompt

The reason is in reply-schemas-validator, the resp of the client we create will be client_default_resp (currently 3): ``` client *createClient(connection *conn) { client *c = zmalloc(sizeof(client)); #ifdef LOG_REQ_RES reqresReset(c, 0); c->resp = server.client_default_resp; #else c->resp = 2; #endif } ``` But current_resp3 in redis-cli will be inconsistent with it, the test adds a simple hello 3 to avoid this failure, test was added in redis#11873. Added help descriptions for log-req-res, force-resp3 and dont-pre-clean options, options was added in redis#10273

The reason is in reply-schemas-validator, the resp of the client we create will be client_default_resp (currently 3): ``` client *createClient(connection *conn) { client *c = zmalloc(sizeof(client)); #ifdef LOG_REQ_RES reqresReset(c, 0); c->resp = server.client_default_resp; #else c->resp = 2; #endif } ``` But current_resp3 in redis-cli will be inconsistent with it, the test adds a simple hello 3 to avoid this failure, test was added in #11873. Added help descriptions for dont-pre-clean option, it was added in #10273

oranagra · 2023-04-02T06:58:48Z

@zuiderkwast maybe you can have a look at a sporadic error from the new test of this PR:
https://github.com/redis/redis/actions/runs/4585861784/jobs/8098245787#step:4:8565

  *** [err]: Interactive CLI: Subscribed mode in tests/integration/redis-cli.tcl
  Expected '1) "unsubscribe"
  2) "ch1"
  3) (integer) 2
  1) "unsubscribe"
  2) "ch2"
  3) (integer) 1
  Reading messages... (press Ctrl-C to quit or any key to type command)
  ' to be equal to '' (context: type eval line 24 cmd {assert_equal $erase$unsub1$unsub2$reading  [run_command $fd "unsubscribe ch1 ch2"]} proc ::test)

maybe a timing issue (freebsd CI is notorious for being slow)

zuiderkwast · 2023-04-04T16:00:10Z

I will take a look in a few days.

zuiderkwast · 2023-04-11T11:25:11Z

I can't tell why this failed. This run_command is like every other command in this test suite. We could try to increase the hard-coded numbers in read_cli, but that would make all checks slower in this test suite.

    proc read_cli {fd} {
        set ret [read $fd]
        while {[string length $ret] == 0} {
            after 10
            set ret [read $fd]
        }

        # We may have a short read, try to read some more.
        set empty_reads 0
        while {$empty_reads < 5} {
            set buf [read $fd]
            if {[string length $buf] == 0} {
                after 10
                incr empty_reads
            } else {
                append ret $buf
                set empty_reads 0
            }
        }
        return $ret
    }

zuiderkwast force-pushed the redis-cli-commands-while-subscribed branch from 725c9af to df7409c Compare March 2, 2023 10:54

zuiderkwast added 3 commits March 6, 2023 14:03

Change to 'press any key' and set TTY accordingly

ddb3eab

Add interactive test for subscribed mode

0b3c677

Add test case for interactive non-TTY CLI and RESP3

09e0409

Fix: reorder functions to avoid 'function undeclared' error

0755e74

zuiderkwast marked this pull request as ready for review March 6, 2023 18:42

bjosv reviewed Mar 8, 2023

View reviewed changes

zuiderkwast mentioned this pull request Mar 8, 2023

[BUG] Wrong response in redis-cli after unsubscribing channels and use another command to get data. #11046

Closed

zuiderkwast requested a review from enjoy-binbin March 8, 2023 16:57

enjoy-binbin mentioned this pull request Mar 9, 2023

redis-cli reads specified number of replies for UNSUBSCRIBE/PUNSUBSCRIBE/SUNSUBSCRIBE #11047

Merged

oranagra approved these changes Mar 12, 2023

View reviewed changes

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

zuiderkwast added 3 commits March 13, 2023 08:12

Merge remote-tracking branch 'redis/unstable' into redis-cli-commands…

d61bcce

…-while-subscribed

CR suggestion

62e84f0

Keep track of current RESP version, separate from options

820f7bf

oranagra approved these changes Mar 13, 2023

View reviewed changes

Use the "any key" as input

d1c7722

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Mar 15, 2023

zuiderkwast added 2 commits March 15, 2023 12:08

Fix strange behviour if enter is pressed before entering readline mode

3db9724

Use '>' as prefix for RESP3 push replies

191da18

oranagra reviewed Mar 15, 2023

View reviewed changes

Update redis-cli.c

d54c69c

comment out the `>` push prompt

oranagra approved these changes Mar 19, 2023

View reviewed changes

update test

336ef39

oranagra merged commit bbf364a into redis:unstable Mar 19, 2023

enjoy-binbin mentioned this pull request Mar 20, 2023

Fix new subscribe mode test in reply-schemas-validator #11939

Merged

zuiderkwast deleted the redis-cli-commands-while-subscribed branch April 11, 2023 11:25

Conversation

zuiderkwast commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjosv commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Mar 6, 2023

Uh oh!

zuiderkwast commented Mar 6, 2023

Uh oh!

bjosv left a comment

Choose a reason for hiding this comment

Uh oh!

zuiderkwast commented Mar 8, 2023

Uh oh!

bjosv commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Mar 9, 2023 • edited 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!

yossigo commented Mar 14, 2023

Uh oh!

zuiderkwast commented Mar 14, 2023

Uh oh!

zuiderkwast commented Mar 14, 2023

Uh oh!

oranagra commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Mar 15, 2023

Uh oh!

zuiderkwast commented Mar 15, 2023

Uh oh!

oranagra Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

oranagra Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

oranagra commented Mar 15, 2023

Uh oh!

zuiderkwast commented Mar 15, 2023

Uh oh!

oranagra commented Apr 2, 2023

Uh oh!

zuiderkwast commented Apr 4, 2023

Uh oh!

zuiderkwast commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zuiderkwast commented Mar 2, 2023 •

edited

Loading

bjosv commented Mar 3, 2023 •

edited

Loading

zuiderkwast commented Mar 3, 2023 •

edited

Loading

bjosv commented Mar 8, 2023 •

edited

Loading

oranagra commented Mar 9, 2023 •

edited

Loading

oranagra commented Mar 15, 2023 •

edited

Loading

zuiderkwast commented Mar 15, 2023 •

edited

Loading