Initialize help when using redis-cli help or redis-cli ? #10382

enjoy-binbin · 2022-03-04T02:48:34Z

The following usage will output an empty newline:

> redis-cli help set
empty line

The reason is that in interactive mode, we have called
cliInitHelp, which initializes help.

When using redis-cli help xxx or redis-cli help ? xxx,
we can't match the command due to empty helpEntries,
so we output an empty newline.

In this commit, we will call cliInitHelp to init the help.
Note that in this case, we need to call cliInitHelp (COMMAND DOCS)
every time, which i think is acceptable.

So now the output will look like:

[redis]# src/redis-cli help get

  GET key
  summary: Get the value of a key
  since: 1.0.0
  group: string

[redis]#

Fixes #10378

This PR also fix a redis-cli crash when using --ldb --eval:

[root]# src/redis-cli --ldb --eval test.lua test 1
Lua debugging session started, please use:
quit    -- End the session.
restart -- Restart the script in debug mode again.
help    -- Show Lua script debugging commands.

* Stopped at 1, stop reason = step over
-> 1   local num = redis.call('GET', KEYS[1]);
redis-cli: redis-cli.c:718: cliCountCommands: Assertion
`commandTable->element[i]->type == 1' failed.
Aborted

Because in ldb mode, COMMAND DOCS or COMMAND will
return an array, only with one element, and the type
is REDIS_REPLY_STATUS, the result is <error> Unknown Redis Lua debugger command or wrong number of arguments.

So if we are in the ldb mode, and init the Redis HELP, we
will get the wrong response and crash the redis-cli.
In ldb mode we don't initialize HELP, help is only initialized
after the lua debugging session ends.

It was broken in #10043

The following usage will output an empty newline: ``` > redis-cli help set empty line ``` The reason is that in interactive mode, we have called `cliInitHelp`, which initializes help. When using `redis-cli help xxx` or `redis-cli help ? xxx`, we can't match the command due to empty `helpEntries`, so we output an empty newline. In this commit, we will call `cliInitHelp` to init the help. Note that in this case, we need to call `cliInitHelp` (COMMAND DOCS) every time, which i think is acceptable. So now the output will look like: ``` [redis]# src/redis-cli help get GET key summary: Get the value of a key since: 1.0.0 group: string [redis]# ``` Fixes redis#10378

oranagra

@enjoy-binbin forgive me for the trivial questions, I'm AFK and not too familiar with this code.
I have two concerns:

I don't like to call COMMAND DOCS when someone does a plain redis-cli set X y or any other usage of the non interactive mode, except help. I suppose this PR is ok in that respect.
Please checj what happens when this is used against an old redis version (without support for DOCS)

enjoy-binbin · 2022-03-04T10:33:17Z

@oranagra sure, no problem. I also did not describe it very clearly, my bad

I don't like to call COMMAND DOCS when someone does a plain redis-cli set X y or any other usage of the non interactive mode, except help. I suppose this PR is ok in that respect.

yes, i also dont like it. so i did check the code, we only call COMMAND DOCS when using help xxx

static int cliSendCommand(int argc, char **argv, long repeat) {
    char *command = argv[0];
    size_t *argvlen;
    int j, output_raw;

    if (!config.eval_ldb && /* In debugging mode, let's pass "help" to Redis. */
        (!strcasecmp(command,"help") || !strcasecmp(command,"?"))) {
        cliOutputHelp(--argc, ++argv);  ------------------->>> here
        return REDIS_OK;
    }

Please checj what happens when this is used against an old redis version (without support for DOCS)

In #10043, if COMMAND DOCS is not suppoert, we will generate help from help.h

[root@binblog redis]# src/redis-cli info server
# Server
redis_version:4.0.14
redis_git_sha1:ff6db5f1
...

[root@binblog redis]# src/redis-cli help lmpop

  LMPOP numkeys key [key ...] LEFT|RIGHT [COUNT count]
  summary: Pop elements from a list
  since: 7.0.0
  group: list

[root@binblog redis]# src/redis-cli help @string

  APPEND key value
  summary: Append a value to a key
  since: 2.0.0
  ....

oranagra · 2022-03-06T07:11:27Z

what about a case where redis isn't running at all?
i.e. if we can print help that's not at all coming from redis, and just use redis-cli as help, maybe we should let it do that when redis is unreachable?

p.s. as far as i could tell this whole feature (help from non-interactive) is not something that was recently broken.
so if it complicate things, we can make compromises.

enjoy-binbin · 2022-03-06T07:55:29Z

interesting idea, i made a quick commit, with this commit, we can do this:

[root@binblog redis]# src/redis-cli
Could not connect to Redis at 127.0.0.1:6379: Connection refused
not connected> help set

  SET key value [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL] [NX|XX] [GET]
  summary: Set the string value of a key
  since: 1.0.0
  group: string

not connected>
[root@binblog redis]# src/redis-cli help lmpop

  LMPOP numkeys key [key ...] LEFT|RIGHT [COUNT count]
  summary: Pop elements from a list
  since: 7.0.0
  group: list

[root@binblog redis]#

Note that in this case, help won't be able to sense the connection status like (unless we do a cliConnect every time in cliOutputHelp):

[root@binblog redis]# src/redis-cli
Could not connect to Redis at 127.0.0.1:6379: Connection refused
not connected> help set

  SET key value [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT 

-------------------------- start the server --------------
not connected> help set

  SET key value [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT 

not connected> ping                ----------->> trigger a reconnect
PONG
127.0.0.1:6379> help set

  SET key value [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT 

------------------------- stop the server ---------------
127.0.0.1:6379> help set

  SET key value [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT

127.0.0.1:6379> ping
Error: Server closed the connection                -------- the new commit fix the prompt see below---------
127.0.0.1:6379> help set

  SET key value [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT

before: ``` 127.0.0.1:6379> ping Error: Server closed the connection 127.0.0.1:6379> ``` after: ``` 127.0.0.1:6379> ping Error: Server closed the connection not connected> ```

oranagra

LGTM.. minor change request.

src/redis-cli.c

``` [root]# src/redis-cli --ldb --eval test.lua test 1 Lua debugging session started, please use: quit -- End the session. restart -- Restart the script in debug mode again. help -- Show Lua script debugging commands. * Stopped at 1, stop reason = step over -> 1 local num = redis.call('GET', KEYS[1]); redis-cli: redis-cli.c:718: cliCountCommands: Assertion `commandTable->element[i]->type == 1' failed. Aborted ``` Because in ldb mode, `COMMAND DOCS` or `COMMAND` will return an array, only with one element, and the type is REDIS_REPLY_STATUS, the result is `<error> Unknown Redis Lua debugger command or wrong number of arguments`. So if we in the ldb mode, and init the Redis HELP, we will get the wrong response and crash the redis-cli

oranagra · 2022-03-10T16:20:11Z

merged. thank you!

Use exit code 1 if redis-cli fails to connect. Before #10382, on a connection failure, exit code would be 1. After this PR, whether connection is established or not, `noninteractive()` return value is used as the exit code. On a failure, this function returns `REDIS_ERR` which is `-1`. It becomes `255` as exit codes are between `0-255`. There is nothing wrong by returning 1 or 255 on failure as far as I know but it'll break things that expect to see 1 as exit code on a connection failure. This is also how we realized the issue. With this PR, changing behavior back to using 1 as exit code to preserve backward compatibility.

oranagra reviewed Mar 4, 2022

View reviewed changes

enjoy-binbin added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Mar 6, 2022

generate help from help.h when redis is unreachable

34ff50e

make the prompt more clearer

3d0b7f8

before: ``` 127.0.0.1:6379> ping Error: Server closed the connection 127.0.0.1:6379> ``` after: ``` 127.0.0.1:6379> ping Error: Server closed the connection not connected> ```

oranagra approved these changes Mar 9, 2022

View reviewed changes

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

src/redis-cli.c Show resolved Hide resolved

enjoy-binbin added 2 commits March 10, 2022 00:08

Merge remote-tracking branch 'upstream/unstable' into cli_init_help

c097bb9

move help logic outside the while loop

f48c0ed

enjoy-binbin commented Mar 9, 2022

View reviewed changes

src/redis-cli.c Show resolved Hide resolved

revert the flag changes, no need to Force, keep it as it is

c0ceb79

oranagra reviewed Mar 9, 2022

View reviewed changes

src/redis-cli.c Show resolved Hide resolved

oranagra approved these changes Mar 10, 2022

View reviewed changes

oranagra merged commit 1797330 into redis:unstable Mar 10, 2022

enjoy-binbin deleted the cli_init_help branch March 10, 2022 16:23

tezc mentioned this pull request Mar 17, 2022

Use exit code 1 if redis-cli fails to connect #10438

Merged

oranagra mentioned this pull request Apr 5, 2022

Release 7.0 rc3 #10532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Initialize help when using redis-cli help or redis-cli ? #10382

Initialize help when using redis-cli help or redis-cli ? #10382

Uh oh!

enjoy-binbin commented Mar 4, 2022 •

edited

Loading

Uh oh!

oranagra left a comment

Uh oh!

enjoy-binbin commented Mar 4, 2022 •

edited

Loading

Uh oh!

oranagra commented Mar 6, 2022

Uh oh!

enjoy-binbin commented Mar 6, 2022 •

edited

Loading

Uh oh!

oranagra left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Initialize help when using redis-cli help or redis-cli ? #10382

Initialize help when using redis-cli help or redis-cli ? #10382

Uh oh!

Conversation

enjoy-binbin commented Mar 4, 2022 • 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!

enjoy-binbin commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Mar 6, 2022

Uh oh!

enjoy-binbin commented Mar 6, 2022 • 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!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

enjoy-binbin commented Mar 4, 2022 •

edited

Loading

enjoy-binbin commented Mar 4, 2022 •

edited

Loading

enjoy-binbin commented Mar 6, 2022 •

edited

Loading