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

[cyningsun]

Description

In high-concurrency scenarios where the connection pool is exhausted, the go-redis library currently uses the incoming request's context (reqCtx) for the entire connection creation process (dialing, TLS handshake, AUTH, etc.) in ConnPool.Get. This creates a critical issue that is at odds with the established best practices implemented in the Go standard library.

If the request times out while the connection is being established, the dialing operation is canceled. This often happens even if the connection is milliseconds away from completion, leading to several detrimental effects that are exacerbated by certain common conditions:

Three key factors compound this problem:

1. PoolTimeout Compression: The configured PoolTimeout period is consumed while waiting for an available connection. By the time a new connection creation is initiated, a significant portion of the request's original timeout budget may already be exhausted, leaving an insufficient time window for the dialing operation to complete successfully. This makes connection establishment far more likely to fail.
2. Asymmetric Timeout Configurations: For latency-sensitive services, the timeout on the request context (dictated by upstream service level objectives) is often significantly shorter than a reasonable DialTimeout (which must account for network latency and TCP/TLS handshakes). This mismatch means the request is likely to time out well before the underlying dial operation has had a fair chance to succeed.
3. Propagated Context Timeout Decay: In distributed systems, the timeout on a request's context is typically set by the initial upstream client and decays as it propagates through the service chain. By the time it reaches a downstream service like Redis, the remaining time is frequently very short—often just milliseconds. This leaves virtually no time for a new connection to be established from scratch, making failures almost certain under pool contention.

These factors lead directly to the following negative outcomes:

1. Unnecessary connection establishment failures: Perfectly good connections are aborted due to brief client-side timeouts.
2. Connection pool starvation: A storm of timed-out requests can prevent the pool from ever being refilled, causing a cascade of failures for subsequent requests.
3. Diagnostic ambiguity and operational overhead: It becomes extremely difficult to distinguish between genuine server-side/network connectivity issues and client-side timeout pressure. Observing a high rate of connection timeouts could indicate a problem with the Redis server, or it could simply be a symptom of the client's own aggressive timeouts cancelling viable connections. This ambiguity makes alerting, debugging, and root cause analysis significantly more complex and time-consuming.
4. Resource waste: Server-side resources (TCP slots, CPU for TLS handshakes) are used without any benefit.

Prior Art in Go Stdlib and the Robust Solution

This is a well-understood problem in connection pool design. The Go standard library's database/sql and net/http packages have already implemented robust solutions to prevent request timeouts from destabilizing the connection pool.

1. database/sql.DB
The sql.DB pool decouples connection creation from request lifecycles. It uses a long-lived, dedicated goroutine (connectionOpener) to serialize connection creation. Crucially, it uses a background context for the connection establishment itself.

• Source Code Reference:
  • The connectionOpener goroutine: sql.OpenDB
  • The openNewConnection method which uses the opener's context: (*DB).openNewConnection
  • The mysql/driver use Connect method which creates new context under opener's context using connection Timeout

2. net/http.Transport
The implementation of http.Transport is almost the same. A dialing operation has its own timeout (net.Dialer.Timeout), but also listens for cancellation from the request context to avoid waste.

• Source Code Reference:
  • The getConn method creating a new dial context: (*Transport).getConn
  • The canonical example of context propagation: Look for the select statement with <-ctx.Done() and dialCancel() inside getConn.
  • The dialConn method using a context which is detached from the request context's cancellation signal: (*Transport).dialConnFor

Expected Behavior

The library should adopt the same context strategy used by the standard library. The creation of a new connection, which is a shared resource, should be resilient to the cancellation of the individual request that triggered it.

The expected behavior is:

• Connection creation should primarily be governed by a independent, reasonable timeout (e.g., the configured DialTimeout).
• It should also listen for cancellation from the original request's context for efficient resource cleanup.
• Most importantly, if the connection establishment succeeds after the original request has canceled, that healthy connection should still be placed into the pool for use by other requests.

Willingness to Contribute

The solution involves modifying the connection establishment logic (in internal/pool) to implement this pattern. The following is the implementation of imitating the standard library:

type wantConn struct {
	mu        sync.Mutex      // protects ctx, done and sending of the result
	ctx       context.Context // context for dial, cleared after delivered or canceled
	cancelCtx context.CancelFunc
	done      bool                // true after delivered or canceled
	result    chan wantConnResult // channel to deliver connection or error
}

func (w *wantConn) tryDeliver(cn *Conn, err error) bool {
	w.mu.Lock()
	defer w.mu.Unlock()
	if w.done {
		return false
	}

	w.done = true
	w.ctx = nil

	w.result <- wantConnResult{cn: cn, err: err}
	close(w.result)

	return true
}

func (w *wantConn) cancel(ctx context.Context, p *ConnPool) {
	w.mu.Lock()
	var cn *Conn
	if w.done {
		select {
		case result := <-w.result:
			cn = result.cn
		default:
		}
	} else {
		close(w.result)
	}

	w.done = true
	w.ctx = nil
	w.mu.Unlock()

	if cn != nil {
		p.Put(ctx, cn)
	}
}

type wantConnResult struct {
	cn  *Conn
	err error
}

func (p *ConnPool) asyncNewConn(ctx context.Context) (*Conn, error) {
	// First try to acquire permission to create a connection
	select {
	case p.dialsInProgress <- struct{}{}:
		// Got permission, proceed to create connection
	case <-ctx.Done():
		p.freeTurn()
		return nil, ctx.Err()
	}

	dialCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), p.cfg.DialTimeout)

	w := &wantConn{
		ctx:       dialCtx,
		cancelCtx: cancel,
		result:    make(chan wantConnResult, 1),
	}
	var err error
	defer func() {
		if err != nil {
			w.cancel(ctx, p)
		}
	}()

	go func(w *wantConn) {
		defer w.cancelCtx()
		defer func() { <-p.dialsInProgress }() // Release connection creation permission

		cn, cnErr := p.newConn(w.ctx, true)
		delivered := w.tryDeliver(cn, cnErr)
		if cnErr == nil && delivered {
			return
		} else if cnErr == nil && !delivered {
			p.Put(w.ctx, cn)
		} else { // freeTurn after error
			p.freeTurn()
		}
	}(w)

	select {
	case <-ctx.Done():
		err = ctx.Err()
		return nil, err
	case result := <-w.result:
		err = result.err
		return result.cn, err
	}
}

I am willing to work on a Pull Request to implement this change and would appreciate guidance from the maintainers on the preferred approach.

[ndyakov]

hello @cyningsun and thank you for the suggestion. There is an underlying pool that will be managed similarly to what you described if you configure it with sufficient number of MinIdleConns and sufficient PoolSize. The path to create a new connection per command is not desirable and it is used as a fallback for when there are no available connection in the pool. It can be limited by MaxActiveConns. In practice, if MaxActiveConns == PoolSize as soon as the available connections in the pool are depleted, the pool will return error (without trying to create a new connection). As for the idle, you can check the checkMinIdle which will start idle conns in a background process. I am currently making some changes to the pool, but in general trying to keep the same fallback present, so we can have a way to handle moments when the whole pool can be unhealthy (e.g. node goes down).

Please review the current mechanism and let me know if you still think it should be altered.

[cyningsun]

Hi @ndyakov Thank you for taking the time to explain the pool's configuration and behavior. I really appreciate your detailed response and the work you're putting into improving the pool implementation. Your point about properly configuring MinIdleConns, PoolSize, and MaxActiveConns is absolutely correct - this is indeed the foundation for a stable connection pool and the first step any user should take.

Your explanation helped me better understand an important observation: with the current default of MinIdleConns = 0, many users might unknowingly rely on the fallback path more often than necessary. This suggests that perhaps the defaults could be re-evaluated to better match production needs, where maintaining some minimum number of idle connections is typically essential for stability.

While optimal configuration is crucial and can significantly reduce how often the fallback path is triggered, I'd like to respectfully suggest that it doesn't fully address the core issue with the fallback mechanism itself. The reason is that even with perfect configuration, there are scenarios where the fallback path will still be needed:

1. Transient failures happen: Network blips, server restarts, or brief load spikes that exceed even well-configured pools can still force connections to be recreated dynamically
2. Configuration isn't perfect in practice: Real-world traffic patterns can be unpredictable, and what was a well-sized MinIdleConns yesterday might be inadequate today
3. The fallback path remains vulnerable: When these edge cases occur, the current approach of tying connection creation to request contexts can create a negative feedback loop where timeouts prevent the pool from recovering

This is where I believe the patterns from database/sql and net/http could provide valuable guidance. By separating connection lifecycle management from request handling, they ensure the pool can recover reliably even when under pressure.

Thank you again for your guidance - I've learned a lot from this discussion about proper pool configuration

[ndyakov]

@cyningsun later this week or next week we are going to merge some changes to the pool. Feel free to try your approach and we will review it. Feel free to add some benchmark tests, we will try to take a look at that and we can continue the discussion from there.

[cyningsun]

Here is a real-world example that occurred in our production environment:

1. Initially, the service was operating normally despite maintaining relatively high CPU utilization.(Figure 1)
2. Suddenly, an event occurred (potentially due to network issues or request timeouts), causing clients to begin massively disconnecting from old connections and establishing new ones.
3. The service's CPU became saturated due to the new connection establishment process, which increased the duration of creating new connections. This led to continuous timeouts in connection establishment, preventing the service from recovering.(Figure 2, 3)
4. The situation persisted until manual intervention was taken to scale out the cluster, after which the cluster gradually resumed normal operation.

Figure 1: CPU became saturated due to the new connections

Figure 2: Long-running connections are kept at a very low water level during overload

Figure 3: Clients are continuously creating new connections during overload