Skip to content

Fix SqlCached buffer async read with continue edge case. #3329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 13, 2025
Merged

Fix SqlCached buffer async read with continue edge case. #3329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a70c81
refine logic around switching from replay to continue for SqlCachedBu…
Wraith2 May 4, 2025
3213a40
add test and cleanup unused variable
Wraith2 May 4, 2025
285fae4
account for xml read canonicalization in tests
Wraith2 May 5, 2025
a42d2c0
reduce iterations on new test
Wraith2 May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser
{
buffer = null;

(bool isAvailable, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses();
(bool isAvailable, bool isStarting, _) = stateObj.GetSnapshotStatuses();

List<byte[]> cachedBytes = null;
if (isAvailable)
{
cachedBytes = stateObj.TryTakeSnapshotStorage() as List<byte[]>;
if (cachedBytes != null && !isStarting && !isContinuing)
if (isStarting)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relates to this question posed on the original change:
https://github.com/dotnet/SqlClient/pull/3161/files#r1994120794

Resetting the snapshot storage here wiped out the data in the snapshot that future SqlCachedBuffers would need.
Now, we only set cachedBytes to null when starting. Any new data we read is added to the cachedBytes and stored in the snapshot as needed, not to be overwritten until we've completed this read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Overall while it works I'm not happy with how complex the code is but I can't find a way to do everything it needs to more simply.

{
stateObj.SetSnapshotStorage(null);
cachedBytes = null;
}
}

Expand All @@ -56,15 +56,13 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser
cachedBytes = new List<byte[]>();
}


// the very first length is already read.
TdsOperationStatus result = parser.TryPlpBytesLeft(stateObj, out ulong plplength);
if (result != TdsOperationStatus.Done)
{
return result;
}


// For now we only handle Plp data from the parser directly.
Debug.Assert(metadata.metaType.IsPlp, "SqlCachedBuffer call on a non-plp data");
do
Expand Down Expand Up @@ -105,10 +103,7 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser

if (returnAfterAdd)
{
if (isStarting || isContinuing)
{
stateObj.SetSnapshotStorage(cachedBytes);
}
stateObj.SetSnapshotStorage(cachedBytes);
return result;
}

Expand Down
203 changes: 203 additions & 0 deletions src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,209 @@ public static async Task<int> CanReadEmployeesTableCompletelyWithNullImageAsync(
return counter;
}

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static async Task<int> CanReadXmlData()
{
const string xml = @"<catalog id1=""00000000-0000-0000-0000-000000000000"" id2=""00000000-0000-0000-0000-000000000000"" id3=""00000000-0000-0000-0000-000000000000"" id4=""00000000-0000-0000-0000-000000000000"" id5=""00000000-0000-0000-0000-000000000000"">
<book id=""bk101"">
<author>Gambardella, Matthew</author>
<title>XML Developer's Guide</title>
<genre>Computer</genre>
<price>44.95</price>
<publish_date>2000-10-01</publish_date>
<description>An in-depth look at creating applications
with XML.</description>
</book>
<book id=""bk102"">
<author>Ralls, Kim</author>
<title>Midnight Rain</title>
<genre>Fantasy</genre>
<price>5.95</price>
<publish_date>2000-12-16</publish_date>
<description>A former architect battles corporate zombies,
an evil sorceress, and her own childhood to become queen
of the world.</description>
</book>
<book id=""bk103"">
<author>Corets, Eva</author>
<title>Maeve Ascendant</title>
<genre>Fantasy</genre>
<price>5.95</price>
<publish_date>2000-11-17</publish_date>
<description>After the collapse of a nanotechnology
society in England, the young survivors lay the
foundation for a new society.</description>
</book>
<book id=""bk104"">
<author>Corets, Eva</author>
<title>Oberon's Legacy</title>
<genre>Fantasy</genre>
<price>5.95</price>
<publish_date>2001-03-10</publish_date>
<description>In post-apocalypse England, the mysterious
agent known only as Oberon helps to create a new life
for the inhabitants of London. Sequel to Maeve
Ascendant.</description>
</book>
<book id=""bk105"">
<author>Corets, Eva</author>
<title>The Sundered Grail</title>
<genre>Fantasy</genre>
<price>5.95</price>
<publish_date>2001-09-10</publish_date>
<description>The two daughters of Maeve, half-sisters,
battle one another for control of England. Sequel to
Oberon's Legacy.</description>
</book>
<book id=""bk106"">
<author>Randall, Cynthia</author>
<title>Lover Birds</title>
<genre>Romance</genre>
<price>4.95</price>
<publish_date>2000-09-02</publish_date>
<description>When Carla meets Paul at an ornithology
conference, tempers fly as feathers get ruffled.</description>
</book>
<book id=""bk107"">
<author>Thurman, Paula</author>
<title>Splish Splash</title>
<genre>Romance</genre>
<price>4.95</price>
<publish_date>2000-11-02</publish_date>
<description>A deep sea diver finds true love twenty
thousand leagues beneath the sea.</description>
</book>
<book id=""bk108"">
<author>Knorr, Stefan</author>
<title>Creepy Crawlies</title>
<genre>Horror</genre>
<price>4.95</price>
<publish_date>2000-12-06</publish_date>
<description>An anthology of horror stories about roaches,
centipedes, scorpions and other insects.</description>
</book>
<book id=""bk109"">
<author>Kress, Peter</author>
<title>Paradox Lost</title>
<genre>Science Fiction</genre>
<price>6.95</price>
<publish_date>2000-11-02</publish_date>
<description>After an inadvertant trip through a Heisenberg
Uncertainty Device, James Salway discovers the problems
of being quantum.</description>
</book>
<book id=""bk110"">
<author>O'Brien, Tim</author>
<title>Microsoft .NET: The Programming Bible</title>
<genre>Computer</genre>
<price>36.95</price>
<publish_date>2000-12-09</publish_date>
<description>Microsoft's .NET initiative is explored in
detail in this deep programmer's reference.</description>
</book>
<book id=""bk111"">
<author>O'Brien, Tim</author>
<title>MSXML3: A Comprehensive Guide</title>
<genre>Computer</genre>
<price>36.95</price>
<publish_date>2000-12-01</publish_date>
<description>The Microsoft MSXML3 parser is covered in
detail, with attention to XML DOM interfaces, XSLT processing,
SAX and more.</description>
</book>
<book id=""bk112"">
<author>Galos, Mike</author>
<title>Visual Studio 7: A Comprehensive Guide</title>
<genre>Computer</genre>
<price>49.95</price>
<publish_date>2001-04-16</publish_date>
<description>Microsoft Visual Studio 7 is explored in depth,
looking at how Visual Basic, Visual C++, C#, and ASP+ are
integrated into a comprehensive development
environment.</description>
</book>
</catalog>";

string tableName = DataTestUtility.GenerateObjectName();

SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString);
builder.PersistSecurityInfo = true;
builder.PacketSize = 3096; // should reproduce failure that this tests for in <100 reads

using (var connection = new SqlConnection(builder.ToString()))
{
await connection.OpenAsync();

try
{
// setup
using (var dropCommand = connection.CreateCommand())
{
dropCommand.CommandText = $"DROP TABLE IF EXISTS [{tableName}]";
dropCommand.ExecuteNonQuery();
}

using (var createCommand = connection.CreateCommand())
{
createCommand.CommandText = $"CREATE TABLE [{tableName}] (Id int PRIMARY KEY, Data xml NOT NULL)";
createCommand.ExecuteNonQuery();
}

for (var i = 0; i < 100; i++)
{
using (var insertCommand = connection.CreateCommand())
{
insertCommand.CommandText = $"INSERT INTO [{tableName}] (Id, Data) VALUES (@id, @data)";
insertCommand.Parameters.AddWithValue("@id", i);
insertCommand.Parameters.AddWithValue("@data", xml);
insertCommand.ExecuteNonQuery();
}
}

// execute
int id = 0;

using (var command = connection.CreateCommand())
{
command.CommandText = $"SELECT Data FROM [{tableName}] ORDER BY Id";
using (var reader = await command.ExecuteReaderAsync())
{
string expectedResult = null;
while (await reader.ReadAsync())
{
var result = reader.GetString(0);
if (expectedResult == null)
{
expectedResult = result;
}
else
{
Assert.Equal(expectedResult, result);
}
id++;
}
}
}
return id;

}
finally
{
try
{
using (var dropCommand = connection.CreateCommand())
{
dropCommand.CommandText = $"DROP TABLE IF EXISTS [{tableName}]";
dropCommand.ExecuteNonQuery();
}
}
catch
{
}
}
}
}

// Synapse: Cannot find data type 'rowversion'.
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
public static void CheckLegacyNullRowVersionIsEmptyArray()
Expand Down
Loading