Fix SqlCached buffer async read with continue edge case. #3329
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). |
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.
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.
There was a problem hiding this comment.
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.
…)" This reverts commit c3857b1.
…)" This reverts commit c3857b1.
* Revert 1af7327 * Revert "Fix TryReadPlpBytes throws ArgumentException (#3470) (#3474)" This reverts commit 0a55896. * Revert 44762d9 * Revert "Improve async string perf and fix reading chars with initial offset. (#3377)" This reverts commit 05df554. * Revert "Refine handling of moving between replay and continue states (#3337)" This reverts commit 265f522. * Revert "Fix SqlCached buffer async read with continue edge case. (#3329)" This reverts commit c3857b1. * Revert "Add `async` snapshot continue capability for multipacket fields (#3161)" This reverts commit 33364e7. * Revert "Add partial packet detection and fixup (#2714)" This reverts commit 8d5e4f2. * Remove methods previously moved to common file. * Supply byte buffer to vector read. * Minor compilation fixes that were missed in the reverts. * Remove partial packet context switch helpers. * Remove accidental duplication of SqlDataReader * Revert len change * Undo buff rental in netfx to simplify 6.0 diff. * Fix missed rented buff code. --------- Co-authored-by: Cheena Malhotra <cmalhotra@microsoft.com>
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.