Skip to content

[6.1] Revert partial packet fixup and replay improvements #3556

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Aug 14, 2025

Conversation

mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Aug 11, 2025

Description

This PR reverts several large changes made to improve partial packet detection, fixup, and replay functionality. It maintains all test coverage added along with the original changes. We have a handle on many of the issues discovered in 6.1.0, but out of an abundance of caution, we've made the decision to revert this functionality so that we can provide a stable 6.1.X release. We'll continue to evaluate the functionality on the main branch with the goal of including it in the next, non-patch release.

To simplify the diff with release/6.0, one additional piece of functionality dealing with rented buffs when reading plp data is also reverted: #2982

Buffer management is also added for reading vector data. This change needs close review.

Issues

#3536
#3519

Testing

New tests are maintained. Existing test coverage should help ensure that we've returned to baseline.
Stress tests have been run on this PR and show identical behavior to 6.0.2.

Guidelines

Please review the contribution guidelines before submitting a pull request:

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 12, 2025

I've looked through the files and it looks like a complete reversion of the work to add continue.
I'm not convinced that it should have any affect on #3536 though. If it does it isn't by direct code change but by assumption invalidation.

@cheenamalhotra cheenamalhotra added this to the 6.1.1 milestone Aug 12, 2025
@cheenamalhotra cheenamalhotra marked this pull request as ready for review August 12, 2025 15:56
@Copilot Copilot AI review requested due to automatic review settings August 12, 2025 15:56
@cheenamalhotra cheenamalhotra requested a review from a team as a code owner August 12, 2025 15:56
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 reverts large-scale changes made to improve partial packet detection, fixup, and replay functionality in the SQL Client library while maintaining test coverage. The changes are primarily focused on simplifying the packet processing logic and removing complex buffer management features. Additionally, buffer management for reading vector data is modified.

Key Changes:

  • Reverts packet multiplexing and replay improvements for partial packet handling
  • Removes complex snapshot state management for asynchronous operations
  • Simplifies buffer reading operations by removing continue-capable multi-packet read functionality

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
DataReaderTest.cs Removes unused import and fixes test method name typo
DataReaderStreamsTest.cs Fixes spelling in comment (minimum -> minimun)
TdsParserStateObjectHelper.cs Removes unused import and adds whitespace formatting
AsyncCancelledConnectionsTest.cs Removes complex tracking logic and simplifies async test execution
TdsParserStateObject.TestHarness.cs Removes entire test harness file
MultiplexerTests.cs Removes entire multiplexer test file
Microsoft.Data.SqlClient.FunctionalTests.csproj Removes references to deleted test files
LocalAppContextSwitchesTests.cs Removes compatibility switch test cases
LocalAppContextSwitchesHelper.cs Removes compatibility switch helper properties
TdsParserStateObject.cs Major simplification of packet processing, snapshot management, and buffer handling
TdsParserStateObject.Multiplexer.cs Removes entire multiplexer implementation file
SqlDataReader.cs Simplifies debug assertion
SqlCachedBuffer.cs Simplifies buffer creation logic
Packet.cs Removes entire packet utility class
LocalAppContextSwitches.cs Removes compatibility switch properties
TdsParserStateObject.netfx.cs Adds simplified ProcessSniPacket implementation
TdsParser.cs Simplifies data reading operations and removes continue-capable methods
TdsParserStateObject.netcore.cs Adds simplified ProcessSniPacket implementation
TdsParser.Windows.cs Removes packet assertion
Microsoft.Data.SqlClient.csproj Removes references to deleted source files

Copy link

codecov bot commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 69.40639% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.84%. Comparing base (7a7b54e) to head (9a6817f).
⚠️ Report is 5 commits behind head on release/6.1.

Files with missing lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 6.81% 41 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 78.18% 12 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 90.27% 7 Missing ⚠️
...osoft/Data/SqlClient/TdsParserStateObject.netfx.cs 76.19% 5 Missing ⚠️
...oft/Data/SqlClient/TdsParserStateObject.netcore.cs 90.90% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/6.1    #3556      +/-   ##
===============================================
- Coverage        69.69%   68.84%   -0.86%     
===============================================
  Files              281      279       -2     
  Lines            62413    61748     -665     
===============================================
- Hits             43500    42511     -989     
- Misses           18913    19237     +324     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore 72.71% <86.36%> (-0.05%) ⬇️
netfx 68.16% <62.67%> (-1.08%) ⬇️

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

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

I'll need a walkthrough of the TdsParser* files.

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 12, 2025

If the change I made in d38da1c does truly fix the gchandle problem then this PR will too because it removes the changed code that contained the mistake.

I'll need a walkthrough of the TdsParser* files.
From me?

@cheenamalhotra
Copy link
Member

@Wraith2 can you share more info on how to reproduce the gchandle issue to confirm this?
Me and @mdaigle haven't been able to reproduce the issue as per repro here: #3519 (comment)

If the change I made in d38da1c does truly fix the gchandle problem then this PR will too because it removes the changed code that contained the mistake.

I'll need a walkthrough of the TdsParser* files.
From me?

This was referenced Aug 15, 2025
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.

6 participants