Backport 18967 to v21 branch connection pool bugfix #201
Conversation
…eopen (vitessio#18967) (vitessio#18970) Signed-off-by: Arthur Schreiber <arthur@planetscale.com> Co-authored-by: Arthur Schreiber <arthurschreiber@github.com>
There was a problem hiding this comment.
Pull request overview
This PR backports a connection pool bugfix (issue vitessio#18967) to the v21 branch that addresses a connection leak issue when idle timeouts occur during connection reopening.
- Fixed a race condition in the
pop()method where connections being closed by the idle worker were not properly skipped - Refactored
closeIdleResources()to usegetNew()instead ofconnReopen()to avoid leaking connections when reopening after idle timeout - Added comprehensive test coverage for the idle timeout connection leak scenario
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| go/pools/smartconnpool/pool.go | Fixed pop() logic to properly skip connections that can't be borrowed and refactored closeIdleResources() to prevent connection leaks during idle timeout handling |
| go/pools/smartconnpool/pool_test.go | Added new test TestIdleTimeoutConnectionLeak to verify the bugfix prevents connection leaks when idle timeout occurs during connection reopening |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LogWait: state.LogWait, | ||
| }).Open(newConnector(&state), nil) | ||
|
|
||
| getCtx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) |
There was a problem hiding this comment.
The method t.Context() was introduced in Go 1.24, but this project uses Go 1.23.10 (as specified in go.mod). This will cause a compilation error. Use context.Background() instead, which is the pattern used consistently throughout this test file.
| getCtx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) | |
| getCtx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) |
| // Try to get connections while they're being reopened | ||
| // This should trigger the bug where connections get discarded | ||
| for i := 0; i < 2; i++ { | ||
| getCtx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond) |
There was a problem hiding this comment.
The method t.Context() was introduced in Go 1.24, but this project uses Go 1.23.10 (as specified in go.mod). This will cause a compilation error. Use context.Background() instead, which is the pattern used consistently throughout this test file.
| assert.Equal(t, int64(2), p.Metrics.IdleClosed()) | ||
|
|
||
| // Try to close the pool - if there are leaked connections, this will timeout | ||
| closeCtx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) |
There was a problem hiding this comment.
The method t.Context() was introduced in Go 1.24, but this project uses Go 1.23.10 (as specified in go.mod). This will cause a compilation error. Use context.Background() instead, which is the pattern used consistently throughout this test file.
| require.Equal(c, int64(2), state.open.Load()) | ||
| // Check the total number of closed connections | ||
| require.Equal(c, int64(2), state.close.Load()) |
There was a problem hiding this comment.
Inside require.EventuallyWithT, you should use assert.Equal instead of require.Equal. The require functions will cause the goroutine to exit immediately on failure, which prevents EventuallyWithT from retrying. This is inconsistent with lines 1318 and 1320 which correctly use assert.Equal.
| require.Equal(c, int64(2), state.open.Load()) | |
| // Check the total number of closed connections | |
| require.Equal(c, int64(2), state.close.Load()) | |
| assert.Equal(c, int64(2), state.open.Load()) | |
| // Check the total number of closed connections | |
| assert.Equal(c, int64(2), state.close.Load()) |
For https://github.com/github/vitess/issues/1549
upstream PR vitessio#18967
This backport is pulled from the v22 backport, since its the closest version vitess upstream backport process added it to (as v21 is out of support officially now)
Description
Related Issue(s)
Checklist
Deployment Notes