send to the redis server a batch of commands, among which there is an invalid resp message:
3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n 3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n 3\r\n$3\r\nset\r\n$5\r\nkey1\r\n$3\r\nval\r\n // a "$5" followed by a four-bytes string "key1" 3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n *3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n
and the server responds this:
+OK +OK -ERR Protocol error: expected '$', got '3'
after that, when I again send valid command to the server, it won't respond correctly. Because the invalid messages remain in the server's input buffer.
So my question is, why didn't the server close the connection when an non-resp message is detected? Or is there a suggested way to deal with such cases?
Thanks!
Comment From: oranagra
@tancehao which version of redis are you using? and how did you interact with it?
this is what i see on the latest:
$ telnet localhost 6379
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
*1
$4
ping
+PONG
*3
$3
set
$5
key1
$3
-ERR Protocol error: expected '$', got '3'
Connection closed by foreign host.
Comment From: tancehao
@oranagra I'm using 4.0.12. I'm implementing a redis proxy, and I need to choose a proper way to handle such cases. I think I have an idea now. Thank you very much!
Comment From: oranagra
@tancehao so is there a bug in Redis 4.0.12 (not closing connection) or not?
Comment From: tancehao
@oranagra yes. I've read the source code, and I noticed that if the function "processMultibulkBuffer" returns a C_ERR, the caller function "processInputBuffer" simply breaks the loop and when it was called next time it starts from the point where it leaves last time and that point is not a correct border between commands because the former command is not a valid one, making all the following commands can't be processed. Since you called it a 'bug', I'll choose the strategy the newest version of redis uses in my proxy.
Comment From: oranagra
@tancehao so can you conform that more recent versions are behaving correctly?
Comment From: tancehao
@oranagra I've tested the version 5.0.9 and 6.0.7, both behave not correctly, that is to say, they didn't closed the connection.
I found the difference between my cases and yours. I send raw bytes to the redis server using scripts,
c, _ := net.Dial("tcp", "127.0.0.1:6379")
cmd := []byte{}
cmd = append(cmd, []byte("*3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n")...)
cmd = append(cmd, []byte("*3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n")...)
cmd = append(cmd, []byte("*3\r\n$3\r\nset\r\n$5\r\nkey1\r\n$3\r\nval\r\n")...)
cmd = append(cmd, []byte("*3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nval\r\n")...)
c.Write(cmd)
buf := make([]byte, 64)
c.Read(buf)
fmt.Println(string(buf))
c.Write([]byte("*3\r\n$3\r\nset\r\n$4\r\nkey1\r\n$3\r\nvam\r\n"))
buf = make([]byte, 64)
c.Read(buf)
fmt.Println(string(buf))
which was thought as multi-bulk, and you use telnet instead which was thought as inline command. I read the source code and found that if the inline command is invalid, the function setProtocolError was called and the client was closed asynchronously, while when the command is multi-bulk, it don't.
Comment From: oranagra
@tancehao both of us were using multi-bulk (RESP) protocol. (i used *3 etc, and when i pressed Enter in telnet it sent \r\n).
as a proof please note that both of us got the response -ERR Protocol error: expected '$', got '3'
the only place in the code that has such a response is processMultibulkBuffer which immediately calls setProtocolError which sets CLIENT_CLOSE_AFTER_REPLY.
https://github.com/redis/redis/blob/573246f73c0d9de6155a7bf5f0cbce98da129afb/src/networking.c#L1707
Comment From: tancehao
@oranagra I rechecked the test and found that I made a mistake. when the server responds with error, I send a command again to the socket and the bytes were wrote successfully, at this point I thought the connection has not been closed yet.But I didn't check the results if I read from that socket, which was an EOF error indeed, so I missed the truth before. Thank you very much! I'll close the connection directly when non-resp data are met.
Comment From: oranagra
ok. closing the issue, feel free to reopen or post if you have further questions.