missing cleanup of CancellationTokenSource by SimonCropp · Pull Request #4009 · dotnet/SqlClient

SimonCropp · 2026-03-06T12:21:45Z

Summary

Several CancellationTokenSource (CTS) instances across the driver were created but never disposed, leaking the timer/registration resources they hold. This PR disposes those token sources at the correct point in their lifecycle, and adds the guards needed so that the new disposal does not introduce ObjectDisposedException faults when an object is closed/disposed more than once or concurrently.

Changes

Managed SNI — `SsrpClient.netcore.cs`

Wrap the CancellationTokenSource used for parallel SSRP resolution (MultiSubnetFailover) in a using so it is disposed when resolution completes.

`SqlDataReader.cs`

Close() and CloseReaderFromConnection() now dispose _cancelAsyncOnCloseTokenSource after cancelling it.
The source is taken with Interlocked.Exchange(ref _cancelAsyncOnCloseTokenSource, null) so it is cancelled/disposed exactly once. This keeps repeated Close()/Dispose() calls (and the broken-connection cleanup path) safe — a second close sees null and does nothing instead of calling Cancel() on a disposed source.

`SqlConnection.cs` — reconnect cancellation

ReconnectAsync now owns its CancellationTokenSource: it is created before the try and disposed (and the _reconnectionCancellationSource field cleared) in the finally, once the token is no longer in use.
Close() only cancels the reconnect source and lets ReconnectAsync's finally dispose it, avoiding disposal while the reconnect task is still using the token. The Cancel() call is wrapped in try/catch (ObjectDisposedException) to tolerate the race where ReconnectAsync disposes the source concurrently.

`SqlCommand.NonQuery.cs` / `SqlCommand.Reader.cs` — reconnect timeout

In the reconnect continuation, timeoutCts is now disposed on every path (already-completed, success, failure, and cancellation), not just the success path.
Cancel() is called before Dispose() so the Task.Delay(timeout, timeoutCts.Token) scheduled by SetTimeoutException is released promptly instead of lingering until the timeout elapses.

`SqlSequentialStream.cs` / `SqlSequentialTextReader.cs`

Dispose(bool) now disposes _disposalTokenSource.
SetClosed() is made idempotent (early-return when already closed). SqlDataReader.CloseActiveSequentialStreamAndTextReader() may call SetClosed() after the consumer has already disposed the stream/reader; without the guard the second Cancel() would throw ObjectDisposedException on the now-disposed source. The first close already cancels and waits for any pending task, so the later call is safely a no-op.

Notes

CancellationTokenSource.Dispose() is idempotent (per the IDisposable contract), so repeated Dispose() calls are safe; only Cancel()/Token throw after disposal, which is what the added guards protect against.
All changes are behavior-compatible; no public API changes.

Checklist

Existing behavior preserved (no public API changes)
Guards added so disposal does not introduce ObjectDisposedException on double-close/concurrent paths
Tests added/updated (CancellationTokenSourceDisposalTest — dispose-then-close and double-close paths)

Copilot

Pull request overview

This PR adds missing Dispose() calls on CancellationTokenSource instances across multiple files in the SQL client library. Undisposed CTS objects can cause minor resource leaks (they hold internal kernel event handles on some platforms), so cleaning them up is a good practice.

Changes:

Disposes _disposalTokenSource in SqlSequentialTextReader and SqlSequentialStream during their Dispose(bool) methods
Disposes _cancelAsyncOnCloseTokenSource in SqlDataReader.Close() and CloseReaderFromConnection() after cancelling it
Disposes reconnection CTS in SqlConnection.Close() and ReconnectAsync(), with field nulling for safety
Disposes timeoutCts in all continuation paths (success, failure, cancellation) in SqlCommand.Reader.cs and SqlCommand.NonQuery.cs
Converts local CTS to a using declaration in SsrpClient.netcore.cs

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`SqlSequentialTextReader.cs`	Adds `_disposalTokenSource.Dispose()` in `Dispose(bool)` after `SetClosed()`
`SqlSequentialStream.cs`	Adds `_disposalTokenSource.Dispose()` in `Dispose(bool)` after `SetClosed()`
`SqlDataReader.cs`	Adds `_cancelAsyncOnCloseTokenSource.Dispose()` in `Close()` and `CloseReaderFromConnection()`
`SqlConnection.cs`	Adds CTS disposal in `Close()` and `ReconnectAsync()` with null-field pattern
`SqlCommand.Reader.cs`	Disposes `timeoutCts` in all continuation paths of reconnect setup
`SqlCommand.NonQuery.cs`	Disposes `timeoutCts` in all continuation paths of reconnect setup
`SsrpClient.netcore.cs`	Changes local CTS to a `using` declaration for automatic disposal

github-actions · 2026-04-11T03:00:44Z

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

paulmedynski · 2026-04-14T16:55:05Z

/azp run

azure-pipelines · 2026-04-14T16:55:31Z

Azure Pipelines successfully started running 2 pipeline(s).

Use Interlocked.Exchange to null out and safely dispose the _cancelAsyncOnCloseTokenSource in SqlDataReader, avoiding races and null/dispose issues during concurrent close operations. In SqlSequentialStream and SqlSequentialTextReader, guard calls to SetClosed (check for null reader / IsClosed) before invoking it and still dispose the disposal token source. These changes prevent potential NREs or double-dispose scenarios during teardown.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

mdaigle · 2026-04-16T19:37:35Z

/azp run

azure-pipelines · 2026-04-16T19:38:03Z

Azure Pipelines successfully started running 2 pipeline(s).

…disposal - SqlConnection.Close(): only cancel the reconnect CTS and let ReconnectAsync's finally dispose it, avoiding ObjectDisposedException races with the in-flight reconnect task. - SqlCommand.Reader/NonQuery reconnect continuations: cancel timeoutCts before disposing on the completed/onFailure/onCancellation paths so the Task.Delay timer is released promptly. - SqlSequentialStream/SqlSequentialTextReader.Dispose(): keep the existing IsClosed/_reader guard (which prevents Cancel() on a disposed CTS) and rely on the idempotent CancellationTokenSource.Dispose(); no behavior change, double-dispose remains safe.

paulmedynski · 2026-06-23T16:15:37Z

/azp run sqclient-pr

azure-pipelines · 2026-06-23T16:15:43Z

No pipelines are associated with this pull request.

…onTokenSource-

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialTextReader.cs:290

Disposing _disposalTokenSource in Dispose(bool) makes later internal cleanup (SqlDataReader.CloseActiveSequentialStreamAndTextReader() calling SetClosed()) able to throw ObjectDisposedException if the consumer disposes the SqlSequentialTextReader before the parent SqlDataReader is closed. SetClosed() currently calls _disposalTokenSource.Cancel() unconditionally, which will throw once the CTS has been disposed.

                _disposalTokenSource.Dispose();
            }

            base.Dispose(disposing);
        }

paulmedynski · 2026-06-23T17:41:54Z

/azp run

azure-pipelines · 2026-06-23T17:42:28Z

Azure Pipelines successfully started running 3 pipeline(s).

…posed CTS - SqlSequentialStream/SqlSequentialTextReader.SetClosed(): make idempotent by returning early when already closed. SqlDataReader.CloseActiveSequentialStreamAndTextReader() can call SetClosed() after the consumer has already disposed the stream/reader (which disposes _disposalTokenSource), so the second Cancel() would throw ObjectDisposedException. - SqlConnection.Close(): guard the reconnect-cancellation Cancel() with try/catch (ObjectDisposedException) to tolerate ReconnectAsync's finally disposing the same CancellationTokenSource concurrently.

paulmedynski · 2026-06-23T18:29:44Z

/azp run sqlclient-pr

azure-pipelines · 2026-06-23T18:29:58Z

Azure Pipelines successfully started running 1 pipeline(s).

Adds ManualTests covering the dispose-then-close paths fixed in this PR: - Disposing a sequential Stream/TextReader before the owning SqlDataReader closes (and advancing past a result set) must not throw ObjectDisposedException via SetClosed(). - Double-dispose of the sequential Stream/TextReader is safe. - Repeated SqlDataReader.Close()/Dispose() is safe after the close-cancellation CancellationTokenSource is disposed.

paulmedynski · 2026-06-24T11:10:49Z

/azp run sqlclient-pr

azure-pipelines · 2026-06-24T11:11:05Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Copilot comments addressed.

missing cleanup of CancellationTokenSource

7e5e3fa

SimonCropp requested a review from a team as a code owner March 6, 2026 12:21

Copilot AI review requested due to automatic review settings March 6, 2026 12:21

github-project-automation Bot added this to SqlClient Board Mar 6, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Mar 6, 2026

Copilot started reviewing on behalf of SimonCropp March 6, 2026 12:22 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

fix cts scope

3b20678

paulmedynski added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Mar 9, 2026

paulmedynski modified the milestones: 7.0.1, 7.1.0-preview1 Mar 9, 2026

benrr101 moved this from To triage to In review in SqlClient Board Mar 17, 2026

github-actions Bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Apr 11, 2026

paulmedynski self-assigned this Apr 14, 2026

paulmedynski removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Apr 14, 2026

paulmedynski moved this from In review to Investigating in SqlClient Board Apr 14, 2026

Copilot AI review requested due to automatic review settings April 14, 2026 22:47

Copilot started reviewing on behalf of SimonCropp April 14, 2026 22:48 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

paulmedynski modified the milestones: 7.1.0-preview1, 7.1.0-preview2 Apr 21, 2026

paulmedynski removed the Author attention needed PRs that require author to respond or make updates to PR. label Jun 23, 2026

paulmedynski moved this from In review to In progress in SqlClient Board Jun 23, 2026

Merge branch 'main' into dev/SimonCropp/missing-cleanup-of-Cancellati…

1d0f1ea

…onTokenSource-

Copilot AI review requested due to automatic review settings June 23, 2026 16:25

Copilot started reviewing on behalf of paulmedynski June 23, 2026 16:25 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialStream.cs

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

mdaigle previously approved these changes Jun 23, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 24, 2026 11:09

paulmedynski dismissed mdaigle’s stale review via 116f976 June 24, 2026 11:09

Copilot started reviewing on behalf of paulmedynski June 24, 2026 11:10 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

paulmedynski approved these changes Jun 24, 2026

View reviewed changes

paulmedynski requested review from benrr101 and mdaigle June 24, 2026 12:14

paulmedynski moved this from In progress to In review in SqlClient Board Jun 24, 2026

mdaigle approved these changes Jun 24, 2026

View reviewed changes

paulmedynski merged commit 145b285 into dotnet:main Jun 24, 2026
57 of 58 checks passed

github-project-automation Bot moved this from In review to Done in SqlClient Board Jun 24, 2026

Uh oh!

Conversation

SimonCropp commented Mar 6, 2026 • edited by paulmedynski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Managed SNI — SsrpClient.netcore.cs

SqlDataReader.cs

SqlConnection.cs — reconnect cancellation

SqlCommand.NonQuery.cs / SqlCommand.Reader.cs — reconnect timeout

SqlSequentialStream.cs / SqlSequentialTextReader.cs

Notes

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 11, 2026

Uh oh!

paulmedynski commented Apr 14, 2026

Uh oh!

azure-pipelines Bot commented Apr 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle commented Apr 16, 2026

Uh oh!

azure-pipelines Bot commented Apr 16, 2026

Uh oh!

paulmedynski commented Jun 23, 2026

Uh oh!

azure-pipelines Bot commented Jun 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

paulmedynski commented Jun 23, 2026

Uh oh!

azure-pipelines Bot commented Jun 23, 2026

Uh oh!

paulmedynski commented Jun 23, 2026

Uh oh!

azure-pipelines Bot commented Jun 23, 2026

Uh oh!

paulmedynski commented Jun 24, 2026

Uh oh!

azure-pipelines Bot commented Jun 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SimonCropp commented Mar 6, 2026 •

edited by paulmedynski

Loading

Managed SNI — `SsrpClient.netcore.cs`

`SqlDataReader.cs`

`SqlConnection.cs` — reconnect cancellation

`SqlCommand.NonQuery.cs` / `SqlCommand.Reader.cs` — reconnect timeout

`SqlSequentialStream.cs` / `SqlSequentialTextReader.cs`