Fix TryReadPlpBytes throws ArgumentException #3470
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3470 +/- ##
===========================================
- Coverage 68.86% 58.78% -10.08%
===========================================
Files 280 270 -10
Lines 62417 61877 -540
===========================================
- Hits 42982 36373 -6609
- Misses 19435 25504 +6069
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:
|
paulmedynski
requested changes
Jul 11, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Show resolved
Hide resolved
|
/azp run |
paulmedynski
approved these changes
Jul 12, 2025
|
Azure Pipelines successfully started running 2 pipeline(s). |
mdaigle
approved these changes
Jul 14, 2025
mdaigle
pushed a commit
that referenced
this pull request
Jul 14, 2025
* add clearing of data sizes when a multi-part result is returned * remove asserts detecting overwrite of snapshot storage which are now no longer accurate * remove empty statement block --------- Co-authored-by: Wraith2 <Wraith2@gmaill.com>
paulmedynski
pushed a commit
that referenced
this pull request
Jul 15, 2025
* add clearing of data sizes when a multi-part result is returned * remove asserts detecting overwrite of snapshot storage which are now no longer accurate * remove empty statement block --------- Co-authored-by: Wraith2 <Wraith2@gmaill.com>
paulmedynski
pushed a commit
that referenced
this pull request
Jul 15, 2025
* add clearing of data sizes when a multi-part result is returned * remove asserts detecting overwrite of snapshot storage which are now no longer accurate * remove empty statement block --------- Co-authored-by: Wraith <wraith2@gmail.com> Co-authored-by: Wraith2 <Wraith2@gmaill.com>
samsharma2700
added a commit
that referenced
this pull request
Aug 5, 2025
mdaigle
added a commit
that referenced
this pull request
Aug 6, 2025
cheenamalhotra
added a commit
that referenced
this pull request
Aug 14, 2025
* 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #3463
fixes #3385
In the reproduction shared in #3463 multiple reads of text fields are ddone in sequence with different lengths until a particular scenario occurs. The situation that causes the error is that a multi-packet text read completes on the third packet of an async operation and the lengths of the text sections in the packets align such that the third packet completes one text read followed by the another text read from the same packet but for a different column.
This hard to reach set of circumstances causes the read logic to see a text read starting in a ContinueReplay state when another read has finished on the same packet and the error is caused by the first completed read leaving behind the data packet byte count used when it completed.
The fix is much easier than trying to replicate and understand the error. We simply need to clean up the data lengths and buffer when we successfully read a value. I've added this logic to the place where the bug was found and precautionarily to the other place in the codebase that does similar logic (one for bytes, one for chars).
Creating a test and runs quickly enough to be reliable in the CI is prohibitively difficult. I haven't been able to go anything that needs fewer than 1600 iterations of large text reads. In this case I feel that the fix is obvious enough that existing coverage may be enough.
This also removes the asserts pointed out as problematic in #3385 i've reviewed them and they're no longer applicable, they were too stringent for the more relaxed buffer handing used when moving from replay to continue states.
@dotnet/sqlclientdevteam can I get a CI run please
/cc @frankbuckley