From d15dfddfe6fc833df1c22f9cf896ed67f48727e7 Mon Sep 17 00:00:00 2001 From: PetarJerinic Date: Sat, 22 Aug 2026 15:52:13 +0200 Subject: [PATCH 1/2] Expose SqlBatch commands on the command diagnostic payloads --- .../SqlClientDiagnostic.xml | 15 ++ .../Microsoft.Data.SqlClient.Diagnostics.cs | 6 + .../Diagnostics/SqlClientCommandAfter.cs | 9 +- .../Diagnostics/SqlClientCommandBefore.cs | 9 +- .../Diagnostics/SqlClientCommandError.cs | 9 +- .../Diagnostics/SqlDiagnosticListener.cs | 9 +- .../src/Microsoft/Data/SqlClient/SqlBatch.cs | 6 + .../Data/SqlClient/SqlCommand.Batch.cs | 12 + .../SqlClientCommandPayloadTest.cs | 205 +++++++++++++++ .../Diagnostics/SqlDiagnosticListenerTest.cs | 71 +++++ .../BatchDiagnosticsTests.cs | 245 ++++++++++++++++++ 11 files changed, 587 insertions(+), 9 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandPayloadTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListenerTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs diff --git a/doc/snippets/Microsoft.Data.SqlClient/SqlClientDiagnostic.xml b/doc/snippets/Microsoft.Data.SqlClient/SqlClientDiagnostic.xml index d84a520c54..9103994549 100644 --- a/doc/snippets/Microsoft.Data.SqlClient/SqlClientDiagnostic.xml +++ b/doc/snippets/Microsoft.Data.SqlClient/SqlClientDiagnostic.xml @@ -98,6 +98,11 @@ The command object that is executing. + + + The commands that make up the batch when the command is executing as part of a , or when it is not. + + @@ -130,6 +135,11 @@ An IDictionary of statistic information about the event that has completed. + + + The commands that make up the batch when the command is executing as part of a , or when it is not. + + @@ -162,6 +172,11 @@ The exception object that caused the command execution to fail. + + + The commands that make up the batch when the command is executing as part of a , or when it is not. + + diff --git a/src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.Diagnostics.cs b/src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.Diagnostics.cs index edbda75d54..a73062617d 100644 --- a/src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.Diagnostics.cs +++ b/src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.Diagnostics.cs @@ -23,6 +23,8 @@ public sealed class SqlClientCommandAfter : System.Collections.Generic.IReadOnly public SqlCommand Command => throw null; /// public System.Collections.IDictionary Statistics => throw null; + /// + public System.Collections.Generic.IReadOnlyList BatchCommands => throw null; /// public int Count => throw null; /// @@ -50,6 +52,8 @@ public sealed class SqlClientCommandBefore : System.Collections.Generic.IReadOnl public long? TransactionId => throw null; /// public SqlCommand Command => throw null; + /// + public System.Collections.Generic.IReadOnlyList BatchCommands => throw null; /// public int Count => throw null; /// @@ -79,6 +83,8 @@ public sealed class SqlClientCommandError : System.Collections.Generic.IReadOnly public SqlCommand Command => throw null; /// public System.Exception Exception { get; } + /// + public System.Collections.Generic.IReadOnlyList BatchCommands => throw null; /// public int Count => throw null; /// diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandAfter.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandAfter.cs index 992e2086d2..a7fa807500 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandAfter.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandAfter.cs @@ -21,7 +21,8 @@ internal SqlClientCommandAfter( Guid? connectionId, long? transactionId, SqlCommand command, - IDictionary statistics) + IDictionary statistics, + IReadOnlyList batchCommands) { OperationId = operationId; Operation = operation; @@ -30,6 +31,7 @@ internal SqlClientCommandAfter( TransactionId = transactionId; Command = command; Statistics = statistics; + BatchCommands = batchCommands; } /// @@ -46,9 +48,11 @@ internal SqlClientCommandAfter( public SqlCommand Command { get; } /// public IDictionary Statistics { get; } + /// + public IReadOnlyList BatchCommands { get; } /// - public int Count => 3 + 4; + public int Count => 3 + 5; /// public KeyValuePair this[int index] @@ -62,6 +66,7 @@ public KeyValuePair this[int index] 4 => new KeyValuePair(nameof(TransactionId), TransactionId), 5 => new KeyValuePair(nameof(Command), Command), 6 => new KeyValuePair(nameof(Statistics), Statistics), + 7 => new KeyValuePair(nameof(BatchCommands), BatchCommands), _ => throw new IndexOutOfRangeException(nameof(index)), }; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandBefore.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandBefore.cs index 71c9df6e1e..04d6bea0bd 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandBefore.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandBefore.cs @@ -20,7 +20,8 @@ internal SqlClientCommandBefore( long timestamp, Guid? connectionId, long? transactionId, - SqlCommand command) + SqlCommand command, + IReadOnlyList batchCommands) { OperationId = operationId; Operation = operation; @@ -28,6 +29,7 @@ internal SqlClientCommandBefore( ConnectionId = connectionId; TransactionId = transactionId; Command = command; + BatchCommands = batchCommands; } /// @@ -42,9 +44,11 @@ internal SqlClientCommandBefore( public long? TransactionId { get; } /// public SqlCommand Command { get; } + /// + public IReadOnlyList BatchCommands { get; } /// - public int Count => 3 + 3; + public int Count => 3 + 4; /// public KeyValuePair this[int index] @@ -57,6 +61,7 @@ public KeyValuePair this[int index] 3 => new KeyValuePair(nameof(ConnectionId), ConnectionId), 4 => new KeyValuePair(nameof(TransactionId), TransactionId), 5 => new KeyValuePair(nameof(Command), Command), + 6 => new KeyValuePair(nameof(BatchCommands), BatchCommands), _ => throw new IndexOutOfRangeException(nameof(index)), }; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandError.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandError.cs index b21d72b575..2f5b02add3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandError.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandError.cs @@ -22,7 +22,8 @@ internal SqlClientCommandError( Guid? connectionId, long? transactionId, SqlCommand command, - Exception exception) + Exception exception, + IReadOnlyList batchCommands) { OperationId = operationId; Operation = operation; @@ -31,6 +32,7 @@ internal SqlClientCommandError( TransactionId = transactionId; Command = command; Exception = exception; + BatchCommands = batchCommands; } /// public Guid OperationId { get; } @@ -46,9 +48,11 @@ internal SqlClientCommandError( public SqlCommand Command { get; } /// public Exception Exception { get; } + /// + public IReadOnlyList BatchCommands { get; } /// - public int Count => 3 + 4; + public int Count => 3 + 5; /// public KeyValuePair this[int index] @@ -62,6 +66,7 @@ public KeyValuePair this[int index] 4 => new KeyValuePair(nameof(TransactionId), TransactionId), 5 => new KeyValuePair(nameof(Command), Command), 6 => new KeyValuePair(nameof(Exception), Exception), + 7 => new KeyValuePair(nameof(BatchCommands), BatchCommands), _ => throw new IndexOutOfRangeException(nameof(index)), }; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListener.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListener.cs index 9ed25cc8d8..0be54abc72 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListener.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListener.cs @@ -86,7 +86,8 @@ public void WriteCommandAfter( sqlCommand.Connection?.ClientConnectionId, transaction?.InternalTransaction?.TransactionId, sqlCommand, - sqlCommand.Statistics?.GetDictionary() + sqlCommand.Statistics?.GetDictionary(), + sqlCommand.BatchCommands ) ); } @@ -112,7 +113,8 @@ public Guid WriteCommandBefore( Stopwatch.GetTimestamp(), sqlCommand.Connection?.ClientConnectionId, transaction?.InternalTransaction?.TransactionId, - sqlCommand + sqlCommand, + sqlCommand.BatchCommands ) ); @@ -142,7 +144,8 @@ public void WriteCommandError( sqlCommand.Connection?.ClientConnectionId, transaction?.InternalTransaction?.TransactionId, sqlCommand, - ex + ex, + sqlCommand.BatchCommands ) ); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBatch.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBatch.cs index d24b9724ac..58d5f1e867 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBatch.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBatch.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.Data; using System.Data.Common; using System.Threading; @@ -23,6 +24,7 @@ public class SqlBatch : private SqlCommand _batchCommand; private List _commands; private SqlBatchCommandCollection _providerCommands; + private ReadOnlyCollection _readOnlyCommands; /// public SqlBatch() @@ -213,6 +215,7 @@ void Dispose() _batchCommand = null; _commands?.Clear(); _commands = null; + _readOnlyCommands = null; #if NET base.Dispose(); #endif @@ -355,6 +358,9 @@ private void SetupBatchCommandExecute() } _batchCommand.Connection = Connection; _batchCommand.Transaction = Transaction; + // Surfaced on the command diagnostic payloads. The wrapper is a view over _commands, + // which is never reassigned while the batch is usable, so it is allocated once. + _batchCommand.BatchCommands = _readOnlyCommands != null ? _readOnlyCommands : _readOnlyCommands = new ReadOnlyCollection(_commands); _batchCommand.SetBatchRPCMode(true, _commands.Count); _batchCommand.Parameters.Clear(); for (int index = 0; index < _commands.Count; index++) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Batch.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Batch.cs index da7d1e2ed7..2c51e33f47 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Batch.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Batch.cs @@ -11,6 +11,18 @@ namespace Microsoft.Data.SqlClient // @TODO: There's a good question here - should this be a separate type of SqlCommand? public sealed partial class SqlCommand { + #region Internal Properties + + /// + /// The commands of the this command is executing on behalf of, or + /// when it is not executing on behalf of a batch. Set by + /// before each execution and surfaced on the command diagnostic + /// payloads. + /// + internal IReadOnlyList BatchCommands { get; set; } + + #endregion + #region Internal Methods internal void AddBatchCommand(SqlBatchCommand batchCommand) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandPayloadTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandPayloadTest.cs new file mode 100644 index 0000000000..c8882409d2 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlClientCommandPayloadTest.cs @@ -0,0 +1,205 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections; +using System.Collections.Generic; +using Microsoft.Data.SqlClient.Diagnostics; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.Diagnostics; + +/// +/// Verifies that the command diagnostic payloads carry the batch that produced them, both as a +/// typed property and as an entry in the key/value view that +/// DiagnosticSource subscribers enumerate. +/// +public class SqlClientCommandPayloadTest +{ + private const string Operation = "ExecuteNonQuery"; + + private static readonly Guid OperationId = Guid.Parse("11111111-1111-1111-1111-111111111111"); + private static readonly Guid ConnectionId = Guid.Parse("22222222-2222-2222-2222-222222222222"); + private const long TransactionId = 42; + private const long Timestamp = 1234567890; + + private static IReadOnlyList CreateBatchCommands() => + new[] + { + new SqlBatchCommand("SELECT 1;"), + new SqlBatchCommand("SELECT 2;"), + }; + + #region SqlClientCommandBefore + + /// + /// Verifies that SqlClientCommandBefore.BatchCommands returns the list it was constructed with. + /// + [Fact] + public void SqlClientCommandBefore_BatchCommands_ReturnsConstructorArgument() + { + IReadOnlyList batchCommands = CreateBatchCommands(); + SqlClientCommandBefore payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, new SqlCommand(), batchCommands); + + Assert.Same(batchCommands, payload.BatchCommands); + } + + /// + /// Verifies that SqlClientCommandBefore.BatchCommands is null - present on the type, but + /// null-valued - when the command is not executing as part of a SqlBatch. + /// + [Fact] + public void SqlClientCommandBefore_BatchCommands_IsNullForNonBatchCommand() + { + SqlClientCommandBefore payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, new SqlCommand(), null); + + Assert.Null(payload.BatchCommands); + } + + /// + /// Verifies that the key/value view of SqlClientCommandBefore appends BatchCommands as the last + /// entry and leaves the meaning of every pre-existing index unchanged. + /// + [Fact] + public void SqlClientCommandBefore_KeyValueView_AppendsBatchCommandsAsLastEntry() + { + IReadOnlyList batchCommands = CreateBatchCommands(); + SqlCommand command = new(); + SqlClientCommandBefore payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, command, batchCommands); + + KeyValuePair[] expected = + { + new("OperationId", OperationId), + new("Operation", Operation), + new("Timestamp", Timestamp), + new("ConnectionId", ConnectionId), + new("TransactionId", TransactionId), + new("Command", command), + new("BatchCommands", batchCommands), + }; + + Assert.Equal(expected, payload); + } + + #endregion + + #region SqlClientCommandAfter + + /// + /// Verifies that SqlClientCommandAfter.BatchCommands returns the list it was constructed with. + /// + [Fact] + public void SqlClientCommandAfter_BatchCommands_ReturnsConstructorArgument() + { + IReadOnlyList batchCommands = CreateBatchCommands(); + SqlClientCommandAfter payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, new SqlCommand(), new Hashtable(), batchCommands); + + Assert.Same(batchCommands, payload.BatchCommands); + } + + /// + /// Verifies that SqlClientCommandAfter.BatchCommands is null - present on the type, but + /// null-valued - when the command is not executing as part of a SqlBatch. + /// + [Fact] + public void SqlClientCommandAfter_BatchCommands_IsNullForNonBatchCommand() + { + SqlClientCommandAfter payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, new SqlCommand(), new Hashtable(), null); + + Assert.Null(payload.BatchCommands); + } + + /// + /// Verifies that the key/value view of SqlClientCommandAfter appends BatchCommands as the last + /// entry and leaves the meaning of every pre-existing index unchanged. + /// + [Fact] + public void SqlClientCommandAfter_KeyValueView_AppendsBatchCommandsAsLastEntry() + { + IReadOnlyList batchCommands = CreateBatchCommands(); + SqlCommand command = new(); + Hashtable statistics = new(); + SqlClientCommandAfter payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, command, statistics, batchCommands); + + KeyValuePair[] expected = + { + new("OperationId", OperationId), + new("Operation", Operation), + new("Timestamp", Timestamp), + new("ConnectionId", ConnectionId), + new("TransactionId", TransactionId), + new("Command", command), + new("Statistics", statistics), + new("BatchCommands", batchCommands), + }; + + Assert.Equal(expected, payload); + } + + #endregion + + #region SqlClientCommandError + + /// + /// Verifies that SqlClientCommandError.BatchCommands returns the list it was constructed with. + /// + [Fact] + public void SqlClientCommandError_BatchCommands_ReturnsConstructorArgument() + { + IReadOnlyList batchCommands = CreateBatchCommands(); + SqlClientCommandError payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, new SqlCommand(), new InvalidOperationException(), batchCommands); + + Assert.Same(batchCommands, payload.BatchCommands); + } + + /// + /// Verifies that SqlClientCommandError.BatchCommands is null - present on the type, but + /// null-valued - when the command is not executing as part of a SqlBatch. + /// + [Fact] + public void SqlClientCommandError_BatchCommands_IsNullForNonBatchCommand() + { + SqlClientCommandError payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, new SqlCommand(), new InvalidOperationException(), null); + + Assert.Null(payload.BatchCommands); + } + + /// + /// Verifies that the key/value view of SqlClientCommandError appends BatchCommands as the last + /// entry and leaves the meaning of every pre-existing index unchanged. + /// + [Fact] + public void SqlClientCommandError_KeyValueView_AppendsBatchCommandsAsLastEntry() + { + IReadOnlyList batchCommands = CreateBatchCommands(); + SqlCommand command = new(); + InvalidOperationException exception = new(); + SqlClientCommandError payload = new( + OperationId, Operation, Timestamp, ConnectionId, TransactionId, command, exception, batchCommands); + + KeyValuePair[] expected = + { + new("OperationId", OperationId), + new("Operation", Operation), + new("Timestamp", Timestamp), + new("ConnectionId", ConnectionId), + new("TransactionId", TransactionId), + new("Command", command), + new("Exception", exception), + new("BatchCommands", batchCommands), + }; + + Assert.Equal(expected, payload); + } + + #endregion +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListenerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListenerTest.cs new file mode 100644 index 0000000000..095e8ef7cc --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListenerTest.cs @@ -0,0 +1,71 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.Data.SqlClient.Diagnostics; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.Diagnostics; + +/// +/// Verifies that SqlDiagnosticListener carries the executing command's batch onto the payload it +/// writes for a completed execution. This is the one construction site that cannot be reached end +/// to end from this project: the simulated TDS server has no RPC handler, so a SqlBatch always +/// faults before WriteCommandAfter is called. +/// +// Serializes execution with the simulated-server tests. Required because constructing a +// SqlDiagnosticListener publishes it to the process-wide DiagnosticListener.AllListeners, which +// those tests subscribe to. +[Collection(SimulatedServerTestCollection.Name)] +public class SqlDiagnosticListenerTest +{ + /// + /// Verifies that WriteCommandAfter reports the commands of the batch the command is executing + /// on behalf of, and not null. + /// + [Fact] + public void WriteCommandAfter_ForBatchCommand_CarriesBatchCommands() + { + IReadOnlyList batchCommands = new[] { new SqlBatchCommand("SELECT 1;") }; + using SqlCommand command = new(); + command.BatchCommands = batchCommands; + + using SqlDiagnosticListener listener = new(); + using PayloadCollector collector = new(listener); + + listener.WriteCommandAfter(Guid.NewGuid(), command, transaction: null); + + SqlClientCommandAfter payload = collector.SinglePayload(SqlClientCommandAfter.Name); + Assert.Same(batchCommands, payload.BatchCommands); + } + + /// + /// Collects the payloads written to a single DiagnosticListener for the lifetime of the + /// instance. + /// + private sealed class PayloadCollector : IObserver>, IDisposable + { + private readonly List> _events = new(); + private readonly IDisposable _subscription; + + public PayloadCollector(SqlDiagnosticListener listener) => _subscription = listener.Subscribe(this); + + public T SinglePayload(string eventName) => + Assert.Single(_events.Where(e => e.Key == eventName).Select(e => e.Value).OfType()); + + public void OnNext(KeyValuePair value) => _events.Add(value); + + public void OnCompleted() + { + } + + public void OnError(Exception error) + { + } + + public void Dispose() => _subscription.Dispose(); + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs new file mode 100644 index 0000000000..1c63a559be --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs @@ -0,0 +1,245 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Reflection; +using System.Threading.Tasks; +using Microsoft.Data.SqlClient.Diagnostics; +using Microsoft.SqlServer.TDS.Servers; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Verifies that a SqlBatch execution reaches a DiagnosticSource subscriber with the batch it +/// executed, that an ordinary SqlCommand execution reports no batch, and that the state SqlBatch +/// caches to make that possible is released on disposal. +/// +// Serializes execution with other SimulatedServerTests classes. Required here because +// DiagnosticListener.AllListeners is process-wide state that these tests subscribe to. +[Collection(SimulatedServerTestCollection.Name)] +public class BatchDiagnosticsTests : IDisposable +{ + private const string WriteCommandBefore = "Microsoft.Data.SqlClient.WriteCommandBefore"; + private const string WriteCommandAfter = "Microsoft.Data.SqlClient.WriteCommandAfter"; + private const string WriteCommandError = "Microsoft.Data.SqlClient.WriteCommandError"; + + private static readonly string[] BatchCommandTexts = { "SELECT 1;", "SELECT 2;", "SELECT 3;" }; + + private readonly TdsServerFixture _fixture; + private readonly string _connectionString; + + public BatchDiagnosticsTests() + { + _fixture = new TdsServerFixture(); + TdsServer server = _fixture.TdsServer; + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false + }; + _connectionString = builder.ConnectionString; + } + + public void Dispose() => _fixture.Dispose(); + + /// + /// Verifies that every SqlBatch execute path - sync and async - surfaces the batch's commands, + /// in order and as a collection the subscriber cannot mutate, on the WriteCommandBefore + /// payload. + /// + [Theory] + [InlineData(nameof(SqlBatch.ExecuteNonQuery))] + [InlineData(nameof(SqlBatch.ExecuteNonQueryAsync))] + [InlineData(nameof(SqlBatch.ExecuteScalar))] + [InlineData(nameof(SqlBatch.ExecuteScalarAsync))] + [InlineData(nameof(SqlBatch.ExecuteReader))] + [InlineData(nameof(SqlBatch.ExecuteReaderAsync))] + public async Task Batch_CommandBeforePayload_CarriesBatchCommands(string executeMethod) + { + using CommandEventCollector collector = new(); + using SqlConnection connection = new(_connectionString); + connection.Open(); + using SqlBatch batch = CreateBatch(connection); + + // The simulated TDS server has no RPC handler, so a batch always faults at the transport + // level. WriteCommandBefore is emitted before the fault, which is the payload under test. + await Assert.ThrowsAnyAsync(() => ExecuteAsync(batch, executeMethod)); + + SqlClientCommandBefore payload = collector.SinglePayload(WriteCommandBefore); + Assert.Equal(BatchCommandTexts, payload.BatchCommands.Select(command => command.CommandText)); + + // The payload must not hand the subscriber a mutable view of the live batch, which it could + // downcast to and edit while the batch is executing. + Assert.False( + payload.BatchCommands is ICollection { IsReadOnly: false }, + "BatchCommands must not be a writable collection."); + } + + /// + /// Verifies that a failed SqlBatch execution surfaces the batch's commands on the + /// WriteCommandError payload. + /// + [Fact] + public async Task Batch_CommandErrorPayload_CarriesBatchCommands() + { + using CommandEventCollector collector = new(); + using SqlConnection connection = new(_connectionString); + connection.Open(); + using SqlBatch batch = CreateBatch(connection); + + await Assert.ThrowsAnyAsync(() => ExecuteAsync(batch, nameof(SqlBatch.ExecuteNonQuery))); + + SqlClientCommandError payload = collector.SinglePayload(WriteCommandError); + Assert.Equal(BatchCommandTexts, payload.BatchCommands.Select(command => command.CommandText)); + } + + /// + /// Verifies that disposing a SqlBatch releases the cached read-only view of its commands that + /// an execution allocated, rather than leaving it holding a wrapper over the emptied list. + /// + [Fact] + public void Batch_Dispose_ReleasesCachedBatchCommandsView() + { + using SqlConnection connection = new(_connectionString); + connection.Open(); + SqlBatch batch = CreateBatch(connection); + + // The view is allocated by the execute path, so it has to run before disposal is meaningful. + Assert.ThrowsAny(() => batch.ExecuteNonQuery()); + Assert.NotNull(BatchAccessor.ReadOnlyCommands(batch)); + + batch.Dispose(); + + Assert.Null(BatchAccessor.ReadOnlyCommands(batch)); + } + + /// + /// Verifies that an ordinary SqlCommand execution - which is not part of a SqlBatch - reports a + /// null BatchCommands on both the WriteCommandBefore and WriteCommandAfter payloads. + /// + [Fact] + public void PlainCommand_CommandPayloads_ReportNoBatchCommands() + { + using CommandEventCollector collector = new(); + using SqlConnection connection = new(_connectionString); + connection.Open(); + using SqlCommand command = new("SELECT 1;", connection); + + command.ExecuteNonQuery(); + + Assert.Null(collector.SinglePayload(WriteCommandBefore).BatchCommands); + Assert.Null(collector.SinglePayload(WriteCommandAfter).BatchCommands); + } + + private static SqlBatch CreateBatch(SqlConnection connection) + { + SqlBatch batch = new(connection); + foreach (string commandText in BatchCommandTexts) + { + batch.BatchCommands.Add(new SqlBatchCommand(commandText)); + } + + return batch; + } + + private static async Task ExecuteAsync(SqlBatch batch, string executeMethod) + { + switch (executeMethod) + { + case nameof(SqlBatch.ExecuteNonQuery): + batch.ExecuteNonQuery(); + break; + case nameof(SqlBatch.ExecuteNonQueryAsync): + await batch.ExecuteNonQueryAsync(); + break; + case nameof(SqlBatch.ExecuteScalar): + batch.ExecuteScalar(); + break; + case nameof(SqlBatch.ExecuteScalarAsync): + await batch.ExecuteScalarAsync(); + break; + case nameof(SqlBatch.ExecuteReader): + batch.ExecuteReader().Dispose(); + break; + case nameof(SqlBatch.ExecuteReaderAsync): + (await batch.ExecuteReaderAsync()).Dispose(); + break; + default: + throw new ArgumentOutOfRangeException(nameof(executeMethod), executeMethod, null); + } + } + + /// + /// Reflection wrapper over the private members of . Nested so that the + /// type initializer runs only for the tests that read them. + /// + private static class BatchAccessor + { + // Private implementation detail, so a rename would silently turn Batch_Dispose_Releases- + // CachedBatchCommandsView into a no-op. Fail loudly instead. + private static readonly FieldInfo s_readOnlyCommands = + typeof(SqlBatch).GetField("_readOnlyCommands", BindingFlags.Instance | BindingFlags.NonPublic) + ?? throw new InvalidOperationException("SqlBatch no longer declares a field '_readOnlyCommands'."); + + internal static object? ReadOnlyCommands(SqlBatch batch) => s_readOnlyCommands.GetValue(batch); + } + + /// + /// Collects the payloads written to the SqlClient DiagnosticListener for the lifetime of the + /// instance. + /// + private sealed class CommandEventCollector + : IObserver, IObserver>, IDisposable + { + private readonly List> _events = new(); + private readonly IDisposable _allListenersSubscription; + private IDisposable? _listenerSubscription; + + public CommandEventCollector() => + _allListenersSubscription = DiagnosticListener.AllListeners.Subscribe(this); + + public T SinglePayload(string eventName) + { + lock (_events) + { + return Assert.Single(_events.Where(e => e.Key == eventName).Select(e => e.Value).OfType()); + } + } + + public void OnNext(DiagnosticListener listener) + { + if (listener.Name == "SqlClientDiagnosticListener") + { + _listenerSubscription = listener.Subscribe(this); + } + } + + public void OnNext(KeyValuePair value) + { + lock (_events) + { + _events.Add(value); + } + } + + public void OnCompleted() + { + } + + public void OnError(Exception error) + { + } + + public void Dispose() + { + _listenerSubscription?.Dispose(); + _allListenersSubscription.Dispose(); + } + } +} From b0792239beeffc550d7c9d75cc799d17a98664bb Mon Sep 17 00:00:00 2001 From: PetarJerinic Date: Tue, 25 Aug 2026 00:37:16 +0200 Subject: [PATCH 2/2] Fixed test repairing subscription leak in BatchDiagnosticsTests.cs --- .../BatchDiagnosticsTests.cs | 97 +++++++++++++++++-- 1 file changed, 90 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs index 1c63a559be..504ba3cdfe 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/BatchDiagnosticsTests.cs @@ -7,6 +7,7 @@ using System.Diagnostics; using System.Linq; using System.Reflection; +using System.Runtime.CompilerServices; using System.Threading.Tasks; using Microsoft.Data.SqlClient.Diagnostics; using Microsoft.SqlServer.TDS.Servers; @@ -27,6 +28,7 @@ public class BatchDiagnosticsTests : IDisposable private const string WriteCommandBefore = "Microsoft.Data.SqlClient.WriteCommandBefore"; private const string WriteCommandAfter = "Microsoft.Data.SqlClient.WriteCommandAfter"; private const string WriteCommandError = "Microsoft.Data.SqlClient.WriteCommandError"; + private const string SqlClientListenerName = "SqlClientDiagnosticListener"; private static readonly string[] BatchCommandTexts = { "SELECT 1;", "SELECT 2;", "SELECT 3;" }; @@ -106,12 +108,13 @@ public async Task Batch_CommandErrorPayload_CarriesBatchCommands() [Fact] public void Batch_Dispose_ReleasesCachedBatchCommandsView() { - using SqlConnection connection = new(_connectionString); - connection.Open(); + // SqlBatch caches the view before it does any I/O, and needs only a non-null connection to + // get there, so this observes disposal without a server, a login or a packet write. + using SqlConnection connection = new(); SqlBatch batch = CreateBatch(connection); // The view is allocated by the execute path, so it has to run before disposal is meaningful. - Assert.ThrowsAny(() => batch.ExecuteNonQuery()); + Assert.Throws(() => batch.ExecuteNonQuery()); Assert.NotNull(BatchAccessor.ReadOnlyCommands(batch)); batch.Dispose(); @@ -137,6 +140,43 @@ public void PlainCommand_CommandPayloads_ReportNoBatchCommands() Assert.Null(collector.SinglePayload(WriteCommandAfter).BatchCommands); } + /// + /// Verifies that disposing a CommandEventCollector leaves no SqlClient DiagnosticListener + /// enabled. The driver publishes one listener per owning type - SqlCommand, SqlConnection and + /// SqlTransaction - all under the same name, so a collector that tracks a single subscription + /// releases one of them and leaves the rest enabled for every later test in the process. + /// + [Fact] + public void CommandEventCollector_Dispose_LeavesNoListenerEnabled() + { + // Each listener is created by its owning type's initializer, so force all three to have + // run rather than depending on which type an earlier test happened to touch first. + RuntimeHelpers.RunClassConstructor(typeof(SqlCommand).TypeHandle); + RuntimeHelpers.RunClassConstructor(typeof(SqlConnection).TypeHandle); + RuntimeHelpers.RunClassConstructor(typeof(SqlTransaction).TypeHandle); + + List listeners = SqlClientListeners(); + Assert.NotEmpty(listeners); + + using (new CommandEventCollector()) + { + Assert.All(listeners, listener => Assert.True(listener.IsEnabled(WriteCommandAfter))); + } + + Assert.All(listeners, listener => Assert.False(listener.IsEnabled(WriteCommandAfter))); + } + + /// + /// Every DiagnosticListener the driver has published under the SqlClient name. Subscribing to + /// AllListeners replays the listeners that already exist, which is the only way to reach them. + /// + private static List SqlClientListeners() + { + List listeners = new(); + DiagnosticListener.AllListeners.Subscribe(new ListenerCollector(listeners)).Dispose(); + return listeners; + } + private static SqlBatch CreateBatch(SqlConnection connection) { SqlBatch batch = new(connection); @@ -190,6 +230,33 @@ private static class BatchAccessor internal static object? ReadOnlyCommands(SqlBatch batch) => s_readOnlyCommands.GetValue(batch); } + /// + /// Records the SqlClient DiagnosticListeners replayed to it by AllListeners, without + /// subscribing to any of them. + /// + private sealed class ListenerCollector : IObserver + { + private readonly List _listeners; + + public ListenerCollector(List listeners) => _listeners = listeners; + + public void OnNext(DiagnosticListener listener) + { + if (listener.Name == SqlClientListenerName) + { + _listeners.Add(listener); + } + } + + public void OnCompleted() + { + } + + public void OnError(Exception error) + { + } + } + /// /// Collects the payloads written to the SqlClient DiagnosticListener for the lifetime of the /// instance. @@ -198,8 +265,8 @@ private sealed class CommandEventCollector : IObserver, IObserver>, IDisposable { private readonly List> _events = new(); + private readonly List _listenerSubscriptions = new(); private readonly IDisposable _allListenersSubscription; - private IDisposable? _listenerSubscription; public CommandEventCollector() => _allListenersSubscription = DiagnosticListener.AllListeners.Subscribe(this); @@ -214,9 +281,16 @@ public T SinglePayload(string eventName) public void OnNext(DiagnosticListener listener) { - if (listener.Name == "SqlClientDiagnosticListener") + if (listener.Name == SqlClientListenerName) { - _listenerSubscription = listener.Subscribe(this); + // The driver publishes one listener per owning type under this name, and a + // listener created later still arrives here, so every subscription has to be + // kept. Dropping one leaves that listener enabled for the rest of the process. + IDisposable subscription = listener.Subscribe(this); + lock (_listenerSubscriptions) + { + _listenerSubscriptions.Add(subscription); + } } } @@ -238,8 +312,17 @@ public void OnError(Exception error) public void Dispose() { - _listenerSubscription?.Dispose(); + // Stop new listeners arriving first, so nothing is added behind the loop below. _allListenersSubscription.Dispose(); + lock (_listenerSubscriptions) + { + foreach (IDisposable subscription in _listenerSubscriptions) + { + subscription.Dispose(); + } + + _listenerSubscriptions.Clear(); + } } } }