diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index b3b6becbdf..6d82040ca6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -4653,8 +4653,10 @@ internal sealed partial class StateSnapshot { private sealed partial class PacketData { - public readonly byte[] Buffer; - public readonly int Read; + // These are mutable so that instances can be recycled through the snapshot's + // free list instead of allocating a new node for every packet that is appended. + public byte[] Buffer; + public int Read; public PacketData NextPacket; public PacketData PrevPacket; @@ -4666,9 +4668,38 @@ private sealed partial class PacketData public int RunningDataSize; public PacketData(byte[] buffer, int read) + { + // Delegate so that a freshly allocated node and a recycled one are initialized + // by exactly one piece of code. Keeping these paths separate is how the + // RunningDataSize reset was missed previously. + Initialize(buffer, read); + } + + /// + /// Re-initializes an instance taken from the free list so that it describes a new packet. + /// + public void Initialize(byte[] buffer, int read) { Buffer = buffer; Read = read; + NextPacket = null; + PrevPacket = null; + RunningDataSize = 0; + ResetDebugState(); + } + + /// + /// Releases all references held by this instance before it is placed on the free list + /// so that recycling a node cannot keep a packet buffer alive. + /// + public void Reset() + { + Buffer = null; + Read = 0; + NextPacket = null; + PrevPacket = null; + RunningDataSize = 0; + ResetDebugState(); } public int PacketID => Packet.GetIDFromHeader(Buffer.AsSpan(0, TdsEnums.HEADER_LEN)); @@ -4698,10 +4729,13 @@ internal int GetPacketDataSize() internal void CheckDebugDataHash() => CheckDebugDataHashImpl(); + internal void ResetDebugState() => ResetDebugStateImpl(); + partial void SetDebugStackImpl(string value); partial void SetDebugPacketIdImpl(int value); partial void SetDebugDataHashImpl(); partial void CheckDebugDataHashImpl(); + partial void ResetDebugStateImpl(); } #if DEBUG @@ -4845,6 +4879,13 @@ public string Status partial void SetDebugPacketIdImpl(int value) => DebugPacketId = value; + partial void ResetDebugStateImpl() + { + DebugPacketId = 0; + Stack = null; + Hash = null; + } + partial void SetDebugDataHashImpl() { if (Buffer != null) @@ -4990,6 +5031,25 @@ internal void Restore(TdsParserStateObject stateObj) private PacketData _current; private PacketData _continuePacket; + /// + /// Head of a free list of instances, linked through + /// . A snapshot is taken and released for every async + /// continuation, so recycling these nodes avoids allocating one per packet per replay. + /// The list is bounded so that an unusually long replay cannot retain nodes indefinitely. + /// + private PacketData _sparePackets; + private int _sparePacketCount; + + /// + /// Upper bound on the free list. The value is empirical rather than derived: most + /// snapshots hold a single packet, so even a one entry list removes the bulk of the + /// allocations, and a larger bound only helps reads that span several packets such as + /// large rows or XML and binary columns. 16 was the point past which the read + /// benchmarks stopped improving. The cost is bounded at roughly 900 bytes retained per + /// state object, and the parked nodes hold no buffer references. + /// + private const int MaxSparePacketCount = 16; + #if DEBUG private int _packetCounter; private int _rollingPend = 0; @@ -5069,7 +5129,7 @@ internal void AppendPacketData(byte[] buffer, int read) } } #endif - PacketData packetData = new PacketData(buffer, read); + PacketData packetData = RentPacket(buffer, read); #if DEBUG packetData.SetDebugStack(_stateObj._lastStack); packetData.SetDebugPacketId(Interlocked.Increment(ref _packetCounter)); @@ -5145,9 +5205,11 @@ internal bool MoveToContinue() internal void CaptureAsStart(TdsParserStateObject stateObj) { - _firstPacket = null; - _lastPacket = null; - _current = null; + // Note this also clears _continuePacket, which the assignments this replaced did + // not. Every path into here runs Clear() first so the chain is already empty in + // practice, but routing through ClearPackets keeps the invariant that no field can + // still reference a node once one has been parked on the free list. + ClearPackets(); _stateObj = stateObj; _replayStateData ??= new StateObjectData(); @@ -5257,10 +5319,64 @@ internal void Clear() private void ClearPackets() { + PacketData current = _firstPacket; + _firstPacket = null; _lastPacket = null; _continuePacket = null; _current = null; + + // Every field that can reference a node is cleared above before any node is parked, + // so nothing on the free list can also be reachable from the live chain. + while (current != null) + { + PacketData next = current.NextPacket; + if (!ReturnPacket(current)) + { + break; + } + current = next; + } + } + + /// + /// Places a on the free list when there is room for it, clearing + /// every field first so a recycled node cannot carry stale state, or a reference to a + /// packet buffer, into its next use. Returns false once the list is full, in which case + /// the node is left to the garbage collector. + /// + private bool ReturnPacket(PacketData packet) + { + if (_sparePacketCount >= MaxSparePacketCount) + { + return false; + } + + // Reset must run before the node is linked in: NextPacket is reused as the free + // list link, so clearing it afterwards would truncate the list. + packet.Reset(); + packet.NextPacket = _sparePackets; + _sparePackets = packet; + _sparePacketCount++; + return true; + } + + /// + /// Takes a from the free list, or allocates one when the list is empty. + /// + private PacketData RentPacket(byte[] buffer, int read) + { + PacketData packetData = _sparePackets; + if (packetData == null) + { + return new PacketData(buffer, read); + } + + // Read the link before Initialize clears NextPacket. + _sparePackets = packetData.NextPacket; + _sparePacketCount--; + packetData.Initialize(buffer, read); + return packetData; } private void ClearState() diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/StateSnapshotPacketRecyclingTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/StateSnapshotPacketRecyclingTests.cs new file mode 100644 index 0000000000..54169dce1e --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/StateSnapshotPacketRecyclingTests.cs @@ -0,0 +1,366 @@ +// 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 System.Reflection; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests +{ + /// + /// Covers the packet node free list. + /// + /// A snapshot is taken and released for every async continuation, so the nodes describing the + /// captured packets are recycled rather than reallocated. Recycling is only safe if a node + /// carries nothing from its previous use; an incomplete reset of RunningDataSize is what + /// produced the incorrect offset/length calculations in dotnet/SqlClient#3519. + /// + /// The nodes and the list are private by design, so these tests reach them by reflection. + /// + public sealed class StateSnapshotPacketRecyclingTests + { + // Must exceed StateSnapshot.MaxSparePacketCount so the bound itself is exercised. + private const int MaxSparePacketCount = 16; + private const int PacketsPerSnapshot = 20; + + private const int HeaderLength = 8; + private const int HeaderLengthFieldOffset = 2; + + #region Tests + + /// + /// Regression guard for dotnet/SqlClient#3519. A node returning from the free list must have + /// RunningDataSize cleared, otherwise the first packet of a new snapshot reports a + /// running total inherited from the previous snapshot and every offset derived from it is + /// wrong. + /// + [Fact] + public void RecycledNodes_DoNotCarryRunningDataSizeFromPreviousSnapshot() + { + SnapshotAccessor snapshot = SnapshotAccessor.Create(); + int[] sizes = { 40, 55, 70, 25 }; + + // First snapshot: append packets and accumulate a running total on each node. + AppendPackets(snapshot, sizes); + AssignRunningDataSizes(snapshot, sizes); + + List firstRound = snapshot.LivePackets(); + Assert.Equal(sizes.Length, firstRound.Count); + // Sanity check that the running totals really were populated, so that finding zeroes + // after recycling is meaningful. + Assert.Equal(sizes.Sum(), SnapshotAccessor.GetRunningDataSize(firstRound[firstRound.Count - 1])); + + snapshot.ClearPackets(); + + // Second snapshot: the nodes are served from the free list. + AppendPackets(snapshot, sizes); + List secondRound = snapshot.LivePackets(); + + Assert.Equal(sizes.Length, secondRound.Count); + Assert.NotEmpty(secondRound.Intersect(firstRound, ReferenceComparer.Instance)); + + // The defect behind #3519: a recycled node kept its running total, so a freshly + // appended packet claimed data that belonged to the previous snapshot. + foreach (object packet in secondRound) + { + Assert.Equal(0, SnapshotAccessor.GetRunningDataSize(packet)); + } + + // With the totals cleared, the derived offsets and lengths are correct again. + AssignRunningDataSizes(snapshot, sizes); + + int expectedOffset = 0; + for (int i = 0; i < sizes.Length; i++) + { + Assert.Equal(expectedOffset, SnapshotAccessor.GetPacketDataOffset(secondRound[i])); + Assert.Equal(sizes[i], SnapshotAccessor.GetPacketDataSize(secondRound[i])); + expectedOffset += sizes[i]; + } + } + + /// + /// A parked node must hold no packet state at all, in particular no reference to the packet + /// buffer, which the snapshot does not own. + /// + [Fact] + public void ParkedNodes_ReleaseAllPacketState() + { + SnapshotAccessor snapshot = SnapshotAccessor.Create(); + + AppendPackets(snapshot, UniformSizes(PacketsPerSnapshot, 64)); + AssignRunningDataSizes(snapshot, UniformSizes(PacketsPerSnapshot, 64)); + snapshot.ClearPackets(); + + List spares = snapshot.SparePackets(); + Assert.NotEmpty(spares); + + foreach (object packet in spares) + { + Assert.Null(SnapshotAccessor.GetBuffer(packet)); + Assert.Equal(0, SnapshotAccessor.GetRead(packet)); + Assert.Equal(0, SnapshotAccessor.GetRunningDataSize(packet)); + Assert.Null(SnapshotAccessor.GetPrevPacket(packet)); + } + } + + /// + /// The free list is bounded, and its tracked count must stay in step with its real length. + /// If the two drift, the list either stops recycling or grows without limit. + /// + [Fact] + public void FreeList_RespectsBoundAndTracksItsOwnCount() + { + SnapshotAccessor snapshot = SnapshotAccessor.Create(); + + for (int round = 0; round < 5; round++) + { + AppendPackets(snapshot, UniformSizes(PacketsPerSnapshot, 32)); + Assert.Equal(0, snapshot.SpareCount); + + snapshot.ClearPackets(); + + List spares = snapshot.SparePackets(); + Assert.Equal(MaxSparePacketCount, spares.Count); + Assert.Equal(spares.Count, snapshot.SpareCount); + Assert.Equal(spares.Count, spares.Distinct(ReferenceComparer.Instance).Count()); + } + } + + /// + /// The most dangerous failure mode for a free list: a node reachable from both the live + /// chain and the free list. Two snapshots would then share a node and silently overwrite + /// each other's packet descriptions. + /// + [Fact] + public void LiveChainAndFreeList_NeverShareANode() + { + SnapshotAccessor snapshot = SnapshotAccessor.Create(); + + for (int round = 0; round < 5; round++) + { + // Alternate between more and fewer packets than the bound so the list is both + // saturated and partially drained across rounds. + int count = round % 2 == 0 ? PacketsPerSnapshot : MaxSparePacketCount / 2; + AppendPackets(snapshot, UniformSizes(count, 48)); + + List live = snapshot.LivePackets(); + List spares = snapshot.SparePackets(); + + Assert.Equal(count, live.Count); + Assert.Empty(live.Intersect(spares, ReferenceComparer.Instance)); + Assert.Equal(live.Count, live.Distinct(ReferenceComparer.Instance).Count()); + + snapshot.ClearPackets(); + Assert.Empty(snapshot.LivePackets()); + } + } + + /// + /// Recycling must not damage the doubly linked chain: replay walks it forwards through + /// NextPacket and the offset maths walks it backwards through PrevPacket. + /// + [Fact] + public void RebuiltChain_HasIntactLinksAfterRecycling() + { + SnapshotAccessor snapshot = SnapshotAccessor.Create(); + + AppendPackets(snapshot, UniformSizes(PacketsPerSnapshot, 16)); + snapshot.ClearPackets(); + AppendPackets(snapshot, UniformSizes(PacketsPerSnapshot, 16)); + + List live = snapshot.LivePackets(); + Assert.Equal(PacketsPerSnapshot, live.Count); + + Assert.Null(SnapshotAccessor.GetPrevPacket(live[0])); + Assert.Null(SnapshotAccessor.GetNextPacket(live[live.Count - 1])); + Assert.Same(live[live.Count - 1], snapshot.LastPacket); + + for (int i = 0; i < live.Count - 1; i++) + { + Assert.Same(live[i + 1], SnapshotAccessor.GetNextPacket(live[i])); + Assert.Same(live[i], SnapshotAccessor.GetPrevPacket(live[i + 1])); + } + } + + #endregion + + #region Helpers + + private static int[] UniformSizes(int count, int dataLength) => + Enumerable.Repeat(dataLength, count).ToArray(); + + private static void AppendPackets(SnapshotAccessor snapshot, int[] dataLengths) + { + for (int i = 0; i < dataLengths.Length; i++) + { + // Each packet gets its own buffer: the snapshot permits several packets to share a + // buffer only for partial reads, which is not what these tests cover. + snapshot.AppendPacketData(CreatePacket(dataLengths[i], (byte)(i + 1)), HeaderLength + dataLengths[i]); + } + } + + /// + /// Walks the live chain and populates the running totals the way the parser does while + /// reading columns, so the offset calculations have real data to work from. + /// + private static void AssignRunningDataSizes(SnapshotAccessor snapshot, int[] sizes) + { + List live = snapshot.LivePackets(); + for (int i = 0; i < sizes.Length && i < live.Count; i++) + { + snapshot.SetCurrent(live[i]); + snapshot.SetPacketDataSize(sizes[i]); + } + snapshot.SetCurrent(null); + } + + /// + /// Builds a well formed TDS packet. The snapshot asserts in DEBUG that the length in the + /// header matches the number of bytes read, so the header cannot be left blank. + /// + private static byte[] CreatePacket(int dataLength, byte packetId) + { + int total = HeaderLength + dataLength; + byte[] buffer = new byte[total]; + + buffer[0] = 4; // MT_TOKENS + buffer[1] = 1; // ST_EOM + buffer[HeaderLengthFieldOffset] = (byte)(total >> 8); // length is big endian and + buffer[HeaderLengthFieldOffset + 1] = (byte)(total & 0xFF); // includes the header + buffer[6] = packetId; + + for (int i = HeaderLength; i < total; i++) + { + buffer[i] = (byte)(packetId + i); + } + + return buffer; + } + + private sealed class ReferenceComparer : IEqualityComparer + { + internal static readonly ReferenceComparer Instance = new(); + + public new bool Equals(object? x, object? y) => ReferenceEquals(x, y); + + public int GetHashCode(object obj) => + System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(obj); + } + + /// + /// Reflection wrapper over the private members of + /// and its nested packet node type. + /// + private sealed class SnapshotAccessor + { + private const BindingFlags Instance = BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public; + + private static readonly Type s_snapshotType = typeof(TdsParserStateObject.StateSnapshot); + private static readonly Type s_packetType = NestedType(s_snapshotType, "PacketData"); + + private static readonly FieldInfo s_firstPacket = Field(s_snapshotType, "_firstPacket"); + private static readonly FieldInfo s_lastPacket = Field(s_snapshotType, "_lastPacket"); + private static readonly FieldInfo s_current = Field(s_snapshotType, "_current"); + private static readonly FieldInfo s_sparePackets = Field(s_snapshotType, "_sparePackets"); + private static readonly FieldInfo s_spareCount = Field(s_snapshotType, "_sparePacketCount"); + private static readonly FieldInfo s_stateObj = Field(s_snapshotType, "_stateObj"); + private static readonly MethodInfo s_clearPackets = Method(s_snapshotType, "ClearPackets"); + + private static readonly FieldInfo s_buffer = Field(s_packetType, "Buffer"); + private static readonly FieldInfo s_read = Field(s_packetType, "Read"); + private static readonly FieldInfo s_next = Field(s_packetType, "NextPacket"); + private static readonly FieldInfo s_prev = Field(s_packetType, "PrevPacket"); + private static readonly FieldInfo s_runningDataSize = Field(s_packetType, "RunningDataSize"); + private static readonly MethodInfo s_getOffset = Method(s_packetType, "GetPacketDataOffset"); + private static readonly MethodInfo s_getSize = Method(s_packetType, "GetPacketDataSize"); + + // These members are private implementation detail, so a rename would silently turn the + // tests into no-ops. Fail loudly instead of returning null. + private static Type NestedType(Type owner, string name) => + owner.GetNestedType(name, BindingFlags.NonPublic) + ?? throw new InvalidOperationException($"{owner.Name} no longer declares a nested type '{name}'."); + + private static FieldInfo Field(Type owner, string name) => + owner.GetField(name, Instance) + ?? throw new InvalidOperationException($"{owner.Name} no longer declares a field '{name}'."); + + private static MethodInfo Method(Type owner, string name) => + owner.GetMethod(name, Instance) + ?? throw new InvalidOperationException($"{owner.Name} no longer declares a method '{name}'."); + + private readonly TdsParserStateObject.StateSnapshot _snapshot; + + private SnapshotAccessor(TdsParserStateObject.StateSnapshot snapshot) => _snapshot = snapshot; + + internal static SnapshotAccessor Create() + { + TdsParserStateObject.StateSnapshot snapshot = new(); + + // In DEBUG the snapshot records the owning state object's last stack trace on each + // appended packet. A real state object needs a live connection, so supply an + // allocated but unconstructed instance: only the null valued _lastStack is read. + Type concrete = typeof(TdsParserStateObject).Assembly + .GetTypes() + .First(t => !t.IsAbstract && typeof(TdsParserStateObject).IsAssignableFrom(t)); +#if NETFRAMEWORK + object stateObj = System.Runtime.Serialization.FormatterServices.GetUninitializedObject(concrete); +#else + object stateObj = System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject(concrete); +#endif + s_stateObj.SetValue(snapshot, stateObj); + + return new SnapshotAccessor(snapshot); + } + + internal int SpareCount => (int)s_spareCount.GetValue(_snapshot)!; + + internal object? LastPacket => s_lastPacket.GetValue(_snapshot); + + internal void AppendPacketData(byte[] buffer, int read) => _snapshot.AppendPacketData(buffer, read); + + internal void SetPacketDataSize(int size) => _snapshot.SetPacketDataSize(size); + + internal void SetCurrent(object? packet) => s_current.SetValue(_snapshot, packet); + + internal void ClearPackets() => s_clearPackets.Invoke(_snapshot, null); + + internal List LivePackets() => Walk(s_firstPacket.GetValue(_snapshot)); + + internal List SparePackets() => Walk(s_sparePackets.GetValue(_snapshot)); + + private static List Walk(object? head) + { + List packets = new(); + object? current = head; + while (current != null) + { + packets.Add(current); + current = GetNextPacket(current); + + // A cycle would otherwise hang the test run rather than fail it. + Assert.True(packets.Count <= 1024, "packet chain does not terminate"); + } + return packets; + } + + internal static byte[]? GetBuffer(object packet) => (byte[]?)s_buffer.GetValue(packet); + + internal static int GetRead(object packet) => (int)s_read.GetValue(packet)!; + + internal static object? GetNextPacket(object packet) => s_next.GetValue(packet); + + internal static object? GetPrevPacket(object packet) => s_prev.GetValue(packet); + + internal static int GetRunningDataSize(object packet) => (int)s_runningDataSize.GetValue(packet)!; + + internal static int GetPacketDataOffset(object packet) => (int)s_getOffset.Invoke(packet, null)!; + + internal static int GetPacketDataSize(object packet) => (int)s_getSize.Invoke(packet, null)!; + } + + #endregion + } +}