Skip to content

Conversation

@cyningsun
Copy link
Contributor

@cyningsun cyningsun commented Sep 13, 2025

Description

This PR continues the work from the previously closed PR (based on #3418), which was automatically closed after its base branch was merged.

It implements a context pattern for connection creation to prevent premature cancellation caused by short-lived request timeouts, which previously led to connection pool instability under high concurrency.

Solution

  • Implemented a context approach for connection creation, similar to database/sql and net/http. The dialing operation now uses an independent context with a reasonable DialTimeout, while still listening for cancellation from the original request context for efficient cleanup
  • Queue-based Fairness: Used a FIFO queue to ensure fairness among waiters when sharing a successful connection, preventing starvation.

Changes

  1. Implemented a FIFO queue for connection sharing.
  2. Introduced a dedicated putIdleConn method to handle placement of newly created connections into the pool, bypassing the Put method’s hooks and idle connection checks.
  • Avoids unnecessary hook executions for newly established connections, improving performance and reducing overhead.
  • Preserves resources by circumventing the MaxIdleConns constraint during initial connection placement, preventing valid connections from being discarded prematurely when p.idleConnsLen.Load() >= p.cfg.MaxIdleConns.
  • Defers enforcement of MaxIdleConns limits to the existing check during subsequent Put operations, rather than immediately rejecting newly created connections. ( Avoids impacting connection acquisition latency by not adding cleanup overhead to popIdle)

References

cyningsun and others added 10 commits September 13, 2025 11:01
- Fix BenchmarkWantConnQueue_Dequeue timeout issue by limiting pre-population
- Use object pooling in BenchmarkWantConnQueue_Enqueue to reduce allocations
- Optimize BenchmarkWantConnQueue_EnqueueDequeue with reusable wantConn pool
- Prevent GitHub Actions benchmark failures due to excessive memory usage

Before: BenchmarkWantConnQueue_Dequeue ran for 11+ minutes and was killed
After: All benchmarks complete in ~8 seconds with consistent performance
@ndyakov
Copy link
Member

ndyakov commented Sep 13, 2025

@cyningsun thank you for opening this pr, looking forward to continuing the discussion.

ndyakov
ndyakov previously approved these changes Oct 20, 2025
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyningsun Thank you for this contribution. I do think we can include it in the library, I do not see any issues with it at the moment.

@ndyakov ndyakov changed the title Feat/improve success rate of new connections feat(pool): Feat/improve success rate of new connections Oct 22, 2025
@ndyakov ndyakov changed the title feat(pool): Feat/improve success rate of new connections feat(pool): Improve success rate of new connections Oct 22, 2025
ndyakov
ndyakov previously approved these changes Oct 22, 2025
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyningsun left a question regarding the pool balance, the rest looks good.

@cyningsun
Copy link
Contributor Author

@ndyakov My apologies for the delayed response. The PR is now updated and ready for your next review.

Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I am working on something related to the semaphore, let's merge this and I will try to resolve the conflicts on my end.

Copy link
Contributor

@htemelski-redis htemelski-redis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too

@ndyakov ndyakov merged commit ae5434c into redis:master Oct 30, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Align context handling for connection creation with database/sql and net/http

3 participants