redigo icon indicating copy to clipboard operation
redigo copied to clipboard

Pooled connections do not handle CLIENT REPLY OFF and block in close

Open qJkee opened this issue 7 years ago • 13 comments

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.

qJkee avatar Aug 20 '18 08:08 qJkee

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().

garyburd avatar Aug 23 '18 00:08 garyburd

+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

MrSong0607 avatar Aug 29 '18 05:08 MrSong0607

image

MrSong0607 avatar Aug 29 '18 05:08 MrSong0607

@garyburd i checked my code,it's look like a wrong in my defer function position

MrSong0607 avatar Aug 30 '18 03:08 MrSong0607

@MrSong0607 can we close this one? @garyburd ?

bx2 avatar Nov 01 '18 17:11 bx2

No, this should not be closed. The issue is not fixed.

garyburd avatar Nov 01 '18 18:11 garyburd

@MrSong0607 Is this problem fixed?

iiinsomnia avatar Nov 07 '18 08:11 iiinsomnia

the socket are add,not close

cookiefour avatar Apr 12 '19 02:04 cookiefour

Can someone write a test that demonstrates this behaviour please.

stevenh avatar Jul 21 '19 10:07 stevenh

Is this a property of just pooled connection or all connections ?

dearchap avatar Dec 20 '20 01:12 dearchap

I believe this is pooled connections only as its part of activeConn https://github.com/gomodule/redigo/blob/master/redis/pool.go#L477

stevenh avatar Jan 06 '21 11:01 stevenh

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?

stevenh avatar Jan 29 '24 23:01 stevenh

@qJkee better late than never right, wonder if you could confirm if #658 fixes your issue?

stevenh avatar Feb 08 '24 10:02 stevenh