Redis [BUG] Should SUBSCRIBE/UNSUBSCRIBE be rejected in MULTI?

Hi @MeirShpilraien, @oranagra,

I noticed that SUBSCRIBE inside MULTI is allowed for maintaining backward compatibility (https://github.com/redis/redis/pull/8025). However, it seems to produce responses that a client can't handle, for example, saying that a client issues the following commands:

MULTI
SUBSCRIBE c1 c2 c3
SET k v
GET k
EXEC

Then Redis will respond with:

+ OK
+ QUEUED
+ QUEUED
+ QUEUED
*
  > subscribe c1
  > subscribe c2
  > subscribe c3
+ OK                // leaked response for SET k v
$ v                 // leaked response for GET k

Is doing SUBSCRIBE/UNSUBSCRIBE inside MULTI really a valid use case?

Comment From: oranagra

oran@Oran-laptop:~/work/redis$ telnet localhost 6379
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
MULTI
+OK
SUBSCRIBE c1 c2 c3
+QUEUED
SET k v
+QUEUED
GET k
+QUEUED
EXEC
*3
*3
$9
subscribe
$2
c1
:1
*3
$9
subscribe
$2
c2
:2
*3
$9
subscribe
$2
c3
:3
+OK
$1
v

so EXEC thinks the subscribe adds just one reply, and the resulting array is of size 3, but then SUBSCRIBE emits a "reply" per channel (3 in this case), breaking the protocol.

the SUBSCRIBE reply is really bad IMHO, but i wonder what we can do now... other than somehow just fix the EXEC response to count them correctly.

Comment From: rueian

I propose just rejecting SUBSCRIBE/UNSUBSCRIBE/PSUBSCRIBE/PUNSUBSCRIBE/SSUBSCRIBE/SUNSUBSCRIBE commands if c->flags & CLIENT_MULTI, since they break the protocol and maybe there is no backward compatibility need to be maintained?

Comment From: enjoy-binbin

i do remember this one #9928

Comment From: rueian

The proposal in #9928 is to postpone all push messages, which looks good to me generally.

However, I think that the size of the EXEC array response should still be maintained. That means we may need to let SUBSCRIBE to respond with a +OK first in MULTI mode.

Comment From: oranagra

we discussed that in a core-team meeting. one option was to fix EXEC in some way to count these, and the other was to completely block SUBSCRIBE in multi. we would like to proceed with blocking, but we'll only be able to do that in 8.0 (since it's a breaking change). in the meanwhile, since this is not a new problem, there's no reason to rush with the other fix.