SqlConnectionInternal starts a fresh ConnectTimeout for in-open round trips instead of honoring the caller's remaining timeout
- 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
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