Skip to content
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

Don't do full drain on chunks containing future buffered data #4215

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Mar 25, 2024

Description

This PR fixes bug where in certain situations, MsQuic would drop/corrupt data when received out of order + specific read timing from application side.

Testing

Failing test added to detect future regressions.

Documentation

N/A.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.33%. Comparing base (7bf265f) to head (bb006f4).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4215      +/-   ##
==========================================
+ Coverage   84.22%   85.33%   +1.11%     
==========================================
  Files          56       56              
  Lines       15358    15359       +1     
==========================================
+ Hits        12935    13107     +172     
+ Misses       2423     2252     -171     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rzikm
Copy link
Member Author

rzikm commented Mar 25, 2024

We still have some failures on the stress tests even with this fix, (dotnet/runtime#100064 (comment)), let's not merge just yet, I will see if there is more to be done in this PR.

@@ -907,7 +907,12 @@ QuicRecvBufferDrain(
}

do {
if ((uint64_t)RecvBuffer->ReadLength > DrainLength) {
if ((uint64_t)RecvBuffer->ReadLength > DrainLength ||
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite the right fix. It should still call QuicRecvBufferFullDrain to indicate it drained everything that was read, but QuicRecvBufferFullDrain should be handling the fact that just because it read everything doesn't mean there isn't more after a gap to read.

Copy link
Member

Choose a reason for hiding this comment

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

I am looking into what the right fix should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got more luck with this sort of change: rzikm@2beca9d

The gist of it is adjusting the ReadStart for multi/circular mode buffers, and moving the rest of the data for "single" mode buffers (not done in referenced commit, I just wanted to have something quic to run in stress env)

Copy link
Member

Choose a reason for hiding this comment

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

We talked over IM and maybe using the partial drain is the right way to go. We'd just kind of redefine 'partial drain' here to indicate it's not partial of the read length, but of all (possibly discontiguous) data in the buffer.

@rzikm rzikm requested a review from nibanks April 8, 2024 14:19
@nibanks nibanks merged commit 6abfbec into microsoft:main Apr 9, 2024
351 checks passed
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