From cc0714f7da10477dc3d4346728eade76990b9c10 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 10 Jul 2025 17:50:47 +0100 Subject: [PATCH 1/3] add clearing of data sizes when a multi-part result is returned --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 11 ++++-- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 11 ++++-- .../Data/SqlClient/TdsParserStateObject.cs | 36 +++++++++++++++---- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index b5febf7add..89ebb9a499 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13405,6 +13405,13 @@ bool writeDataSizeToSnapshot break; } } + + if (writeDataSizeToSnapshot) + { + stateObj.SetSnapshotStorage(null); + stateObj.ClearSnapshotDataSize(); + } + return TdsOperationStatus.Done; static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value) @@ -13424,7 +13431,7 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti current = 0; } - stateObj.SetSnapshotDataSize(current + value); + stateObj.AddSnapshotDataSize(current + value); // return new packetid so next time we see this packet we know it isn't new return currentPacketId; @@ -13432,7 +13439,7 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti else { current = stateObj.GetSnapshotDataSize(); - stateObj.SetSnapshotDataSize(current + value); + stateObj.AddSnapshotDataSize(current + value); return previousPacketId; } } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs index 3acd517f73..cb6acf62be 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13596,6 +13596,13 @@ bool writeDataSizeToSnapshot break; } } + + if (writeDataSizeToSnapshot) + { + stateObj.SetSnapshotStorage(null); + stateObj.ClearSnapshotDataSize(); + } + return TdsOperationStatus.Done; static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value) @@ -13615,7 +13622,7 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti current = 0; } - stateObj.SetSnapshotDataSize(current + value); + stateObj.AddSnapshotDataSize(current + value); // return new packetid so next time we see this packet we know it isn't new return currentPacketId; @@ -13623,7 +13630,7 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti else { current = stateObj.GetSnapshotDataSize(); - stateObj.SetSnapshotDataSize(current + value); + stateObj.AddSnapshotDataSize(current + value); return previousPacketId; } } 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 781f360991..805b30c794 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -1512,7 +1512,7 @@ public TdsOperationStatus TryReadByteArray(Span buff, int len, out int tot if (writeDataSizeToSnapshot) { - SetSnapshotDataSize(bytesToRead); + AddSnapshotDataSize(bytesToRead); } AssertValidState(); @@ -2058,7 +2058,7 @@ internal TdsOperationStatus TryReadPlpLength(bool returnPlpNullIfNull, out ulong // bool firstchunk = false; bool isNull = false; - Debug.Assert(_longlenleft == 0, "Out of synch length read request"); + Debug.Assert(_longlenleft == 0, "Out of sync length read request"); if (_longlen == 0) { // First chunk is being read. Find out what type of chunk it is @@ -2139,6 +2139,7 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len } return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, canContinue, canContinue, compatibilityMode); } + // Reads the requested number of bytes from a plp data stream, or the entire data if // requested length is -1 or larger than the actual length of data. First call to this method // should be preceeded by a call to ReadPlpLength or ReadDataLength. @@ -2254,14 +2255,14 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len SetSnapshotStorage(buff); if (writeDataSizeToSnapshot) { - SetSnapshotDataSize(bytesRead); + AddSnapshotDataSize(bytesRead); } } return result; } if (writeDataSizeToSnapshot && canContinue) { - SetSnapshotDataSize(bytesRead); + AddSnapshotDataSize(bytesRead); } if (_longlenleft == 0) @@ -2281,7 +2282,8 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len SetSnapshotStorage(buff); if (writeDataSizeToSnapshot) { - SetSnapshotDataSize(bytesRead); + // data bytes read from the current packet must be 0 here so do not save the snapshot data size + //SetSnapshotDataSize(bytesRead); } } return result; @@ -2296,6 +2298,12 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len break; } } + + if (canContinue) + { + SetSnapshotStorage(null); + ClearSnapshotDataSize(); + } return TdsOperationStatus.Done; } @@ -3605,12 +3613,18 @@ internal void SetSnapshotStorage(object buffer) /// countOfBytesCopiedFromCurrentPacket to be calculated /// /// - internal void SetSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket) + internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket) { Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to store packet data size"); _snapshot.SetPacketDataSize(countOfBytesCopiedFromCurrentPacket); } + internal void ClearSnapshotDataSize() + { + Debug.Assert(_snapshot != null, "_snapshot must exist to store packet data size"); + _snapshot?.ClearPacketDataSize(); + } + internal int GetSnapshotTotalSize() { Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to read total size"); @@ -4307,6 +4321,16 @@ internal void SetPacketDataSize(int size) target.RunningDataSize = total + size; } + internal void ClearPacketDataSize() + { + PacketData current = _firstPacket; + while (current != null) + { + current.RunningDataSize = 0; + current = current.NextPacket; + } + } + internal int GetPacketDataOffset() { int offset = 0; From e6e7b464a1c9b752802d782c270d2e42ef4f4dab Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 10 Jul 2025 17:56:43 +0100 Subject: [PATCH 2/3] remove asserts detecting overwrite of snapshot storage which are now no longer accurate --- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 9 --------- 1 file changed, 9 deletions(-) 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 805b30c794..03d001a4c1 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -3539,9 +3539,6 @@ internal void ResetSnapshot() { StateSnapshot snapshot = _snapshot; _snapshot = null; - // TODO(GH-3385): Not sure what this is trying to assert, but it - // currently fails the DataReader tests. - // Debug.Assert(snapshot._storage == null); snapshot.Clear(); Interlocked.CompareExchange(ref _cachedSnapshot, snapshot, null); } @@ -3599,9 +3596,6 @@ internal object TryTakeSnapshotStorage() internal void SetSnapshotStorage(object buffer) { Debug.Assert(_snapshot != null, "should not access snapshot accessor functions without first checking that the snapshot is available"); - // TODO(GH-3385): Not sure what this is trying to assert, but it - // currently fails the DataReader tests. - // Debug.Assert(_snapshot._storage == null, "should not overwrite snapshot stored buffer"); if (_snapshot != null) { _snapshot._storage = buffer; @@ -4380,9 +4374,6 @@ private void ClearPackets() private void ClearState() { - // TODO(GH-3385): Not sure what this is trying to assert, but it - // currently fails the DataReader tests. - // Debug.Assert(_storage == null); _storage = null; _replayStateData.Clear(_stateObj); _continueStateData?.Clear(_stateObj, trackStack: false); From 5c73ac7a87056051f0988b68ee33dc83a45e17d1 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 11 Jul 2025 21:05:43 +0100 Subject: [PATCH 3/3] remove empty statement block --- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) 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 03d001a4c1..3c768f4565 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -2280,11 +2280,7 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len else if (canContinue && result == TdsOperationStatus.NeedMoreData) { SetSnapshotStorage(buff); - if (writeDataSizeToSnapshot) - { - // data bytes read from the current packet must be 0 here so do not save the snapshot data size - //SetSnapshotDataSize(bytesRead); - } + // data bytes read from the current packet must be 0 here so do not save the snapshot data size } return result; }