Pooled connections do not handle CLIENT REPLY OFF and block in close #361
Comments
|
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 |
|
|
|
@garyburd i checked my code,it's look like a wrong in my defer function position |
|
@MrSong0607 can we close this one? |
|
No, this should not be closed. The issue is not fixed. |
|
@MrSong0607 Is this problem fixed? |
garyburd
changed the title
garyburd
changed the title
|
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 |
|
I take it back the underlying issue is in A workaround is to set a read timeout but that does break reuse on these connections. The current connection state management is in 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? |
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
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
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
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 inpool.go:417you are waiting for a response, which will never come.The text was updated successfully, but these errors were encountered: