Revert "Fix TryReadPlpBytes throws ArgumentException (#3470) (#3474)"

samsharma2700 · samsharma2700 · commit 8c301016d57a · 2025-08-05T14:24:27.000-07:00
This reverts commit 0a55896.
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
@@ -13359,13 +13359,6 @@ bool writeDataSizeToSnapshot
                     break;
                 }
             }
-
-            if (writeDataSizeToSnapshot)
-            {
-                stateObj.SetSnapshotStorage(null);
-                stateObj.ClearSnapshotDataSize();
-            }
-
             return TdsOperationStatus.Done;
 
             static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)
@@ -13385,15 +13378,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti
                         current = 0;
                     }
 
-                    stateObj.AddSnapshotDataSize(current + value);
+                    stateObj.SetSnapshotDataSize(current + value);
 
                     // return new packetid so next time we see this packet we know it isn't new
                     return currentPacketId;
                 }
                 else
                 {
                     current = stateObj.GetSnapshotDataSize();
-                    stateObj.AddSnapshotDataSize(current + value);
+                    stateObj.SetSnapshotDataSize(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
@@ -13541,13 +13541,6 @@ bool writeDataSizeToSnapshot
                     break;
                 }
             }
-
-            if (writeDataSizeToSnapshot)
-            {
-                stateObj.SetSnapshotStorage(null);
-                stateObj.ClearSnapshotDataSize();
-            }
-
             return TdsOperationStatus.Done;
 
             static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)
@@ -13567,15 +13560,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti
                         current = 0;
                     }
 
-                    stateObj.AddSnapshotDataSize(current + value);
+                    stateObj.SetSnapshotDataSize(current + value);
 
                     // return new packetid so next time we see this packet we know it isn't new
                     return currentPacketId;
                 }
                 else
                 {
                     current = stateObj.GetSnapshotDataSize();
-                    stateObj.AddSnapshotDataSize(current + value);
+                    stateObj.SetSnapshotDataSize(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
@@ -1510,7 +1510,7 @@ public TdsOperationStatus TryReadByteArray(Span<byte> buff, int len, out int tot
 
                 if (writeDataSizeToSnapshot)
                 {
-                    AddSnapshotDataSize(bytesToRead);
+                    SetSnapshotDataSize(bytesToRead);
                 }
 
                 AssertValidState();
@@ -2056,7 +2056,7 @@ internal TdsOperationStatus TryReadPlpLength(bool returnPlpNullIfNull, out ulong
             // bool firstchunk = false;
             bool isNull = false;
 
-            Debug.Assert(_longlenleft == 0, "Out of sync length read request");
+            Debug.Assert(_longlenleft == 0, "Out of synch length read request");
             if (_longlen == 0)
             {
                 // First chunk is being read. Find out what type of chunk it is
@@ -2137,7 +2137,6 @@ 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.
@@ -2253,14 +2252,14 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
                         SetSnapshotStorage(buff);
                         if (writeDataSizeToSnapshot)
                         {
-                            AddSnapshotDataSize(bytesRead);
+                            SetSnapshotDataSize(bytesRead);
                         }
                     }
                     return result;
                 }
                 if (writeDataSizeToSnapshot && canContinue)
                 {
-                    AddSnapshotDataSize(bytesRead);
+                    SetSnapshotDataSize(bytesRead);
                 }
 
                 if (_longlenleft == 0)
@@ -2278,7 +2277,10 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
                         else if (canContinue && result == TdsOperationStatus.NeedMoreData)
                         {
                             SetSnapshotStorage(buff);
-                            // data bytes read from the current packet must be 0 here so do not save the snapshot data size
+                            if (writeDataSizeToSnapshot)
+                            {
+                                SetSnapshotDataSize(bytesRead);
+                            }
                         }
                         return result;
                     }
@@ -2292,12 +2294,6 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
                     break;
                 }
             }
-
-            if (canContinue)
-            {
-                SetSnapshotStorage(null);
-                ClearSnapshotDataSize();
-            }
             return TdsOperationStatus.Done;
         }
 
@@ -3533,6 +3529,9 @@ 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);
             }
@@ -3590,6 +3589,9 @@ 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;
@@ -3601,18 +3603,12 @@ internal void SetSnapshotStorage(object buffer)
         /// countOfBytesCopiedFromCurrentPacket to be calculated
         /// </summary>
         /// <param name="countOfBytesCopiedFromCurrentPacket"></param>
-        internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)
+        internal void SetSnapshotDataSize(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");
@@ -4309,16 +4305,6 @@ 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;
@@ -4368,6 +4354,9 @@ 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);

Original file line number	Diff line number	Diff line change
`@@ -13359,13 +13359,6 @@ bool writeDataSizeToSnapshot`
`13359`	`13359`	`break;`
`13360`	`13360`	`}`
`13361`	`13361`	`}`
`13362`		`-`
`13363`		`- if (writeDataSizeToSnapshot)`
`13364`		`- {`
`13365`		`- stateObj.SetSnapshotStorage(null);`
`13366`		`- stateObj.ClearSnapshotDataSize();`
`13367`		`- }`
`13368`		`-`
`13369`	`13362`	`return TdsOperationStatus.Done;`
`13370`	`13363`
`13371`	`13364`	`static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)`
`@@ -13385,15 +13378,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti`
`13385`	`13378`	`current = 0;`
`13386`	`13379`	`}`
`13387`	`13380`
`13388`		`- stateObj.AddSnapshotDataSize(current + value);`
	`13381`	`+ stateObj.SetSnapshotDataSize(current + value);`
`13389`	`13382`
`13390`	`13383`	`// return new packetid so next time we see this packet we know it isn't new`
`13391`	`13384`	`return currentPacketId;`
`13392`	`13385`	`}`
`13393`	`13386`	`else`
`13394`	`13387`	`{`
`13395`	`13388`	`current = stateObj.GetSnapshotDataSize();`
`13396`		`- stateObj.AddSnapshotDataSize(current + value);`
	`13389`	`+ stateObj.SetSnapshotDataSize(current + value);`
`13397`	`13390`	`return previousPacketId;`
`13398`	`13391`	`}`
`13399`	`13392`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13541,13 +13541,6 @@ bool writeDataSizeToSnapshot`
`13541`	`13541`	`break;`
`13542`	`13542`	`}`
`13543`	`13543`	`}`
`13544`		`-`
`13545`		`- if (writeDataSizeToSnapshot)`
`13546`		`- {`
`13547`		`- stateObj.SetSnapshotStorage(null);`
`13548`		`- stateObj.ClearSnapshotDataSize();`
`13549`		`- }`
`13550`		`-`
`13551`	`13544`	`return TdsOperationStatus.Done;`
`13552`	`13545`
`13553`	`13546`	`static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)`
`@@ -13567,15 +13560,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti`
`13567`	`13560`	`current = 0;`
`13568`	`13561`	`}`
`13569`	`13562`
`13570`		`- stateObj.AddSnapshotDataSize(current + value);`
	`13563`	`+ stateObj.SetSnapshotDataSize(current + value);`
`13571`	`13564`
`13572`	`13565`	`// return new packetid so next time we see this packet we know it isn't new`
`13573`	`13566`	`return currentPacketId;`
`13574`	`13567`	`}`
`13575`	`13568`	`else`
`13576`	`13569`	`{`
`13577`	`13570`	`current = stateObj.GetSnapshotDataSize();`
`13578`		`- stateObj.AddSnapshotDataSize(current + value);`
	`13571`	`+ stateObj.SetSnapshotDataSize(current + value);`
`13579`	`13572`	`return previousPacketId;`
`13580`	`13573`	`}`
`13581`	`13574`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1510,7 +1510,7 @@ public TdsOperationStatus TryReadByteArray(Span<byte> buff, int len, out int tot`
`1510`	`1510`
`1511`	`1511`	`if (writeDataSizeToSnapshot)`
`1512`	`1512`	`{`
`1513`		`- AddSnapshotDataSize(bytesToRead);`
	`1513`	`+ SetSnapshotDataSize(bytesToRead);`
`1514`	`1514`	`}`
`1515`	`1515`
`1516`	`1516`	`AssertValidState();`
`@@ -2056,7 +2056,7 @@ internal TdsOperationStatus TryReadPlpLength(bool returnPlpNullIfNull, out ulong`
`2056`	`2056`	`// bool firstchunk = false;`
`2057`	`2057`	`bool isNull = false;`
`2058`	`2058`
`2059`		`- Debug.Assert(_longlenleft == 0, "Out of sync length read request");`
	`2059`	`+ Debug.Assert(_longlenleft == 0, "Out of synch length read request");`
`2060`	`2060`	`if (_longlen == 0)`
`2061`	`2061`	`{`
`2062`	`2062`	`// First chunk is being read. Find out what type of chunk it is`
`@@ -2137,7 +2137,6 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2137`	`2137`	`}`
`2138`	`2138`	`return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, canContinue, canContinue, compatibilityMode);`
`2139`	`2139`	`}`
`2140`		`-`
`2141`	`2140`	`// Reads the requested number of bytes from a plp data stream, or the entire data if`
`2142`	`2141`	`// requested length is -1 or larger than the actual length of data. First call to this method`
`2143`	`2142`	`// should be preceeded by a call to ReadPlpLength or ReadDataLength.`
`@@ -2253,14 +2252,14 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2253`	`2252`	`SetSnapshotStorage(buff);`
`2254`	`2253`	`if (writeDataSizeToSnapshot)`
`2255`	`2254`	`{`
`2256`		`- AddSnapshotDataSize(bytesRead);`
	`2255`	`+ SetSnapshotDataSize(bytesRead);`
`2257`	`2256`	`}`
`2258`	`2257`	`}`
`2259`	`2258`	`return result;`
`2260`	`2259`	`}`
`2261`	`2260`	`if (writeDataSizeToSnapshot && canContinue)`
`2262`	`2261`	`{`
`2263`		`- AddSnapshotDataSize(bytesRead);`
	`2262`	`+ SetSnapshotDataSize(bytesRead);`
`2264`	`2263`	`}`
`2265`	`2264`
`2266`	`2265`	`if (_longlenleft == 0)`
`@@ -2278,7 +2277,10 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2278`	`2277`	`else if (canContinue && result == TdsOperationStatus.NeedMoreData)`
`2279`	`2278`	`{`
`2280`	`2279`	`SetSnapshotStorage(buff);`
`2281`		`- // data bytes read from the current packet must be 0 here so do not save the snapshot data size`
	`2280`	`+ if (writeDataSizeToSnapshot)`
	`2281`	`+ {`
	`2282`	`+ SetSnapshotDataSize(bytesRead);`
	`2283`	`+ }`
`2282`	`2284`	`}`
`2283`	`2285`	`return result;`
`2284`	`2286`	`}`
`@@ -2292,12 +2294,6 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2292`	`2294`	`break;`
`2293`	`2295`	`}`
`2294`	`2296`	`}`
`2295`		`-`
`2296`		`- if (canContinue)`
`2297`		`- {`
`2298`		`- SetSnapshotStorage(null);`
`2299`		`- ClearSnapshotDataSize();`
`2300`		`- }`
`2301`	`2297`	`return TdsOperationStatus.Done;`
`2302`	`2298`	`}`
`2303`	`2299`
`@@ -3533,6 +3529,9 @@ internal void ResetSnapshot()`
`3533`	`3529`	`{`
`3534`	`3530`	`StateSnapshot snapshot = _snapshot;`
`3535`	`3531`	`_snapshot = null;`
	`3532`	`+ // TODO(GH-3385): Not sure what this is trying to assert, but it`
	`3533`	`+ // currently fails the DataReader tests.`
	`3534`	`+ // Debug.Assert(snapshot._storage == null);`
`3536`	`3535`	`snapshot.Clear();`
`3537`	`3536`	`Interlocked.CompareExchange(ref _cachedSnapshot, snapshot, null);`
`3538`	`3537`	`}`
`@@ -3590,6 +3589,9 @@ internal object TryTakeSnapshotStorage()`
`3590`	`3589`	`internal void SetSnapshotStorage(object buffer)`
`3591`	`3590`	`{`
`3592`	`3591`	`Debug.Assert(_snapshot != null, "should not access snapshot accessor functions without first checking that the snapshot is available");`
	`3592`	`+ // TODO(GH-3385): Not sure what this is trying to assert, but it`
	`3593`	`+ // currently fails the DataReader tests.`
	`3594`	`+ // Debug.Assert(_snapshot._storage == null, "should not overwrite snapshot stored buffer");`
`3593`	`3595`	`if (_snapshot != null)`
`3594`	`3596`	`{`
`3595`	`3597`	`_snapshot._storage = buffer;`
`@@ -3601,18 +3603,12 @@ internal void SetSnapshotStorage(object buffer)`
`3601`	`3603`	`/// countOfBytesCopiedFromCurrentPacket to be calculated`
`3602`	`3604`	`/// </summary>`
`3603`	`3605`	`/// <param name="countOfBytesCopiedFromCurrentPacket"></param>`
`3604`		`- internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)`
	`3606`	`+ internal void SetSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)`
`3605`	`3607`	`{`
`3606`	`3608`	`Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to store packet data size");`
`3607`	`3609`	`_snapshot.SetPacketDataSize(countOfBytesCopiedFromCurrentPacket);`
`3608`	`3610`	`}`
`3609`	`3611`
`3610`		`- internal void ClearSnapshotDataSize()`
`3611`		`- {`
`3612`		`- Debug.Assert(_snapshot != null, "_snapshot must exist to store packet data size");`
`3613`		`- _snapshot?.ClearPacketDataSize();`
`3614`		`- }`
`3615`		`-`
`3616`	`3612`	`internal int GetSnapshotTotalSize()`
`3617`	`3613`	`{`
`3618`	`3614`	`Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to read total size");`
`@@ -4309,16 +4305,6 @@ internal void SetPacketDataSize(int size)`
`4309`	`4305`	`target.RunningDataSize = total + size;`
`4310`	`4306`	`}`
`4311`	`4307`
`4312`		`- internal void ClearPacketDataSize()`
`4313`		`- {`
`4314`		`- PacketData current = _firstPacket;`
`4315`		`- while (current != null)`
`4316`		`- {`
`4317`		`- current.RunningDataSize = 0;`
`4318`		`- current = current.NextPacket;`
`4319`		`- }`
`4320`		`- }`
`4321`		`-`
`4322`	`4308`	`internal int GetPacketDataOffset()`
`4323`	`4309`	`{`
`4324`	`4310`	`int offset = 0;`
`@@ -4368,6 +4354,9 @@ private void ClearPackets()`
`4368`	`4354`
`4369`	`4355`	`private void ClearState()`
`4370`	`4356`	`{`
	`4357`	`+ // TODO(GH-3385): Not sure what this is trying to assert, but it`
	`4358`	`+ // currently fails the DataReader tests.`
	`4359`	`+ // Debug.Assert(_storage == null);`
`4371`	`4360`	`_storage = null;`
`4372`	`4361`	`_replayStateData.Clear(_stateObj);`
`4373`	`4362`	`_continueStateData?.Clear(_stateObj, trackStack: false);`