Skip to content

Commit a2863dd

Browse files
committed
Revert "Refine handling of moving between replay and continue states (#3337)"
This reverts commit 265f522.
1 parent 5b31219 commit a2863dd

File tree

5 files changed

+47
-111
lines changed

5 files changed

+47
-111
lines changed

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6549,8 +6549,8 @@ private TdsOperationStatus TryReadByteArrayWithContinue(TdsParserStateObject sta
65496549
bytes = null;
65506550
int offset = 0;
65516551
byte[] temp = null;
6552-
(bool canContinue, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
6553-
if (canContinue)
6552+
(bool isAvailable, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
6553+
if (isAvailable)
65546554
{
65556555
if (isContinuing || isStarting)
65566556
{
@@ -13126,9 +13126,9 @@ internal TdsOperationStatus TryReadPlpUnicodeCharsWithContinue(TdsParserStateObj
1312613126
char[] temp = null;
1312713127
bool buffIsRented = false;
1312813128
int startOffset = 0;
13129-
(bool canContinue, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
13129+
(bool isAvailable, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
1313013130

13131-
if (canContinue)
13131+
if (isAvailable)
1313213132
{
1313313133
if (isContinuing || isStarting)
1313413134
{
@@ -13147,7 +13147,7 @@ internal TdsOperationStatus TryReadPlpUnicodeCharsWithContinue(TdsParserStateObj
1314713147
length >> 1,
1314813148
stateObj,
1314913149
out length,
13150-
supportRentedBuff: !canContinue, // do not use the arraypool if we are going to keep the buffer in the snapshot
13150+
supportRentedBuff: !isAvailable, // do not use the arraypool if we are going to keep the buffer in the snapshot
1315113151
rentedBuff: ref buffIsRented,
1315213152
startOffset,
1315313153
isStarting || isContinuing

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6745,8 +6745,8 @@ private TdsOperationStatus TryReadByteArrayWithContinue(TdsParserStateObject sta
67456745
bytes = null;
67466746
int offset = 0;
67476747
byte[] temp = null;
6748-
(bool canContinue, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
6749-
if (canContinue)
6748+
(bool isAvailable, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
6749+
if (isAvailable)
67506750
{
67516751
if (isContinuing || isStarting)
67526752
{
@@ -13307,9 +13307,9 @@ internal TdsOperationStatus TryReadPlpUnicodeCharsWithContinue(TdsParserStateObj
1330713307
char[] temp = null;
1330813308
bool buffIsRented = false;
1330913309
int startOffset = 0;
13310-
(bool canContinue, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
13310+
(bool isAvailable, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
1331113311

13312-
if (canContinue)
13312+
if (isAvailable)
1331313313
{
1331413314
if (isContinuing || isStarting)
1331513315
{
@@ -13328,7 +13328,7 @@ internal TdsOperationStatus TryReadPlpUnicodeCharsWithContinue(TdsParserStateObj
1332813328
length >> 1,
1332913329
stateObj,
1333013330
out length,
13331-
supportRentedBuff: !canContinue, // do not use the arraypool if we are going to keep the buffer in the snapshot
13331+
supportRentedBuff: !isAvailable, // do not use the arraypool if we are going to keep the buffer in the snapshot
1333213332
rentedBuff: ref buffIsRented,
1333313333
startOffset,
1333413334
isStarting || isContinuing

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,10 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser
3939
{
4040
buffer = null;
4141

42-
(bool canContinue, bool isStarting, _) = stateObj.GetSnapshotStatuses();
42+
(bool isAvailable, bool isStarting, _) = stateObj.GetSnapshotStatuses();
4343

4444
List<byte[]> cachedBytes = null;
45-
if (canContinue)
45+
if (isAvailable)
4646
{
4747
cachedBytes = stateObj.TryTakeSnapshotStorage() as List<byte[]>;
4848
if (isStarting)
@@ -78,10 +78,10 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser
7878
byte[] byteArr = new byte[cb];
7979
// pass false for the writeDataSizeToSnapshot parameter because we want to only take data
8080
// from the current packet and not try to do a continue-capable multi packet read
81-
result = stateObj.TryReadPlpBytes(ref byteArr, 0, cb, out cb, canContinue, writeDataSizeToSnapshot: false, compatibilityMode: false);
81+
result = stateObj.TryReadPlpBytes(ref byteArr, 0, cb, out cb, writeDataSizeToSnapshot: false, compatibilityMode: false);
8282
if (result != TdsOperationStatus.Done)
8383
{
84-
if (result == TdsOperationStatus.NeedMoreData && canContinue && cb == byteArr.Length)
84+
if (result == TdsOperationStatus.NeedMoreData && isAvailable && cb == byteArr.Length)
8585
{
8686
// succeeded in getting the data but failed to find the next plp length
8787
returnAfterAdd = true;

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

Lines changed: 33 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1922,7 +1922,7 @@ internal TdsOperationStatus TryReadStringWithEncoding(int length, System.Text.En
19221922
}
19231923
byte[] buf = null;
19241924
int offset = 0;
1925-
(bool canContinue, bool isStarting, bool isContinuing) = GetSnapshotStatuses();
1925+
(bool isAvailable, bool isStarting, bool isContinuing) = GetSnapshotStatuses();
19261926

19271927
if (isPlp)
19281928
{
@@ -1940,7 +1940,7 @@ internal TdsOperationStatus TryReadStringWithEncoding(int length, System.Text.En
19401940
if (((_inBytesUsed + length) > _inBytesRead) || (_inBytesPacket < length))
19411941
{
19421942
int startOffset = 0;
1943-
if (canContinue)
1943+
if (isAvailable)
19441944
{
19451945
if (isContinuing || isStarting)
19461946
{
@@ -1958,7 +1958,7 @@ internal TdsOperationStatus TryReadStringWithEncoding(int length, System.Text.En
19581958
buf = new byte[length];
19591959
}
19601960

1961-
TdsOperationStatus result = TryReadByteArray(buf, length, out _, startOffset, canContinue);
1961+
TdsOperationStatus result = TryReadByteArray(buf, length, out _, startOffset, isAvailable);
19621962

19631963
if (result != TdsOperationStatus.Done)
19641964
{
@@ -2088,23 +2088,22 @@ internal int ReadPlpBytesChunk(byte[] buff, int offset, int len)
20882088

20892089
internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len, out int totalBytesRead)
20902090
{
2091-
bool canContinue = false;
20922091
bool isStarting = false;
20932092
bool isContinuing = false;
20942093
bool compatibilityMode = LocalAppContextSwitches.UseCompatibilityAsyncBehaviour;
20952094
if (!compatibilityMode)
20962095
{
2097-
(canContinue, isStarting, isContinuing) = GetSnapshotStatuses();
2096+
(_, isStarting, isContinuing) = GetSnapshotStatuses();
20982097
}
2099-
return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, canContinue, canContinue, compatibilityMode);
2098+
return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, isStarting || isContinuing, compatibilityMode);
21002099
}
21012100
// Reads the requested number of bytes from a plp data stream, or the entire data if
21022101
// requested length is -1 or larger than the actual length of data. First call to this method
21032102
// should be preceeded by a call to ReadPlpLength or ReadDataLength.
21042103
// Returns the actual bytes read.
21052104
// NOTE: This method must be retriable WITHOUT replaying a snapshot
21062105
// Every time you call this method increment the offset and decrease len by the value of totalBytesRead
2107-
internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len, out int totalBytesRead, bool canContinue, bool writeDataSizeToSnapshot, bool compatibilityMode)
2106+
internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len, out int totalBytesRead, bool writeDataSizeToSnapshot, bool compatibilityMode)
21082107
{
21092108
totalBytesRead = 0;
21102109

@@ -2129,16 +2128,9 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
21292128
// If total length is known up front, allocate the whole buffer in one shot instead of realloc'ing and copying over each time
21302129
if (buff == null && _longlen != TdsEnums.SQL_PLP_UNKNOWNLEN)
21312130
{
2132-
if (compatibilityMode && _snapshot != null && _snapshotStatus != SnapshotStatus.NotActive)
2133-
{
2134-
// legacy replay path perf optimization
2135-
// if there is a snapshot which contains a stored plp buffer take it
2136-
// and try to use it if it is the right length
2137-
buff = TryTakeSnapshotStorage() as byte[];
2138-
}
2139-
else if (writeDataSizeToSnapshot && canContinue && _snapshot != null)
2131+
if (writeDataSizeToSnapshot)
21402132
{
2141-
// if there is a snapshot which it contains a stored plp buffer take it
2133+
// if there is a snapshot and it contains a stored plp buffer take it
21422134
// and try to use it if it is the right length
21432135
buff = TryTakeSnapshotStorage() as byte[];
21442136
if (buff != null)
@@ -2147,7 +2139,13 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
21472139
totalBytesRead = offset;
21482140
}
21492141
}
2150-
2142+
else if (compatibilityMode && _snapshot != null && _snapshotStatus != SnapshotStatus.NotActive)
2143+
{
2144+
// legacy replay path perf optimization
2145+
// if there is a snapshot and it contains a stored plp buffer take it
2146+
// and try to use it if it is the right length
2147+
buff = TryTakeSnapshotStorage() as byte[];
2148+
}
21512149

21522150
if ((ulong)(buff?.Length ?? 0) != _longlen)
21532151
{
@@ -2199,49 +2197,43 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
21992197
_longlenleft -= (ulong)bytesRead;
22002198
if (result != TdsOperationStatus.Done)
22012199
{
2202-
if (compatibilityMode && _snapshot != null)
2200+
if (writeDataSizeToSnapshot)
22032201
{
2204-
// legacy replay path perf optimization
22052202
// a partial read has happened so store the target buffer in the snapshot
22062203
// so it can be re-used when another packet arrives and we read again
22072204
SetSnapshotStorage(buff);
2205+
SetSnapshotDataSize(bytesRead);
2206+
22082207
}
2209-
else if (canContinue)
2208+
else if (compatibilityMode && _snapshot != null)
22102209
{
2210+
// legacy replay path perf optimization
22112211
// a partial read has happened so store the target buffer in the snapshot
22122212
// so it can be re-used when another packet arrives and we read again
22132213
SetSnapshotStorage(buff);
2214-
if (writeDataSizeToSnapshot)
2215-
{
2216-
SetSnapshotDataSize(bytesRead);
2217-
}
22182214
}
22192215
return result;
22202216
}
2221-
if (writeDataSizeToSnapshot && canContinue)
2222-
{
2223-
SetSnapshotDataSize(bytesRead);
2224-
}
22252217

22262218
if (_longlenleft == 0)
22272219
{
22282220
// Read the next chunk or cleanup state if hit the end
22292221
result = TryReadPlpLength(false, out _);
22302222
if (result != TdsOperationStatus.Done)
22312223
{
2232-
if (compatibilityMode && _snapshot != null)
2233-
{
2234-
// a partial read has happened so store the target buffer in the snapshot
2235-
// so it can be re-used when another packet arrives and we read again
2236-
SetSnapshotStorage(buff);
2237-
}
2238-
else if (canContinue && result == TdsOperationStatus.NeedMoreData)
2224+
if (writeDataSizeToSnapshot)
22392225
{
2240-
SetSnapshotStorage(buff);
2241-
if (writeDataSizeToSnapshot)
2226+
if (result == TdsOperationStatus.NeedMoreData)
22422227
{
2228+
SetSnapshotStorage(buff);
22432229
SetSnapshotDataSize(bytesRead);
22442230
}
2231+
}
2232+
else if (compatibilityMode && _snapshot != null)
2233+
{
2234+
// a partial read has happened so store the target buffer in the snapshot
2235+
// so it can be re-used when another packet arrives and we read again
2236+
SetSnapshotStorage(buff);
22452237
}
22462238
return result;
22472239
}
@@ -3516,17 +3508,17 @@ internal bool IsSnapshotContinuing()
35163508
_snapshotStatus == TdsParserStateObject.SnapshotStatus.ContinueRunning;
35173509
}
35183510

3519-
internal (bool CanContinue, bool IsStarting, bool IsContinuing) GetSnapshotStatuses()
3511+
internal (bool IsAvailable, bool IsStarting, bool IsContinuing) GetSnapshotStatuses()
35203512
{
3521-
bool canContinue = _snapshot != null && _snapshot.ContinueEnabled && _snapshotStatus != SnapshotStatus.NotActive;
3513+
bool isAvailable = _snapshot != null && _snapshot.ContinueEnabled;
35223514
bool isStarting = false;
35233515
bool isContinuing = false;
3524-
if (canContinue)
3516+
if (isAvailable)
35253517
{
35263518
isStarting = _snapshotStatus == SnapshotStatus.ReplayStarting;
35273519
isContinuing = _snapshotStatus == SnapshotStatus.ContinueRunning;
35283520
}
3529-
return (canContinue, isStarting, isContinuing);
3521+
return (isAvailable, isStarting, isContinuing);
35303522
}
35313523

35323524
internal int GetSnapshotStorageLength<T>()

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@
66
using System.Collections.Generic;
77
using System.Data;
88
using System.Data.SqlTypes;
9-
using System.Diagnostics;
10-
using System.Linq;
119
using System.Reflection;
1210
using System.Text;
1311
using System.Threading;
@@ -593,60 +591,6 @@ integrated into a comprehensive development
593591
}
594592
}
595593

596-
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
597-
public static async Task CanReadBinaryData()
598-
{
599-
const int Size = 20_000;
600-
601-
byte[] data = Enumerable.Range(0, Size)
602-
.Select(i => (byte)(i % 256))
603-
.ToArray();
604-
string tableName = DataTestUtility.GenerateObjectName();
605-
606-
using (var connection = new SqlConnection(DataTestUtility.TCPConnectionString))
607-
{
608-
await connection.OpenAsync();
609-
610-
try
611-
{
612-
using (var createCommand = connection.CreateCommand())
613-
{
614-
createCommand.CommandText = $@"
615-
DROP TABLE IF EXISTS [{tableName}]
616-
CREATE TABLE [{tableName}] (Id INT IDENTITY(1,1) PRIMARY KEY, Data VARBINARY(MAX));
617-
INSERT INTO [{tableName}] (Data) VALUES (@data);";
618-
createCommand.Parameters.Add(new SqlParameter("@data", SqlDbType.VarBinary, Size) { Value = data });
619-
await createCommand.ExecuteNonQueryAsync();
620-
}
621-
622-
using (var command = connection.CreateCommand())
623-
{
624-
625-
command.CommandText = $"SELECT Data FROM [{tableName}]";
626-
command.Parameters.Clear();
627-
var result = (byte[])await command.ExecuteScalarAsync();
628-
629-
Assert.Equal(data, result);
630-
}
631-
632-
}
633-
finally
634-
{
635-
try
636-
{
637-
using (var dropCommand = connection.CreateCommand())
638-
{
639-
dropCommand.CommandText = $"DROP TABLE IF EXISTS [{tableName}]";
640-
dropCommand.ExecuteNonQuery();
641-
}
642-
}
643-
catch
644-
{
645-
}
646-
}
647-
}
648-
}
649-
650594
// Synapse: Cannot find data type 'rowversion'.
651595
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
652596
public static void CheckLegacyNullRowVersionIsEmptyArray()

0 commit comments

Comments
 (0)