Redis [BUG] SUSCRIBE in RESP3 gets no proper reply, only push

Describe the bug

After sending SUBSCRIBE and UNSUBSCRIBE (and P and S variants), the message ["subscribe", "ch1", 1] comes as a push message, not a proper reply. If "SUBSCRIBE ch1 ch2", you get two push messages, but no reply. Clients get out of sync due to this.

To reproduce

$ telnet localhost 6379
Trying ::1...
Connected to localhost.
Escape character is '^]'.
hello 3
%7
$6
server
$5
redis
$7
version
$11
255.255.255
$5
proto
:3
$2
id
:211
$4
mode
$10
standalone
$4
role
$6
master
$7
modules
*0
subscribe ch1 ch2
>3
$9
subscribe
$3
ch1
:1
>3
$9
subscribe
$3
ch2
:2
ping
+PONG

Expected behavior

SUBSCRIBE should get +OK or something like that, so that each command gets a reply. (Push are out-of-band.)

Additional information

This was reported in #7026, which was assumed to be a problem in redis-cli, but it has nothing to do with redis-cli.

I guess we can't fix this now, so perhaps it should instead be clearly documented? RESP3-capable clients need to compensate for this or else they will get out of sync...

Comment From: ranshid

@zuiderkwast thank you for reporting this. So basically you suggest we should change the 'addReplyPushLen' to 'addReplyArrayLen'? I guess this will also need to handle the unsubscribe and pattern cases. I can create a PR to fix that but since this will be a breaking change will that be possible to target 7.2 or only 8.0? @oranagra

Comment From: oranagra

i don't think we want (or can afford) to break this now. also it seems very much intended, so maybe just document this?

disclaimer: i didn't think this through much, and it's not my area of expertise. if clients can't handle that in a proper way (creates complications distinguishing between things), then maybe we have to sort it out.

CC @itamarhaber @guybe7

Comment From: mgravell

I'm going to add another vote for "don't change this"; unfortunate? maybe - but: changing it now is just adding a landmine for library authors and comsumers, breaking things when they innocently connect to a different server.

Perhaps the way to consider this is more conceptual - as a documentation clarification to https://redis.io/docs/manual/pubsub/

"Commands that in RESP2 you would have issued to a pub/sub connection such as SUBSCRIBE respond in RESP3 via "push" responses, not standard request/response responses"

and perhaps with a RESP3 request/response example where relevant, rather than just RESP2

(obviously after checking whether this is indeed the case for all of [P|S][UN]SUBSCRIBE and or [S]PUBLISH, such that the documentation reflects the current reality)

This does have the side-effect that clients need to special-case [P|S][UN]SUBSCRIBE and/or [S]PUBLISH (plus renamed) to avoid de-sync, so I can see the logic behind wanting to change it, but... it just feels too late. If anything, I'd say you'd have to rev this with RESP4 rather than the server version, because this is (mostly) a protocol detail. And honestly at that point: since folks would need to write clients/code compatible with RESP3 too, this is just making work and complexity for purism's sake, so again: IMO just leave it.

Comment From: itamarhaber

I agree - this isn't how proper commands should behave, so an "+OK" can definitely make this better. It is a breaking change though, so >= 8.0. I assume the OP stumbled on this as the maintainer of Erlang clients, but I'd also like to hear from other client authors (thanks for jumping in @mgravell) CC @mp911de @redis/client-developers

Comment From: guybe7

@zuiderkwast can you please explain what you mean by "Clients get out of sync due to this"? AFAIU, in RESP2, once a client issues a SUBSCRIBE (or the other variants) it enters a special mode where you can only handle pubsub commands and PING in RESP3 there are no limits, push notifications can arrive at any time, so what's the difference between a message that came via push from the SUBSCRIBE command itself, or from another client doing PUBLISH

if anything, what's weird is that UNSUBSCRIBE also replies with push. the problem here is that the client is considered CLIENT_PUBSUB while it is subscribed to at least one channel. that means that if I execute UNSUBSCRIBE with no args (or the UNSUBSCRIBE that causes the client not to be subscribed to anything anymore) i get a push reply while not in pusub mode.

so yes, it does look weird, but does it cause an actual issue with clients?

Comment From: mp911de

There are many variants to argue for one or the other. I'm for leaving things as-is. Receiving the confirmation as Pub/Sub push seems a better design because the subscription confirmation arrives once Redis is ready with subscribing.

Imagine that Redis could require (somewhere in the future) more work than adding subscribers to a subscription table (i. e. doing actual I/O). Once the subscription process is done, Redis returns and confirms the channel/pattern subscription instead of immediately confirming the command.

I understand the stance of

Clients get out of sync due to this

as an improper client-side expectation might not be met. However, instead of changing every client available, I suggest fixing the one client that can't handle subscribe commands.

Comment From: ranshid

I do not think this is a resp3 protocol intended issue but simply a bug. For example take the issue reported in #2967. we allow resp3 to issue different commands while in subscribed mode since clients are able to set different handler on the pushed commands, but it makes no sense that command reply will get into push handler from client implementation POV. I understand the breaking change for clients, but I support fixing this, even if late as redis 8

Comment From: zuiderkwast

Thanks all for jumping in.

@zuiderkwast thank you for reporting this. So basically you suggest we should change the 'addReplyPushLen' to 'addReplyArrayLen'?

@ranshid No, since one SUBSCRIBE ch1 ch2 produces two push replies (one for each channel), so if we change to an array, we'd send two array responses to one command which would also break the request/response model. Rather an (in-band) +OK response to the subscribe command.

@zuiderkwast can you please explain what you mean by "Clients get out of sync due to this"?

@guybe7 A naive client would offload any push messages, for example to a callback function that handles them or putting them in a separate queue on the side, and then continue the request/response flow, expecting one response for each command sent to Redis. A push message can come any time when some other reply is expected. That's why they're called out-of-band.

When a client sends a [S|P][UN]SUBSCRIBE command, it gets one push reply per channel, which may be more than one for the same command. Besides, there may be other push messages sent by Redis before and after the one corresponding to the [S|P][UN]SUBSCRIBE command just sent, such as a message on another channel, etc. So as @mgravell wrote, clients need to special-case [P|S][UN]SUBSCRIBE. I can think of three ways clients can handle this:

1. Match on the command name. If the command is [S|P][UN]SUBSCRIBE, return 'null' or something without waiting for an (in-band) reply. (Doesn't work that well for renamed commands.)
2. Forbid these commands in the regular command API and instead provide a separate API for them. Let's say redis.call("get", "k1") vs redis.pubsub("subscribe", "ch1", "ch2") where the latter doesn't return anything, a void function. Some callback is used for handling the push messages.
3. Provide a separate function or method for each Redis command. E.g. redis.mget("foo", "bar") or redis.ssubscribe("ch1", "ch2"). Only fancy clients do this. :grin:

I think @mgravell put this very well:

changing it now is just adding a landmine for library authors and comsumers, breaking things when they innocently connect to a different server

and if we do fix it in Redis 8, bumping the resp version to RESP4 could be a sensible idea too.. or we just don't fix it, just document it well.

Comment From: ranshid

No, since one SUBSCRIBE ch1 ch2 produces two push replies (one for each channel), so if we change to an array, we'd send two array responses to one command which would also break the request/response model. Rather an (in-band) +OK response to the subscribe command.

Isn't that the case for multi bulk reply in resp2? we can place an array of arrays for each channel. I still think that having an indication of how many channels this client is subscribed to is something we would like to preserve

Comment From: zuiderkwast

@ranshid In RESP2, for SUBSCRIBE ch1 ch2 you get two multi-bulk replies ["subscribe", "ch1", 1] and ["subscribe", "ch2", 2]. Do you want to wrap these together in one nested multi-multi-bulk reply? Sounds weird to me.

Comment From: ranshid

@ranshid In RESP2, for SUBSCRIBE ch1 ch2 you get two multi-bulk replies ["subscribe", "ch1", 1] and ["subscribe", "ch2", 2]. Do you want to wrap these together in one nested multi-multi-bulk reply? Sounds weird to me.

Not really I only suggested that returning array replies is somewhat the same as in resp2. I think that it if fine to keep things as they are and make sure to document it, but having a separate indication for each channel can be helpful someday (lets say we would like to indicate only the channels the user was able to subscribe to and on which he had errors)

Comment From: zuiderkwast

I think we can conclude: Don't fix it. We can't break existing clients.

I just wish the RESP3 behavior for various commands were documented. This is not for just one client, but for clients in general, to make it as clear as possible how they should work. The only command that actually mentions RESP3 is HELLO afaik.

Another example is MONITOR. It replies with +OK and then it doesn't use push but every monitored command comes as a simple string. This too requires special handling in clients. Example:

$ telnet localhost 6379
Trying ::1...
Connected to localhost.
Escape character is '^]'.
hello 3
%7
(...snip...)
monitor
+OK
ping
+PONG
+1676541668.738627 [0 [::1]:46296] "ping"

Comment From: zuiderkwast

Solved by documentation update.

Comment From: mzimbres

I consider the current behaviour a bug because it makes it impossible to implement async clients without adding some heuristics. SUBSCRIBE has no-response (or has push response) if not well formed. However if I happen to send an ill formed SUBSCRIBE e.g. without argument, Redis will send a simple error response e.g. -error message\r\n. That means, it has no response on success, but has a response on error. I use the following heuristics in Boost.Redis: https://github.com/boostorg/redis/blob/e7ff1cedf347c3c805f72469ce152abe2bbaba20/include/boost/redis/detail/connection_ops.hpp#L359.

Comment From: oranagra

@mzimbres we all agree. but imagine what you'll have to do if we fix it? you'll still need to support older versions, so your code will be considerably more complicated.