Flatten | SqlInternalConnectionTds and SqlInternalConnection #3773

benrr101 · 2025-11-14T20:20:50Z

Description

This PR is part of an ongoing (undocumented) effort to clean up, and modernize the codebase. In this installment, we have the SqlInternalConnection and SqlInternalConnectionTds class. These are a remnant of when context connections existed - two classes inherited from SqlInternalConnection: SqlInternalConnectionTds and SqlInternalConnectionSmi (I think). The SMI version was removed a while ago as part of removing functionally dead codepaths. As such, there's no need for all this inheritance. We can merge SqlInternalConnectionTds into SqlInternalConnection.

At the same time, we also move SqlInternalConnectionTds into the Connection namespace, and rename it to SqlConnectionInternal to alphabetically organize it with SqlConnection.

Comments have been added to the PR to highlight key changes in the code.

Issues

N/A

Testing

Code continues to compile, CI will validate.

Copilot

Pull Request Overview

This PR refactors the internal connection class hierarchy by merging SqlInternalConnectionTds into SqlInternalConnection and renaming the merged class to SqlConnectionInternal. The class has been moved to the Microsoft.Data.SqlClient.Connection namespace for better organization. Additionally, a new CachedContexts class has been introduced to centralize the management of cached asynchronous call contexts.

Key changes:

Elimination of unnecessary inheritance between SqlInternalConnection and SqlInternalConnectionTds
Renaming to SqlConnectionInternal for alphabetical alignment with SqlConnection
Introduction of CachedContexts helper class to manage async call context caching
Updates to all references across the codebase to use the new type name and namespace

Reviewed Changes

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

Show a summary per file

File	Description
SqlConnectionInternal.cs	New merged class combining SqlInternalConnection and SqlInternalConnectionTds
CachedContexts.cs	New helper class for managing cached async contexts
SqlTransaction.cs	Updated type references and added namespace import
SqlInternalTransaction.cs	Updated type references and added namespace import
TdsParser.cs	Updated type references throughout
SqlUtil.cs	Updated method signatures to use new type name
SqlException.cs	Updated method signatures to use new type name
SqlDelegatedTransaction.cs	Updated type references and added namespace import
SqlDataReader.cs	Updated to use CachedContexts for async context management
SqlConnection.cs	Updated all internal connection references
SqlCommand.cs	Updated type references and added namespace import
SqlCommand.*.cs	Updated async context management to use CachedContexts
SqlBulkCopy.cs	Updated type references and added namespace import
SqlConnectionFactory.cs	Updated instantiation to use new type name
AdapterUtil.cs	Updated method signatures and added namespace import
*.csproj	Updated project files to reference new file locations

Comments suppressed due to low confidence (10)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:1036

The Dispose() method no longer calls base.Dispose() as it did in the original SqlInternalConnection.cs. This could lead to resource leaks if the base class DbConnectionInternal has cleanup logic that needs to be executed. The base class's Dispose() method should be called to ensure proper cleanup of inherited resources.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:68
The GUID value has been changed from lowercase to uppercase (from 1c742caf-6680-40ea-9c26-6b6846079764 to 1C742CAF-6680-40EA-9C26-6B6846079764). While GUIDs are case-insensitive, this change appears unintentional and could lead to confusion. If this was intentional, it should be documented in the PR description.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:821
The property IsAzureSqlConnection was originally an internal property in SqlInternalConnection.cs, but is now marked as private in SqlConnectionInternal.cs. This could be a breaking change if other parts of the codebase outside this class relied on accessing this property. Please verify that this access modifier change is intentional and doesn't break existing functionality.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:651
The properties IsGlobalTransaction and IsGlobalTransactionsEnabledForServer were originally internal in SqlInternalConnection.cs, but are now marked as private in the new merged class. This appears to be an unintentional access modifier change that could break code that depends on these properties being accessible from outside the class.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:639
The property IsEnlistedInTransaction was originally private in SqlInternalConnection.cs, but is now marked as internal in SqlConnectionInternal.cs. While this expands access rather than restricting it, this change should be intentional and documented. If the expanded access is not needed, it should remain private to maintain encapsulation.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:724
The property PromotedDtcToken was originally internal with both getter and setter in SqlInternalConnection.cs, but is now internal with only a private setter. This could be a breaking change if other code sets this property from outside the class. Please verify this change is intentional.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:532
The property CurrentDatabase was originally internal with both getter and setter in SqlInternalConnection.cs, but now has a private setter. This could be a breaking change if other code sets this property from outside the class. Please verify this change is intentional.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs:1065
Variable this._activeConnection may be null at this access as suggested by this null check.

                $"Client Connection Id {_activeConnection.ClientConnectionId}, " +

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:2438

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs:5805
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

                    if (_connection?.InnerConnection is SqlConnectionInternal sqlInternalConnection)
                    {
                        _snapshot = sqlInternalConnection.CachedContexts.ClearDataReaderSnapshot() ?? new Snapshot();
                    }
                    else
                    {
                        _snapshot = new Snapshot();
                    }

Copilot · 2025-11-14T20:26:38Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlTransaction.cs

            // rollback, else we'll throw an exception there that is a breaking change. Of course,
            // if the connection is already closed, then we're free to zombify...
-            if (_connection.InnerConnection is SqlInternalConnection internalConnection && !_isFromApi)
+            if (_connection.InnerConnection is SqlConnectionInternal && !_isFromApi)


The variable internalConnection is declared but never used in this code block. The original code used this variable, but after the type pattern matching change, it's no longer referenced. Consider removing the variable declaration or using a discard pattern if (_connection.InnerConnection is SqlConnectionInternal and not _isFromApi) if the type check is intentional.

Copilot · 2025-11-14T20:26:38Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs

-        private void ValidateActiveOnConnection(SqlInternalConnection connection)
+        private void ValidateActiveOnConnection(SqlConnectionInternal connection)
        {
            bool valid = _active && (connection == _connection) && (connection.DelegatedTransaction == this);


Variable connection may be null at this access as suggested by this null check.

Suggested change

bool valid = _active && (connection == _connection) && (connection.DelegatedTransaction == this);

bool valid = _active

&& (connection == _connection)

&& (connection != null && connection.DelegatedTransaction == this);

Technically true, though I'm not sure the implications of making this change.

Copilot · 2025-11-14T20:26:39Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs

                if (pool == null || (pool != null && pool.Count <= 0))
                { // Non-pooled or pooled and no connections in the pool.
-                    SqlInternalConnectionTds sseConnection = null;
+                    SqlConnectionInternal sseConnection = null;


This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

This also looks odd to me. Will investigate...

Copilot · 2025-11-14T20:26:39Z

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

            // Normally we would simply create a regular connection and open it, but there is no other way to pass the
            // new password down to the constructor. This would have an unwanted impact on the connection pool.
-            SqlInternalConnectionTds con = null;
+            SqlConnectionInternal con = null;


This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

You're not wrong, but because the internal connection is immediately disposed after construction, the cleanest way to write it is with the manual disposal. I'm too unsure what magic happens with the pools when the internal connection is constructed to want to mess with when the internal connection is disposed (ie, via a using declaration)

It looks like this method is opening a new connection (including a round trip) just to seed a new connection pool. Then it's explicitly clearing the old pool so that it's no longer used...
I want to look into this more because it feels fishy.

edwardneal · 2025-11-15T12:40:45Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs

+        protected override void CleanupTransactionOnCompletion(Transaction transaction) =>
+            DelegatedTransaction?.TransactionEnded(transaction);


This movement is missing a comment, and it highlights a slight change in approach: rather than getting the value of a property into a local variable and null-coalescing on the local variable, we null-coalesce on the property directly. If the value of the property changes halfway through the operation, we'll be operating on two different values (or might get a NullReferenceException.)

This might be fine - SqlConnection isn't supposed to be used in a non-thread-safe manner, and this is the first way I can see which would trigger that behaviour - but I want to make sure it's being done intentionally.

As far as I understand, the compile will implicitly create a temporary variable for this. Thus, we get the same behavior as before, but in four less lines :)

I've just checked in SharpLab - you're right!

…nalConnectionTds

…ionTds.cs

…eInternal into ChangeDatabase, remove abstract ChangeDatabaseInternal

…te into Deactivate

…qlInternalConnectionTds

…SqlInternalConnection into that. Migrate usages of ExecuteReaderAsyncCallContext to the new class.

… class.

…w class

…the usages of it are basically doing the same thing

…action, HasLocalTransactionFromAPI

…ce, DelegatedTransaction, Is2008OrNewer, IsAzureSqlConnection, IsEnlistedInTransaction, IsGlobalTransaction, IsGlobalTransactionEnabledForServer, IsLockedForBulkCopy, IsTransactionRoot, PromotedDtcToken

….Data.SqlClient.Connection.SqlInternalConnectionTds

Copilot

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:68

[nitpick] The GUID value in the comment has a different casing than the string literal used to initialize s_globalTransactionTMID. The comment shows 1c742caf-6680-40ea-9c26-6b6846079764 (lowercase) but the code uses 1C742CAF-6680-40EA-9C26-6B6846079764 (uppercase). While functionally equivalent, they should match for consistency.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:1501
[nitpick] Inconsistent formatting in the trace message. The log message ends with a period in $"Object ID {ObjectID}." on line 1501, but most other similar trace messages in this file don't use trailing periods (e.g., lines 1041-1043, 1060-1063). Consider removing the period for consistency.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs:1065
Variable this._activeConnection may be null at this access as suggested by this null check.

                $"Client Connection Id {_activeConnection.ClientConnectionId}, " +

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:2439

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs:5805
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

                    if (_connection?.InnerConnection is SqlConnectionInternal sqlInternalConnection)
                    {
                        _snapshot = sqlInternalConnection.CachedContexts.ClearDataReaderSnapshot() ?? new Snapshot();
                    }
                    else
                    {
                        _snapshot = new Snapshot();
                    }

Copilot · 2025-11-18T16:52:12Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/CachedContexts.cs

+namespace Microsoft.Data.SqlClient.Connection
+{
+    /// <summary>
+    /// Provides cached asynchronous call contexts shared between objects in a connection's context.


[nitpick] Missing XML documentation for the CachedContexts class. As a newly introduced internal class with public-facing methods, it should have a summary describing its purpose and usage pattern, especially since it deals with thread-safe context caching using Interlocked operations.

Suggested change

/// Provides cached asynchronous call contexts shared between objects in a connection's context.

/// Provides thread-safe caching and sharing of asynchronous call contexts between objects within a single SQL connection context.

/// <para>

/// This internal class manages reusable context objects for various asynchronous operations (such as ExecuteNonQueryAsync, ExecuteReaderAsync, etc.)

/// performed on a connection, enabling efficient reuse and reducing allocations.

/// </para>

/// <para>

/// Thread safety is ensured via atomic operations using <see cref="Interlocked"/> methods, allowing concurrent access and updates

/// without explicit locking. All accessors and mutators are designed to be safe for use by multiple threads.

/// </para>

/// <para>

/// Intended for internal use by connection management infrastructure; not part of the public API.

/// </para>

…use a using block. Also removing redundant if statement condition.

Copilot

Pull Request Overview

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

Comments suppressed due to low confidence (9)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:724

The property PromotedDtcToken setter visibility has been changed from implicit (which would have been internal in the base class) to explicitly private set. This could be a breaking change if there are internal consumers that set this property. Please verify that no code within the assembly depends on setting this property externally.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:821
The property IsAzureSqlConnection has been changed from internal to private. This is a breaking change that could affect other code within the assembly that depends on accessing this property. Please verify that no internal code accesses this property.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:645
The property IsGlobalTransaction setter has been changed from implicit (likely internal in base class) to explicitly private set. This could be a breaking change if there are internal consumers that set this property. Please verify that no code within the assembly depends on setting this property externally.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:651
The property IsGlobalTransactionsEnabledForServer setter has been changed from implicit (likely internal in base class) to explicitly private set. This could be a breaking change if there are internal consumers that set this property. Please verify that no code within the assembly depends on setting this property externally.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:639
The property IsEnlistedInTransaction is now defined here with both getter and setter, but it was previously defined as a private property in the base class SqlInternalConnection. The setter visibility has changed from private to internal. This could be an intentional visibility expansion, but should be verified as it may affect thread safety or transaction state management.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:532
The property CurrentDatabase setter visibility has been changed from implicit (which would be internal in the base class) to explicitly private set. This could be a breaking change if there were any internal consumers that set this property. Please verify that no code within the assembly depends on setting this property externally.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs:1065
Variable this._activeConnection may be null at this access as suggested by this null check.

                $"Client Connection Id {_activeConnection.ClientConnectionId}, " +

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs:2439

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs:5805
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

                    if (_connection?.InnerConnection is SqlConnectionInternal sqlInternalConnection)
                    {
                        _snapshot = sqlInternalConnection.CachedContexts.ClearDataReaderSnapshot() ?? new Snapshot();
                    }
                    else
                    {
                        _snapshot = new Snapshot();
                    }

Copilot · 2025-11-18T22:49:57Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/CachedContexts.cs

+        private static bool TrySetContext<TContext>(TContext value, ref TContext? location)
+            where TContext : class
+        {
+            if (value is null)
+            {
+                throw new ArgumentNullException(nameof(value));
+            }
+
+            return Interlocked.CompareExchange(ref location, value, null) is null;
+        }


The TrySetContext method throws ArgumentNullException when value is null. However, the public-facing methods that call this helper (e.g., TrySetCommandExecuteNonQueryAsyncContext, TrySetDataReaderSnapshot) don't document this behavior. Consider adding XML documentation to these methods indicating that they will throw ArgumentNullException if a null value is provided, or handle the null case gracefully by returning false instead of throwing.

Copilot · 2025-11-18T22:49:58Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs

                    // The retry logic can use the current values to get back to the right state.
-                    if (_connection?.InnerConnection is SqlInternalConnection sqlInternalConnection && sqlInternalConnection.CachedDataReaderSnapshot is null)
+                    if (_connection?.InnerConnection is SqlConnectionInternal sqlInternalConnection)
                    {


[nitpick] The logic for caching the snapshot has changed subtly. Previously, the code checked sqlInternalConnection.CachedDataReaderSnapshot is null before attempting to cache. Now it unconditionally calls TrySetDataReaderSnapshot. While functionally equivalent due to the CompareExchange inside TrySetContext, the semantic meaning is less clear. The previous pattern explicitly checked for null, making the intent clearer. Consider adding a comment explaining that TrySetDataReaderSnapshot will only cache if no snapshot is already present.

Suggested change

{

{

// TrySetDataReaderSnapshot will only cache the snapshot if no snapshot is already present.

mdaigle · 2025-11-18T23:47:28Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs

        /// Indicates whether the connection is currently enlisted in a transaction.
        /// </summary>
-        internal bool IsEnlistedInTransaction { get; private set; }
+        internal bool IsEnlistedInTransaction { get; set; }


Change this back to private set

mdaigle · 2025-11-18T23:52:25Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

            }

-            base.Dispose();
+            _whereAbouts = null;


Missing a call to base.Dispose()

mdaigle · 2025-11-19T17:22:37Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs

                    if (_connection?.InnerConnection is SqlInternalConnectionTds sqlInternalConnection)
                    {
-                        _snapshot = Interlocked.Exchange(ref sqlInternalConnection.CachedDataReaderSnapshot, null) ?? new Snapshot();
+                        _snapshot = sqlInternalConnection.CachedContexts.ClearDataReaderSnapshot() ?? new Snapshot();


Lines like this make me think that "pop" might make more sense than "clear". The ultimate intent often isn't to wipe out the cache, it's to check out the cache value.

mdaigle · 2025-11-19T17:26:43Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

            IDbConnectionPool pool = null,
            Func<SqlAuthenticationParameters, CancellationToken, Task<SqlAuthenticationToken>> accessTokenCallback = null,
-            SspiContextProvider sspiContextProvider = null) : base(connectionOptions)
+            SspiContextProvider sspiContextProvider = null)


Keep this call to the base constructor, we still inherit from DbConnectionInternal

mdaigle · 2025-11-19T17:47:08Z

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

            // Normally we would simply create a regular connection and open it, but there is no other way to pass the
            // new password down to the constructor. This would have an unwanted impact on the connection pool.
-            SqlInternalConnectionTds con = null;
+            SqlConnectionInternal con = null;


It looks like this method is opening a new connection (including a round trip) just to seed a new connection pool. Then it's explicitly clearing the old pool so that it's no longer used...
I want to look into this more because it feels fishy.

mdaigle · 2025-11-19T17:48:13Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs

                if (pool == null || (pool != null && pool.Count <= 0))
                { // Non-pooled or pooled and no connections in the pool.
-                    SqlInternalConnectionTds sseConnection = null;
+                    SqlConnectionInternal sseConnection = null;


This also looks odd to me. Will investigate...

benrr101 added this to the 7.0.0-preview3 milestone Nov 14, 2025

benrr101 added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Nov 14, 2025

Copilot AI review requested due to automatic review settings November 14, 2025 20:20

Copilot started reviewing on behalf of benrr101 November 14, 2025 20:21 View session

benrr101 changed the title ~~Dev/russellben/flatten/sqlinternalconnection~~ Flatten | SqlInternalConnectionTds and SqlInternalConnection Nov 14, 2025

Copilot finished reviewing on behalf of benrr101 November 14, 2025 20:22

Copilot AI reviewed Nov 14, 2025

View reviewed changes

edwardneal reviewed Nov 15, 2025

View reviewed changes

benrr101 added 22 commits November 18, 2025 10:42

Change all possible references from SqlInternalConnection to SqlInter…

66c2cd6

…nalConnectionTds

Move BeginTransaction and BeginSqlTransaction into SqlInternalConnect…

2d664a6

…ionTds.cs

Move ChangeDatabase into SqlInternalConnectionTds, move ChangeDatabas…

c796212

…eInternal into ChangeDatabase, remove abstract ChangeDatabaseInternal

Move EnlistTransaction to SqlInternalConnectionTds

13c373c

Remove abstract ValidateConnectionForExecute

a176e2f

Move Enlist into SqlInternalConnectionTds

c5863e3

Move EnlistNonNull into SqlInternalConnectionTds

8002ec7

Move EnlistNull into SqlInternalConnectionTds

202beb5

Removed no longer necessary abstractions in SqlInternalConnection

bb3d541

Move OnError into SqlInternalConnectionTds

4d39842

Move Deactivate into SqlInternalConnectionTds, merge InternalDeactiva…

58c358f

…te into Deactivate

Move GetTransactionCookie into SqlInternalConnectionTds

07750f8

Move FindLiveReader into SqlInternalConnectionTds

dbe9d9e

Merge SqlInternalConnection.Dispose into SqlInternalConnectionTds

355f2d2

Move CleanupTransactionOnCompletion, CreateReferenceCollection into S…

74b0c92

…qlInternalConnectionTds

Introduce a CachedContexts class, move the cached call contexts from …

ce46515

…SqlInternalConnection into that. Migrate usages of ExecuteReaderAsyncCallContext to the new class.

Migrate usages of CachedCommandExecuteNonQueryAsyncContext to the new…

79cf052

… class.

Migrate usages of CachedCommandExecuteXmlReaderAsyncContext to the ne…

0a45c97

…w class

Migrate usage of ReadAsyncCallContext to the new class

7b5a637

Migrate usage of IsDBNullAsyncCallContext to the new class.

ba4c7df

Comments on CachedContexts

a0a1f24

Sure why not move the reader snapshot into the CachedContexts class? …

41b7f2a

…the usages of it are basically doing the same thing

benrr101 added 8 commits November 18, 2025 10:45

Move _whereAbouts and s_globalTransactionTMID

1b52999

Merge constructors

8fb7b0e

Merge AvailableInternalTransaction, CurrentTransaction, HasLocalTrans…

0e78a6e

…action, HasLocalTransactionFromAPI

Merge Connection, ConnectionOptions, CurrentDatabase, CurrentDataSour…

858d98e

…ce, DelegatedTransaction, Is2008OrNewer, IsAzureSqlConnection, IsEnlistedInTransaction, IsGlobalTransaction, IsGlobalTransactionEnabledForServer, IsLockedForBulkCopy, IsTransactionRoot, PromotedDtcToken

Remove SqlInternalConnection

6dfbd5d

Move SqlInternalConnectionTds.cs to SqlConnectionInternal.cs

a83bd7c

Renane Microsoft.Data.SqlClient.SqlInternalConnectionTds to Microsoft…

c4db055

….Data.SqlClient.Connection.SqlInternalConnectionTds

Fix one reference to SqlInternalConnectionTds in unit tests

d010d48

Copilot AI review requested due to automatic review settings November 18, 2025 16:46

benrr101 force-pushed the dev/russellben/flatten/sqlinternalconnection branch from 65e24d1 to d010d48 Compare November 18, 2025 16:46

Copilot started reviewing on behalf of benrr101 November 18, 2025 16:47 View session

Copilot finished reviewing on behalf of benrr101 November 18, 2025 16:49

Copilot AI reviewed Nov 18, 2025

View reviewed changes

benrr101 added 2 commits November 18, 2025 16:40

As per copilot comment, rewriting a block in SqlConnectionFactory to …

4d02cc2

…use a using block. Also removing redundant if statement condition.

As per copilot comment, adding more comments to CachedContexts class

04e5cdf

benrr101 marked this pull request as ready for review November 18, 2025 22:44

benrr101 requested a review from a team as a code owner November 18, 2025 22:44

Copilot AI review requested due to automatic review settings November 18, 2025 22:44

Copilot started reviewing on behalf of benrr101 November 18, 2025 22:44 View session

Copilot finished reviewing on behalf of benrr101 November 18, 2025 22:47

Copilot AI reviewed Nov 18, 2025

View reviewed changes

mdaigle requested changes Nov 19, 2025

View reviewed changes

		protected override void CleanupTransactionOnCompletion(Transaction transaction) =>
		DelegatedTransaction?.TransactionEnded(transaction);

-    /// Provides cached asynchronous call contexts shared between objects in a connection's context.
+    /// Provides thread-safe caching and sharing of asynchronous call contexts between objects within a single SQL connection context.
+    /// <para>
+    /// This internal class manages reusable context objects for various asynchronous operations (such as ExecuteNonQueryAsync, ExecuteReaderAsync, etc.)
+    /// performed on a connection, enabling efficient reuse and reducing allocations.
+    /// </para>
+    /// <para>
+    /// Thread safety is ensured via atomic operations using <see cref="Interlocked"/> methods, allowing concurrent access and updates
+    /// without explicit locking. All accessors and mutators are designed to be safe for use by multiple threads.
+    /// </para>
+    /// <para>
+    /// Intended for internal use by connection management infrastructure; not part of the public API.
+    /// </para>

	{
	{
	// TrySetDataReaderSnapshot will only cache the snapshot if no snapshot is already present.

Flatten | SqlInternalConnectionTds and SqlInternalConnection #3773

Are you sure you want to change the base?

Flatten | SqlInternalConnectionTds and SqlInternalConnection #3773

Uh oh!

Conversation

benrr101 commented Nov 14, 2025

Description

Issues

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

mdaigle Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mdaigle Nov 18, 2025 •

edited

Loading