valkey-io / valkey

A flexible distributed key-value datastore that is optimized for caching and other realtime workloads.
https://valkey.io
Other
17.11k stars 641 forks source link

[BUG] `CLIENT REPLY OFF` in transaction handled incorrectly #1268

Open Yury-Fridlyand opened 2 hours ago

Yury-Fridlyand commented 2 hours ago

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: {5716FB12-9246-4485-ABC3-465C6036E484}

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 commented 2 hours ago

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 commented 41 minutes ago

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.