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

Describe the bug I was using redis-cli to communicate with redis-server. First I used UNSUBSCRIBE command to unsubscribe multiple channels (I did not use SUBSCRIBE command to subscribe those channels). And then I tried to use SET or other commands (may be LRANGE) to get some data from the redis-server, but I got the unexpected response.

To reproduce Environment: * redis-server: Redis server v=7.0.3 sha=00000000:0 malloc=jemalloc-5.2.1 bits=64 build=9e1e27bf47eab674 * redis-cli: redis-cli 7.0.3 * OS: Ubuntu 16.04

Steps to reproduce the behavior and/or a minimal code sample.

Expected behavior I think the GET a command should return 100 right after I use UNSUBSCRIBE ch1 ch2, rather than the response from UNSUBSCRIBE command.

Comment From: enjoy-binbin

thanks for reporting, indeed a bug (redis-cli bug), i tested in unstable branch

Comment From: oranagra

it seems like the protocol is broken by the UNSUBSCRIBE command (returning multiple RESP replies). and it seems to have existed in previous versions as well.

Comment From: enjoy-binbin

it seems like the protocol is broken by the UNSUBSCRIBE command (returning multiple RESP replies). and it seems to have existed in previous versions as well.

yes and yes, i suppose we need to read the specified number of replies according to the number of parameters. i am testing it. have a demo fix, didn't test in cluster mode..

It's just that the code's special handling of SUBSCRIBE related commands... i will make a PR and see how it will work

Comment From: oranagra

i'm not a pubsub expert, but all other commands reply with a RESP array when they have multiple replies (even if they got multiple inputs). maybe it's different when the connection is in pubsub mode, but in this case it isn't. maybe it should just reply with an error?

@itamarhaber please keep me honest.

Comment From: itamarhaber

Bringing in clients to gain clarity - @chayim @leibale please opine

Comment From: oranagra

trying to pick this up again, i still think it's a bug in redis (not the CLI). the connection is not in a pubsub mode, so it should reply with a plain error, and not with a pubsub type message.

Comment From: enjoy-binbin

yes, think again now, i agree with oran. the connection is not in pubsub mode, and i guess reject these command is the better way to go

Comment From: leibale

so you are suggesting that

UNSUBSCRIBE channel

will throw an error, but

SUBSCRIBE channel
UNSUBSCRIBE another-channel

will "work"? and what about UNSUBSCRIBE without arguments (which normally unsubscribes from all channels) when the client is not in pubsub mode?

IMO we should leave it as is and fix the bug in redis-cli

Comment From: oranagra

yes, i think an UNSUBSCUBE that arrives to a connection that's in pubsub mode should speak the "pubsub protocol", but it if arrives on a connection that's not in pubsub mode it should speak a "normal" redis protocol.

same about UNSUBSCRIBE without argument, if it arrives on a connection that's not in pubsub mode it should respond with a "normal" redis protocol response. however, it seems that in this case we don't have a problem since the subscribed channel list is expected to be empty, and redis will respond with a single array response.

I don't think the bug is in redis-cli, so we can't fix it there, it's redis that breaks it's own protocol.

@leibale are you aware of any client library that issues an UNSUBSCRIBE on a connection that's not in pubsub mode and will get broken by such a change?

Comment From: enjoy-binbin

the connection is not in a pubsub mode, so it should reply with a plain error, and not with a pubsub type message.

trying to pick this up, it's a breaking change, but it doesn't seem to matter much, maybe we can do it in 7.2? @oranagra WDYT

Comment From: enjoy-binbin

It appears to be a known situation, breaking change

    test "UNSUBSCRIBE from non-subscribed channels" {
        set rd1 [redis_deferring_client]
        assert_equal {0 0 0} [unsubscribe $rd1 {foo bar quux}]

        # clean up clients
        $rd1 close
    }

Comment From: oranagra

i no longer remember all the details, i suppose that considering it's a non-common edge case it doesn't worth fixing and breaking something. but on the other hand, if we decide not to fix it in redis, we should fix redis-cli, despite the fact the bug is in redis, and i'm very much uncomfortable with it. maybe somehow we can get a reassurance that this breaking change is very unlikely to be noticed by anyone? @itamarhaber can you help here?

Comment From: hpatro

@oranagra @enjoy-binbin Wouldn't it be fine to introduce a breaking change to reject UNSUBSCRIBE/SUNSUBSCRIBE /PUNSUBSCRIBE command done outside pubsub mode. I think this adds guardrails to avoid issues further in the future.

We currently do the inverse of rejecting most of the commands inside pubsub mode. https://github.com/redis/redis/blob/unstable/src/server.c#L3997-L4011

Comment From: oranagra

it makes sense to me, but the fact there's an explicit test for this behavior scares me. maybe with other commands, clients expect getting an error (i.e. it's a "valid" response), but for unsubscribe they expect getting only the standard unsubscribe error. hoping someone will be able to shed more light.

in any case, we should be aiming to avoid introducing breaking changes in semver minor versions (exceptions are when it's a new interface that is better fixed early before many use it, or a fix that we have to fix in order to proceed with something else, or a security issue). i'll mark this for redis 8.0.

Comment From: zuiderkwast

This is fixed in redis-cli in #11873.