[BUG] CLIENT REPLY OFF in transaction handled incorrectly

[Yury-Fridlyand]

Describe the bug

Server returns malformed response.

127.0.0.1:7000> multi
OK
127.0.0.1:7000(TX)> ping before
QUEUED
127.0.0.1:7000(TX)> client reply off 
QUEUED
127.0.0.1:7000(TX)> ping 1
QUEUED
127.0.0.1:7000(TX)> ping 2
QUEUED
127.0.0.1:7000(TX)> client reply on
QUEUED
127.0.0.1:7000(TX)> ping after
QUEUED
127.0.0.1:7000(TX)> exec

Server replies with RESP array of declared size 6, but it contains only 2 elements: "before" and "after". That is correct to receive 2 elements, but size should be declared as 2.
Wireshark helps to track this.

In case if transaction is "reply off; ; reply on; end", received RESP array is empty, but declared size reflects the number of commands.

See Wireshark screenshot:

To reproduce

See above

Expected behavior

Declared RESP array lenght should be valid (match number of elements in it, regardless of real transaction size)

Additional information

A low priority issue.

CLI tool also fails to process the response. Use Wireshark to see what happens on the wire.

[Yury-Fridlyand]

I'd like to do a fix for that if you can give me some guidelines on which files/classes to start with and how to debug the server.

[enjoy-binbin]

Thanks for the report. The place to look (and start) is execCommand, we need to modify array len based on the client flags. During the looking, i have fixed and verified it locally. Let me know if you need more infomation or need me to take over. (btw, we need to fix this without affecting the performance of multi)

[enjoy-binbin]

ok, the fix may require some understanding and some tricks in the codebase, i don't want to waste the effort and waste your time, i am going to prepare the PR, you can do a review in that and get familiar the the codebase.

[enjoy-binbin]

Ok, this is more troublesome than I thought, like don't know how the client is supposed to read the response correctly.

case 1

client reply off
multi
ping 1
exec
client reply on
ping last

case 2

client reply skip
multi
ping 1
exec
client reply on
ping last

case 4

client reply off
multi
ping 1
client reply on
ping 2
exec

case 5

client reply off
multi
ping 1
client reply on
ping 2
client reply off
ping 3
exec

case 6

client reply off
multi
ping 1
client reply on
ping 2
client reply off
exec

[madolson]

I honestly think we should just disable client reply in a transaction. I can't really think of a valid use case for issuing this command inside of multi. I would just let it be broken for old versions and then we make the "breaking change" in Valkey 9.0.

[enjoy-binbin]

I honestly think we should just disable client reply in a transaction. I can't really think of a valid use case for issuing this command inside of multi. I would just let it be broken for old versions and then we make the "breaking change" in Valkey 9.0.

yean, this make sense to me. so we can reject multi when client is marked with reply off, and reject client reply when client is makred with multi.

[PingXie]

+1 on rejecting client reply in a transaction.

[zuiderkwast]

Counting the thumbs up and comments from TSC memebers, I belive we have a decision, right?

The details can be sorted out when we're reviewing the PR.