missing cleanup of CancellationTokenSource

[SimonCropp]

No description provided.

[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

[Copilot]

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]

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]

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]

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.