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.

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

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented May 4, 2025

fixes #3319

SqlCachedBuffer is used only when reading xml data. When adding the async-continue feature in #3161 the read function was changed to enable the use of stored snapshots and this edge case was missed.

When reading in async continue mode the first two packets are read in replay mode and at the end of the second packet if NeedsMoreData is returned an it is safe to mark as a continue point the snapshot marked the packet end of the second packet as the continue point. The code was incorrectly discarding the data that had already been read into the List<byte[]> from the first two packets which lead to the contents of the read being the remaining portion of the data from the packets after the continue point.

This change alters the logic used to decide whether to store the data which has been read into the snapshot and the logic used to decide whether the data fetched from the snapshot should be used.

@ErikEJ
Copy link
Contributor

ErikEJ commented May 4, 2025

Possible to add a test? Seems like a test was missing.

@Wraith2
Copy link
Contributor Author

Wraith2 commented May 4, 2025

Yes. Working on that today. It's a little complicated to reproduce cleanly.

@Wraith2
Copy link
Contributor Author

Wraith2 commented May 4, 2025

Test added. I tried for several hours to make an effective reduction but the combination of packet spanning and lengths just isn't conducive to simplification. Ultimately the failure causing logic is copied from the reproduction supplied relatively directly and altered for the test harness that we're using.

I'm unable to test locally because the build fails, I've raised #3330 to cover that problem.
@dotnet/sqlclientdevteam could someone trigger the CI please.

@ErikEJ
Copy link
Contributor

ErikEJ commented May 4, 2025

@Wraith2 Forgot to push the test?

To get pwsh on your system:

winget install --id Microsoft.PowerShell --source winget

@Wraith2
Copy link
Contributor Author

Wraith2 commented May 4, 2025

@Wraith2 Forgot to push the test?

To get pwsh on your system:

winget install --id Microsoft.PowerShell --source winget

The test is there, it's called CanReadXmlData.
I have powershell installed. As far as I know it's installed by default on 11. It's there and usable it's just not called pwsh.

@ErikEJ
Copy link
Contributor

ErikEJ commented May 5, 2025

Pwsh is Powershell Core, based on .NET Core. There are two "Powershells"

@cheenamalhotra
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Wraith2
Copy link
Contributor Author

Wraith2 commented May 5, 2025

I forgot xml reads to string canonicalize. test is updated. please trigger again.

@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Wraith2
Copy link
Contributor Author

Wraith2 commented May 5, 2025

I've reduced the iterations on the new test by 10x by finding a packet size which causes the failure much earlier. Previously it needed 800 iterations to find the failure, this one will find it in <100 iterations. That means the overall iterations to ensure failure is much lower. I hope this makes the test now succeed meaning that the only problem was that it was too slow in combination with all the other tests in stage 2 of the tests. Please re-run CI.

@cheenamalhotra
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented May 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.49%. Comparing base (eef8636) to head (a42d2c0).
Report is 17 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (eef8636) and HEAD (a42d2c0). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (eef8636) HEAD (a42d2c0)
addons 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3329       +/-   ##
===========================================
- Coverage   92.58%   59.49%   -33.09%     
===========================================
  Files           6      292      +286     
  Lines         310    65225    +64915     
===========================================
+ Hits          287    38806    +38519     
- Misses         23    26419    +26396     
Flag Coverage Δ
addons ?
netcore 62.80% <100.00%> (?)
netfx 60.57% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


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.

@mdaigle mdaigle merged commit c3857b1 into dotnet:main May 13, 2025
237 checks passed
@Wraith2 Wraith2 deleted the fix-3319 branch May 13, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Xml.XmlException: Unexpected end tag
6 participants