- Notifications
You must be signed in to change notification settings - Fork311
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ffer used in xml reads
Possible to add a test? Seems like a test was missing. |
Yes. Working on that today. It's a little complicated to reproduce cleanly. |
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. |
@Wraith2 Forgot to push the test? To get pwsh on your system:
|
The test is there, it's called CanReadXmlData. |
Pwsh is Powershell Core, based on .NET Core. There are two "Powershells" |
/azp run |
| Azure Pipelines successfully started running 2 pipeline(s). |
I forgot xml reads to string canonicalize. test is updated. please trigger again. |
/azp run |
| Azure Pipelines successfully started running 2 pipeline(s). |
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. |
/azp run |
| Azure Pipelines successfully started running 2 pipeline(s). |
codecovbot commentedMay 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| List<byte[]> cachedBytes = null; | ||
| if (isAvailable) | ||
| { | ||
| cachedBytes = stateObj.TryTakeSnapshotStorage() as List<byte[]>; | ||
| if (cachedBytes != null && !isStarting && !isContinuing) | ||
| if (isStarting) |
There was a problem hiding this comment.
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 setcachedBytes to null whenstarting. Any new data we read is added to thecachedBytes and stored in the snapshot as needed, not to be overwritten until we've completed this read.
There was a problem hiding this comment.
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.
c3857b1 intodotnet:mainUh oh!
There was an error while loading.Please reload this page.
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.