dotnet / dotnet/SqlClient

SqlConnectionInternal starts a fresh ConnectTimeout for in-open round trips instead of honoring the caller's remaining timeout

Open
#4,582 2 comments 0 reactions 0 assignees View on GitHub
:new: Triage Needed Area\Engineering
Dominant language
C#
Stars
989
Forks
340
Avg merge
4d 18h
Merged PRs (30d)
69

Description

## Summary

Several call sites in `SqlConnectionInternal` issue a TDS round trip during connection open using a **fresh** `ConnectionOptions.ConnectTimeout`, rather than the `TimeoutTimer` already counting down for the in-flight `Open()`. Time already spent (pool wait, login) therefore does not count against the caller's budget, so a connection string with `Connect Timeout=15` can block substantially longer than 15 seconds.

Raised by @mdaigle during review of #4335: https://github.com/dotnet/SqlClient/pull/4335#discussion_r3834481934

## Affected call sites

All in `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs`:

| Line | Call |
|---|---|
| 757 | `ChangeDatabase` |
| 2128 | `GetDTCAddress` |
| 2154 | `PropagateTransactionCookie` |
| 2419 | `ReassertSessionIsolationLevel` (added by #4335) |
| 2795 | Transaction manager request |

Each passes `ConnectionOptions.ConnectTimeout`. Downstream, `TdsParser.TdsExecuteSQLBatch` calls `TdsParserStateObject.SetTimeoutSeconds`, which restarts `_timeoutTime` from the current time.

## Example

With `Connect Timeout=15`:

- 10s waiting for a pooled connection
- 4s login
- the in-open batch then starts a **new** 15s budget

Worst case is roughly 29s against a documented 15s limit.

## Why this is not a one-line fix

`Activate(Transaction)` is an abstract override on `DbConnectionInternal` with no timeout parameter. It is reached through `DbConnectionInternal.ActivateConnection(Transaction)` from both `WaitHandleDbConnectionPool` and `ChannelDbConnectionPool`. Threading a `TimeoutTimer` down requires changing the base signature and both pool implementations.

## Existing precedent

`SqlConnectionInternal.ResolveLoginTimeout` already models exactly this choice for the login phase:

```csharp
internal static TimeoutTimer ResolveLoginTimeout(TimeoutTimer callerTimeout, int connectTimeoutSeconds)
=> LocalAppContextSwitches.UseOverallConnectTimeoutForPoolWait
? callerTimeout
: TimeoutTimer.StartNew(TimeSpan.FromSeconds(connectTimeoutSeconds));
```

`Switch.Microsoft.Data.SqlClient.UseOverallConnectTimeoutForPoolWait` defaults to `false`, i.e. the driver currently restarts the clock by default. Any fix here should decide whether these five call sites follow that switch or change unconditionally.

## Suggested scope

1. Thread the live `TimeoutTimer` through `ActivateConnection` / `Activate` and both pool implementations.
2. Apply the change consistently across all five call sites.
3. Decide switch-gated vs. unconditional, consistent with `UseOverallConnectTimeoutForPoolWait`.

Note: the connection pool implementations are under active rewrite, so this likely wants to sequence after that work.

Contributor guide

Open the contributing guide

Research direction

Start in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs, then trace ActivateConnection and Activate through DbConnectionInternal, WaitHandleDbConnectionPool, and ChannelDbConnectionPool. Check the five listed call sites and the existing ResolveLoginTimeout precedent; done means the live timeout is handled consistently and the switch-gated versus unconditional behavior is decided.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.