redigo
redigo copied to clipboard
Pooled connections do not handle CLIENT REPLY OFF and block in close
If you turn off redis responses with CLIENT REPLY OFF.
Then, if you use the pool, and try to call Close() method from the connection, it will never close, because in pool.go:417 you are waiting for a response, which will never come.
Thank you for reporting.
Pooled connections should detect the use of the CLIENT REPLY OFF command and execute the CLIENT REPLY ON command in Close().
+1 root@iZbp1334v38jb1mcjpg572Z:~# netstat -n|grep 6379|wc -l 1365 i have the same situiation too, my redis config,max active is 500,but there have 1365 ESTABLISHED connection

@garyburd i checked my code,it's look like a wrong in my defer function position
@MrSong0607 can we close this one? @garyburd ?
No, this should not be closed. The issue is not fixed.
@MrSong0607 Is this problem fixed?
the socket are add,not close
Can someone write a test that demonstrates this behaviour please.
Is this a property of just pooled connection or all connections ?
I believe this is pooled connections only as its part of activeConn https://github.com/gomodule/redigo/blob/master/redis/pool.go#L477
I take it back the underlying issue is in conn, which counts pending responses but doesn't know about CLIENT REPLY so is expecting responses which will never come when processing the Do("").
A workaround is to set a read timeout but that does break reuse on these connections.
The current connection state management is in activeConn which can be extended to handle CLIENT REPLY but it has no way to communicate this with conn. The correct way would arguably be to push the state management down to conn which will also allow non pooled connections to understand multi, watch, subscriptions and client reply however because Dial returns an interface instead of struct wrappers embed the interface Conn which would prevent activeConn from using type conversion to access new state methods, as demonstrated by this playground example.
With all this I don't see a backwards compatible way to fix this edge case without introducing a performance penalty.
Does anyone have any ideas?
@qJkee better late than never right, wonder if you could confirm if #658 fixes your issue?