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.

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

fix: connection close hang #658

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: connection close hang #658

wants to merge 2 commits into from

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Feb 2, 2024

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=BenchmarkConnectionState-count=10 -benchmem > orig.log
benchstat orig.log fix-reply-off.log
goos: linux
goarch: amd64
pkg: github.com/gomodule/redigo/redis
cpu: 11th Gen Intel(R) Core(TM) i9-11900H @ 2.50GHz
                                    │   orig.log   │          fix-reply-off.log          │
                                    │    sec/op    │   sec/op     vs base                │
ConnectionStateUpdateCorrectCase-16    57.65n ± 1%   65.50n ± 2%  +13.63% (p=0.000 n=10)
ConnectionStateUpdateMixedCase-16     360.55n ± 1%   66.04n ± 1%  -81.68% (p=0.000 n=10)
ConnectionStateUpdateNoMatch-16       168.20n ± 1%   75.83n ± 2%  -54.92% (p=0.000 n=10)
geomean                                151.8n        68.96n       -54.56%

                                    │   orig.log   │            fix-reply-off.log            │
                                    │     B/op     │    B/op     vs base                     │
ConnectionStateUpdateCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
ConnectionStateUpdateMixedCase-16     32.00 ± 0%      0.00 ± 0%  -100.00% (p=0.000 n=10)
ConnectionStateUpdateNoMatch-16       0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
geomean                                          ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

                                    │   orig.log   │            fix-reply-off.log            │
                                    │  allocs/op   │ allocs/op   vs base                     │
ConnectionStateUpdateCorrectCase-16   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
ConnectionStateUpdateMixedCase-16     4.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=10)
ConnectionStateUpdateNoMatch-16       0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=10) ¹
geomean                                          ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

@stevenh stevenh force-pushed the fix/reply-off branch 6 times, most recently from 8df9f74 to 6e0c9b8 Compare February 3, 2024 15:03
@stevenh stevenh marked this pull request as ready for review February 3, 2024 15:45
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
Add support for newer command which impact the state of a client
connection hence need special handling when returning connections to the
pool.

Also add missing type assertions for ConnWithContext on connection types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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