Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8870,11 +8870,15 @@ var _ = Describe("Commands", func() {
It("returns latencies", func() {
const key = "latency-monitor-threshold"

// reset all latencies first to ensure clean state
err := client.LatencyReset(ctx).Err()
Expect(err).NotTo(HaveOccurred())

old := client.ConfigGet(ctx, key).Val()
client.ConfigSet(ctx, key, "1")
defer client.ConfigSet(ctx, key, old[key])

err := client.Do(ctx, "DEBUG", "SLEEP", 0.01).Err()
err = client.Do(ctx, "DEBUG", "SLEEP", 0.01).Err()
Expect(err).NotTo(HaveOccurred())

result, err := client.Latency(ctx).Result()
Expand Down Expand Up @@ -8921,6 +8925,10 @@ var _ = Describe("Commands", func() {
It("reset latencies by add event name args", func() {
const key = "latency-monitor-threshold"

// reset all latencies first to ensure clean state
err := client.LatencyReset(ctx).Err()
Expect(err).NotTo(HaveOccurred())

old := client.ConfigGet(ctx, key).Val()
client.ConfigSet(ctx, key, "1")
defer client.ConfigSet(ctx, key, old[key])
Expand Down
6 changes: 6 additions & 0 deletions internal/pool/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,12 @@ func (p *ConnPool) newConn(ctx context.Context, pooled bool) (*Conn, error) {
return nil, ErrPoolExhausted
}

// Protect against nil context due to race condition in queuedNewConn
// where the context can be set to nil after timeout/cancellation
if ctx == nil {
ctx = context.Background()
}

dialCtx, cancel := context.WithTimeout(ctx, p.cfg.DialTimeout)
defer cancel()
cn, err := p.dialConn(dialCtx, pooled)
Expand Down
58 changes: 58 additions & 0 deletions internal/pool/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,64 @@ var _ = Describe("queuedNewConn", func() {
testPool.Put(ctx, reqBConn)
Eventually(func() int { return testPool.QueueLen() }, "600ms").Should(Equal(0))
})
// Test for race condition where nil context can be passed to newConn
// This reproduces the issue reported in GitHub where queuedNewConn panics
// with "cannot create context from nil parent"
It("should handle nil context race condition in queuedNewConn", func() {
// Create a pool with very short timeouts to trigger the race condition
testPool := pool.NewConnPool(&pool.Options{
Dialer: func(ctx context.Context) (net.Conn, error) {
// Add a small delay to increase chance of race condition
time.Sleep(50 * time.Millisecond)
return dummyDialer(ctx)
},
PoolSize: int32(10),
MaxConcurrentDials: 10,
PoolTimeout: 10 * time.Millisecond, // Very short timeout
DialTimeout: 100 * time.Millisecond,
ConnMaxIdleTime: time.Millisecond,
})
defer testPool.Close()

// Try to trigger the race condition by making many concurrent requests
// with short timeouts
const numGoroutines = 50
var wg sync.WaitGroup
errors := make(chan error, numGoroutines)

for i := 0; i < numGoroutines; i++ {
wg.Add(1)
go func() {
defer GinkgoRecover()
defer wg.Done()

// Use a very short context timeout to trigger the race
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond)
defer cancel()

_, err := testPool.Get(ctx)
if err != nil {
// We expect timeout errors, but not panics
errors <- err
}
}()
}

wg.Wait()
close(errors)

// Check that we got timeout errors (expected) but no panics
// The test passes if it doesn't panic
timeoutCount := 0
for err := range errors {
if err == context.DeadlineExceeded || err == pool.ErrPoolTimeout {
timeoutCount++
}
}

// We should have at least some timeouts due to the short timeout
Expect(timeoutCount).To(BeNumerically(">", 0))
})
})

func init() {
Expand Down
53 changes: 53 additions & 0 deletions internal/pool/want_conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,3 +442,56 @@ func BenchmarkWantConnQueue_EnqueueDequeue(b *testing.B) {
q.dequeue()
}
}

// TestWantConn_RaceConditionNilContext tests the race condition where
// getCtxForDial can return nil after the context is cancelled.
// This test verifies that the fix in newConn handles nil context gracefully.
func TestWantConn_RaceConditionNilContext(t *testing.T) {
// This test simulates the race condition described in the issue:
// 1. Main goroutine creates a wantConn with a context
// 2. Background goroutine starts but hasn't called getCtxForDial yet
// 3. Main goroutine times out and calls cancel(), setting w.ctx to nil
// 4. Background goroutine calls getCtxForDial() and gets nil
// 5. Background goroutine calls newConn(nil, true) which should not panic

dialCtx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancel()

w := &wantConn{
ctx: dialCtx,
cancelCtx: cancel,
result: make(chan wantConnResult, 1),
}

// Simulate the race condition by canceling the context
// and then trying to get it
var wg sync.WaitGroup
wg.Add(1)

go func() {
defer wg.Done()
// Small delay to ensure cancel happens first
time.Sleep(10 * time.Millisecond)

// This should return nil after cancel
ctx := w.getCtxForDial()

// Verify that we got nil context
if ctx != nil {
t.Errorf("Expected nil context after cancel, got %v", ctx)
}
}()

// Cancel the context immediately
w.cancel()

wg.Wait()

// Verify the wantConn state
if !w.done {
t.Error("wantConn should be marked as done after cancel")
}
if w.ctx != nil {
t.Error("wantConn.ctx should be nil after cancel")
}
}
Loading