Skip to content

missing cleanup of CancellationTokenSource #4009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SimonCropp wants to merge 3 commits into
base: main
Choose a base branch
from
Open

missing cleanup of CancellationTokenSource #4009

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e5e3fa
missing cleanup of CancellationTokenSource
SimonCropp Mar 6, 2026
3b20678
fix cts scope
SimonCropp Mar 6, 2026
c6af540
Safely dispose cancellation tokens; guard SetClosed
SimonCropp Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SsrpClient.netcore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ private static SsrpResult SendUDPRequest(IPAddress[] ipAddresses, int port, byte
if (allIPsInParallel) // Used for MultiSubnetFailover
{
List<Task<SsrpResult>> tasks = new(ipAddresses.Length);
CancellationTokenSource cts = new CancellationTokenSource();
using CancellationTokenSource cts = new CancellationTokenSource();
for (int i = 0; i < ipAddresses.Length; i++)
{
IPEndPoint endPoint = new IPEndPoint(ipAddresses[i], port);
Expand Down
8 changes: 6 additions & 2 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,11 +879,13 @@ private void RunExecuteNonQueryTdsSetupReconnnectContinuation(
{
if (completion.Task.IsCompleted)
{
timeoutCts.Dispose();
return;
}

Interlocked.CompareExchange(ref _reconnectionCompletionSource, null, completion);
timeoutCts.Cancel();
timeoutCts.Dispose();

Task subTask = RunExecuteNonQueryTds(
methodName,
Expand All @@ -903,7 +905,9 @@ private void RunExecuteNonQueryTdsSetupReconnnectContinuation(
state: completion,
onSuccess: static state => ((TaskCompletionSource<object>)state).SetResult(null));
}
});
},
onFailure: _ => timeoutCts.Dispose(),
onCancellation: () => timeoutCts.Dispose());
Comment on lines +909 to +910
}

#endregion
Expand Down
6 changes: 5 additions & 1 deletion src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,11 +1651,13 @@ private void RunExecuteReaderTdsSetupReconnectContinuation(
{
if (completion.Task.IsCompleted)
{
timeoutCts.Dispose();
return;
}

Interlocked.CompareExchange(ref _reconnectionCompletionSource, null, completion);
timeoutCts.Cancel();
timeoutCts.Dispose();

RunExecuteReaderTds(
cmdBehavior,
Expand All @@ -1680,7 +1682,9 @@ private void RunExecuteReaderTdsSetupReconnectContinuation(
state: completion,
onSuccess: static state => ((TaskCompletionSource<object>)state).SetResult(null));
}
});
},
onFailure: _ => timeoutCts.Dispose(),
onCancellation: () => timeoutCts.Dispose());
Comment on lines +1686 to +1687
}

private SqlDataReader RunExecuteReaderTdsWithTransparentParameterEncryption(
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,6 +1419,7 @@
if (cts != null)
{
cts.Cancel();
cts.Dispose();
}
Comment on lines 1418 to 1423
AsyncHelper.WaitForCompletion(reconnectTask, 0, null, rethrowExceptions: false); // we do not need to deal with possible exceptions in reconnection
if (State != ConnectionState.Open)
Expand Down Expand Up @@ -1728,6 +1729,8 @@
}
finally
{
_reconnectionCancellationSource = null;
cts.Dispose();

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context

Check failure on line 1733 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The name 'cts' does not exist in the current context
_recoverySessionData = null;
_suppressStateChangeForReconnection = false;
}
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,7 @@ public override void Close()

// Request that the current task is stopped
_cancelAsyncOnCloseTokenSource.Cancel();
_cancelAsyncOnCloseTokenSource.Dispose();
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Calling Dispose() on _cancelAsyncOnCloseTokenSource here makes Close() unsafe to be called multiple times. Before this change, calling Close() twice was safe because Cancel() on an already-cancelled CTS is a no-op. Now, the second call to Close() will invoke Cancel() (line 883) on an already-disposed CancellationTokenSource, which throws ObjectDisposedException.

This is a realistic scenario: Dispose(bool) calls Close(), and the ADO.NET contract allows both Close() and Dispose() to be called (e.g., reader.Close(); reader.Dispose(); or just using + explicit Close()). Also, CloseReaderFromConnection() calls Close() (line 1114) for the OpenLoggedIn case, which could overlap.

To fix, either add an early return guard (if (_isClosed) return;) before the Cancel()/Dispose() calls, or null-check the source using an interlocked swap pattern (e.g., Interlocked.Exchange(ref _cancelAsyncOnCloseTokenSource, null)?.Cancel()/?.Dispose()).

Suggested change
_cancelAsyncOnCloseTokenSource.Cancel();
_cancelAsyncOnCloseTokenSource.Dispose();
var cancelTokenSource = Interlocked.Exchange(ref _cancelAsyncOnCloseTokenSource, null);
cancelTokenSource?.Cancel();
cancelTokenSource?.Dispose();

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski Apr 14, 2026

Choose a reason for hiding this comment

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

I'll take a closer look here. I suspect we're missing a proper disposal chain somewhere.

var currentTask = _currentTask;
if ((currentTask != null) && (!currentTask.IsCompleted))
{
Expand Down Expand Up @@ -1121,6 +1122,7 @@ virtual internal void CloseReaderFromConnection()
_isClosed = true;
// Request that the current task is stopped
_cancelAsyncOnCloseTokenSource.Cancel();
_cancelAsyncOnCloseTokenSource.Dispose();
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Same issue as in Close(): if Close() was previously called (which disposes _cancelAsyncOnCloseTokenSource), and then CloseReaderFromConnection() is called on the broken-connection path, calling Cancel() on the already-disposed CTS will throw ObjectDisposedException. The comment on line 1119 even notes this must be thread-safe. Consider using the same guard pattern recommended for Close() (e.g., Interlocked.Exchange or null-check after swap).

Suggested change
_cancelAsyncOnCloseTokenSource.Cancel();
_cancelAsyncOnCloseTokenSource.Dispose();
var cancelTokenSource = Interlocked.Exchange(ref _cancelAsyncOnCloseTokenSource, null);
if (cancelTokenSource != null)
{
cancelTokenSource.Cancel();
cancelTokenSource.Dispose();
}

Copilot uses AI. Check for mistakes.
if (stateObj != null)
{
var networkPacketTaskSource = stateObj._networkPacketTaskSource;
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ protected override void Dispose(bool disposing)
{
// Set the stream as closed
SetClosed();
_disposalTokenSource.Dispose();
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Same issue as in SqlSequentialTextReader: adding _disposalTokenSource.Dispose() after SetClosed() makes Dispose(bool) unsafe for double-disposal. On the second call, SetClosed() calls _disposalTokenSource.Cancel() (line 238) on the already-disposed CTS, which throws ObjectDisposedException.

SetClosed() can also be called externally from SqlDataReader.CloseActiveSequentialStreamAndTextReader() before the user calls Dispose(), so this pattern needs a guard to prevent calling Cancel() on a disposed CTS.

Copilot uses AI. Check for mistakes.
}

base.Dispose(disposing);
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialTextReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ protected override void Dispose(bool disposing)
{
// Set the textreader as closed
SetClosed();
_disposalTokenSource.Dispose();
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Adding _disposalTokenSource.Dispose() after SetClosed() makes Dispose(bool) unsafe for double-disposal. On the second call, SetClosed() calls _disposalTokenSource.Cancel() (line 292) which will throw ObjectDisposedException because the CTS was already disposed on the first call.

Additionally, SetClosed() can be called externally from SqlDataReader.CloseActiveSequentialStreamAndTextReader(), followed by an eventual Dispose() call — the Dispose() would work, but a subsequent second Dispose() would still crash.

Consider guarding: either check if already disposed/closed before calling SetClosed(), or swap the order so Dispose() happens before Cancel() is called again (e.g., use a disposed flag, or move the Cancel() call out of SetClosed() into Dispose() with a guard).

Copilot uses AI. Check for mistakes.
}

base.Dispose(disposing);
Expand Down
Loading