Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
qJkee opened this issue Aug 20, 2018 · 13 comments · May be fixed by #658
Open

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

qJkee opened this issue Aug 20, 2018 · 13 comments · May be fixed by #658
Labels
Bug

Comments

@qJkee
Copy link

qJkee commented Aug 20, 2018

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.
@garyburd
Copy link
Member

garyburd commented Aug 23, 2018 •
edited
Loading

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

@MrSong0607
Copy link

+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
Copy link

image

@MrSong0607
Copy link

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

@bx2
Copy link

bx2 commented Nov 1, 2018

@MrSong0607 can we close this one?
@garyburd ?

@garyburd
Copy link
Member

garyburd commented Nov 1, 2018 •
edited
Loading

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

@iiinsomnia
Copy link

@MrSong0607 Is this problem fixed?

@garyburd garyburd changed the title Pool connections doesn't closing after Close() Pooled connections do not handle CLIENT REPLY ON and block in close Nov 7, 2018
@garyburd garyburd changed the title Pooled connections do not handle CLIENT REPLY ON and block in close Pooled connections do not handle CLIENT REPLY OFF and block in close Nov 7, 2018
@cookiefour
Copy link

the socket are add,not close

@stevenh
Copy link
Collaborator

stevenh commented Jul 21, 2019

Can someone write a test that demonstrates this behaviour please.

@dearchap
Copy link

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

@stevenh
Copy link
Collaborator

stevenh commented Jan 6, 2021

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

@stevenh
Copy link
Collaborator

stevenh commented Jan 29, 2024

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 added a commit that referenced this issue Feb 2, 2024
@stevenh
fix: connection close hang
ad4982a 
Fix connection Close hanging if CLIENT REPLY is set to OFF or SKIP.

This includes a rework of how command actions are handled to use a
single map lookup based on generated permutations to improve performance
and allow it to be used in both activeConn and conn. Replication of this
lookup is need as its impossible to share information between the
different Conn interface implementations due to returning interface
instead of concrete type.

The performance improvements, 33-89% sec/op and eliminating all memory
allocations, help to mitigate the impact of double lookup.

This also expands the benchmark coverage to allow for wider validation
of this change.

Fixes #361

go test -run=^$ -bench=BenchmarkLookupCommand -count=10 -benchmem > orig.log
benchstat orig.log permute.log
goos: linux
goarch: amd64
pkg: github.com/gomodule/redigo/redis
cpu: 11th Gen Intel(R) Core(TM) i9-11900H @ 2.50GHz
                                │   orig.log   │             permute.log             │
                                │    sec/op    │   sec/op     vs base                │
LookupCommandInfoCorrectCase-16    55.89n ± 7%   37.24n ± 2%  -33.37% (p=0.000 n=10)
LookupCommandInfoMixedCase-16     359.85n ± 9%   38.79n ± 2%  -89.22% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       262.45n ± 1%   36.98n ± 4%  -85.91% (p=0.000 n=10)
geomean                            174.1n        37.66n       -78.37%

                                │   orig.log   │               permute.log               │
                                │     B/op     │    B/op     vs base                     │
LookupCommandInfoCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
LookupCommandInfoMixedCase-16     32.00 ± 0%      0.00 ± 0%  -100.00% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       8.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
geomean                                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

                                │   orig.log   │               permute.log               │
                                │  allocs/op   │ allocs/op   vs base                     │
LookupCommandInfoCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
LookupCommandInfoMixedCase-16     4.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       2.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
geomean                                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean
@stevenh stevenh linked a pull request Feb 2, 2024 that will close this issue
Open
stevenh added a commit that referenced this issue Feb 3, 2024
@stevenh
fix: connection close hang
db8722c 
Fix connection Close hanging if CLIENT REPLY is set to OFF or SKIP.

This includes a rework of how command actions are handled to use a
single map lookup based on generated permutations to improve performance
and allow it to be used in both activeConn and conn. Replication of this
lookup is need as its impossible to share information between the
different Conn interface implementations due to returning interface
instead of concrete type.

The performance improvements, 33-89% sec/op and eliminating all memory
allocations, help to mitigate the impact of double lookup.

This also expands the benchmark coverage to allow for wider validation
of this change.

Fixes #361

go test -run=^$ -bench=BenchmarkLookupCommand -count=10 -benchmem > orig.log
benchstat orig.log permute.log
goos: linux
goarch: amd64
pkg: github.com/gomodule/redigo/redis
cpu: 11th Gen Intel(R) Core(TM) i9-11900H @ 2.50GHz
                                │   orig.log   │             permute.log             │
                                │    sec/op    │   sec/op     vs base                │
LookupCommandInfoCorrectCase-16    55.89n ± 7%   37.24n ± 2%  -33.37% (p=0.000 n=10)
LookupCommandInfoMixedCase-16     359.85n ± 9%   38.79n ± 2%  -89.22% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       262.45n ± 1%   36.98n ± 4%  -85.91% (p=0.000 n=10)
geomean                            174.1n        37.66n       -78.37%

                                │   orig.log   │               permute.log               │
                                │     B/op     │    B/op     vs base                     │
LookupCommandInfoCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
LookupCommandInfoMixedCase-16     32.00 ± 0%      0.00 ± 0%  -100.00% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       8.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
geomean                                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

                                │   orig.log   │               permute.log               │
                                │  allocs/op   │ allocs/op   vs base                     │
LookupCommandInfoCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
LookupCommandInfoMixedCase-16     4.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       2.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
geomean                                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean
stevenh added a commit that referenced this issue Feb 3, 2024
@stevenh
fix: connection close hang
6177dc9 
Fix connection Close hanging if CLIENT REPLY is set to OFF or SKIP.

This includes a rework of how command actions are handled to use a
single map lookup based on generated permutations to improve performance
and allow it to be used in both activeConn and conn. Replication of this
lookup is need as its impossible to share information between the
different Conn interface implementations due to returning interface
instead of concrete type.

The performance improvements, 33-89% sec/op and eliminating all memory
allocations, help to mitigate the impact of double lookup.

This also expands the benchmark coverage to allow for wider validation
of this change.

Fixes #361

go test -run=^$ -bench=BenchmarkLookupCommand -count=10 -benchmem > orig.log
benchstat orig.log permute.log
goos: linux
goarch: amd64
pkg: github.com/gomodule/redigo/redis
cpu: 11th Gen Intel(R) Core(TM) i9-11900H @ 2.50GHz
                                │   orig.log   │             permute.log             │
                                │    sec/op    │   sec/op     vs base                │
LookupCommandInfoCorrectCase-16    55.89n ± 7%   37.24n ± 2%  -33.37% (p=0.000 n=10)
LookupCommandInfoMixedCase-16     359.85n ± 9%   38.79n ± 2%  -89.22% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       262.45n ± 1%   36.98n ± 4%  -85.91% (p=0.000 n=10)
geomean                            174.1n        37.66n       -78.37%

                                │   orig.log   │               permute.log               │
                                │     B/op     │    B/op     vs base                     │
LookupCommandInfoCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
LookupCommandInfoMixedCase-16     32.00 ± 0%      0.00 ± 0%  -100.00% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       8.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
geomean                                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

                                │   orig.log   │               permute.log               │
                                │  allocs/op   │ allocs/op   vs base                     │
LookupCommandInfoCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
LookupCommandInfoMixedCase-16     4.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
LookupCommandInfoNoMatch-16       2.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
geomean                                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean
@stevenh
Copy link
Collaborator

stevenh commented Feb 8, 2024

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

@stevenh stevenh added Bug and removed Help wanted labels Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: connection close hang gomodule/redigo
8 participants
@garyburd @bx2 @stevenh @dearchap @iiinsomnia @qJkee @MrSong0607 @cookiefour