Skip to content

Conversation

mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Jul 14, 2025

Description

Ports #3470 to release/6.1

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.

Issues

fixes #3463
fixes #3385

* 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>
@mdaigle mdaigle changed the base branch from main to release/6.1 July 14, 2025 23:13
@mdaigle mdaigle marked this pull request as ready for review July 14, 2025 23:13
@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 23:13
@mdaigle mdaigle requested a review from a team as a code owner July 14, 2025 23:13
@mdaigle mdaigle added this to the 6.1.0 milestone Jul 14, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Ports the fix from issue #3470 into the 6.1 release branch, ensuring snapshot buffers and data sizes are reset after successful reads.

  • Replaces SetSnapshotDataSize with AddSnapshotDataSize to accumulate read sizes instead of overwriting.
  • Introduces ClearSnapshotDataSize and clears snapshot storage when transitioning between packets.
  • Removes outdated Debug.Assert comments and ports similar changes to both .NET Framework and .NET Core parsers.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs Use AddSnapshotDataSize, add ClearSnapshotDataSize(), clear snapshot storage, drop stale asserts
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs Port ClearSnapshotDataSize() and switch to AddSnapshotDataSize in packet‐size increment
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs Port ClearSnapshotDataSize() and switch to AddSnapshotDataSize in packet‐size increment
Comments suppressed due to low confidence (3)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs:3582

  • Newly added methods AddSnapshotDataSize and ClearSnapshotDataSize don’t appear to have direct unit tests. Consider adding targeted tests to verify that snapshot data size and storage are properly reset after operations.
        internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)

@paulmedynski paulmedynski changed the title [6.1] Port #3470 [6.1] Fix TryReadPlpBytes throws ArgumentException Jul 15, 2025
Copy link

codecov bot commented Jul 15, 2025

Codecov Report

Attention: Patch coverage is 52.17391% with 11 lines in your changes missing coverage. Please review.

Project coverage is 66.81%. Comparing base (6773a65) to head (2fb758e).

Files with missing lines Patch % Lines
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 5 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 5 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 92.30% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/6.1    #3474      +/-   ##
===============================================
- Coverage        68.87%   66.81%   -2.07%     
===============================================
  Files              280      280              
  Lines            62322    62344      +22     
===============================================
- Hits             42927    41655    -1272     
- Misses           19395    20689    +1294     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 68.84% <66.66%> (-3.93%) ⬇️
netfx 69.23% <66.66%> (+1.18%) ⬆️

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.

Copy link
Contributor

@apoorvdeshmukh apoorvdeshmukh left a comment

Choose a reason for hiding this comment

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

:shipit:

@paulmedynski paulmedynski merged commit 0a55896 into release/6.1 Jul 15, 2025
129 checks passed
@paulmedynski paulmedynski deleted the dev/mdaigle/port-3470 branch July 15, 2025 13:57
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
Labels
None yet
Projects
None yet
4 participants