Skip to content

Conversation

paulmedynski
Copy link
Contributor

Cherry-pick of #3470 from main into release/6.1:

  • 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

Issues

#3385
#3463

* 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 <[email protected]>
@paulmedynski paulmedynski added this to the 6.1.0 milestone Jul 15, 2025
@Copilot Copilot AI review requested due to automatic review settings July 15, 2025 10:32
@paulmedynski paulmedynski requested a review from a team as a code owner July 15, 2025 10:32
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

This PR backports fixes to snapshot data size handling in PLP (partially-length prefixed) reads, addressing an ArgumentException in TryReadPlpBytes. It replaces single-value snapshots with additive accumulation, ensures sizes are cleared between results, and removes obsolete assertions.

  • Switch from SetSnapshotDataSize to AddSnapshotDataSize in multiple locations.
  • Introduce ClearSnapshotDataSize and ClearPacketDataSize to reset snapshot state.
  • Remove outdated Debug.Assert calls and an empty statement block.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs Replaced snapshot size setters, added clearing methods, removed stale asserts
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs Updated PLP read logic to clear snapshot data and use additive sizing
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs Mirrored netfx changes for PLP snapshot size accumulation and clearing
Comments suppressed due to low confidence (3)

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

  • [nitpick] The parameter name 'countOfBytesCopiedFromCurrentPacket' is quite verbose and differs from other methods that use 'bytesRead'. Consider renaming it to 'bytesRead' for consistency and readability.
        internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)

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

  • Add XML documentation comments to AddSnapshotDataSize (and similarly to ClearSnapshotDataSize) explaining their purpose, parameters, and when they should be called, to maintain consistency with other internal methods.
        internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)

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

  • Introduce unit tests covering AddSnapshotDataSize, ClearSnapshotDataSize, and ClearPacketDataSize to verify correct accumulation and resetting of snapshot sizes and prevent future regressions.
        internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)

@paulmedynski
Copy link
Contributor Author

Superceded by PR #3474

@paulmedynski paulmedynski removed this from the 6.1.0 milestone Jul 15, 2025
Copy link

codecov bot commented Jul 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.58%. Comparing base (df35633) to head (d1cdeab).
Report is 2 commits behind head on release/6.1.

Additional details and impacted files
@@               Coverage Diff                @@
##           release/6.1    #3476       +/-   ##
================================================
+ Coverage        66.91%   92.58%   +25.66%     
================================================
  Files              280        6      -274     
  Lines            62386      310    -62076     
================================================
- Hits             41745      287    -41458     
+ Misses           20641       23    -20618     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore ?
netfx ?

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.

@paulmedynski paulmedynski deleted the dev/paul/release/6.1-tryreadplpbytes branch August 15, 2025 11:04
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.

2 participants