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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c117aeb
Don't do full drain on chunks containing future buffered data
rzikm Mar 22, 2024
9498d50
Fix comment
rzikm Mar 25, 2024
0e2341f
Fix
rzikm Apr 8, 2024
c05854e
Remove commented out code
rzikm Apr 8, 2024
a1f7bb3
Revert change which breaks tests
rzikm Apr 9, 2024
7b02ecd
Fix test failures
rzikm Apr 9, 2024
bb006f4
Fix test
rzikm Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/core/recv_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

//
// If there are 2 or more written ranges, it means that there may be
// more data later in the chunk that couldn't be read because there is a gap.
//
RecvBuffer->WrittenRanges.UsedLength > 1) {
QuicRecvBufferPartialDrain(RecvBuffer, DrainLength);
return FALSE;
}
Expand Down
56 changes: 56 additions & 0 deletions src/core/unittest/RecvBufferTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,62 @@ TEST_P(WithMode, RecvCrypto) // Note - Values based on a previous failing MsQuic
}
}

TEST_P(WithMode, DrainFrontChunkWithPendingGap)
{
RecvBuffer RecvBuf;
ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam(), false));
uint64_t InOutWriteLength = DEF_TEST_BUFFER_LENGTH;
BOOLEAN NewDataReady = FALSE;

// place data at some future offset
ASSERT_EQ(
QUIC_STATUS_SUCCESS,
RecvBuf.Write(
2,
1,
&InOutWriteLength,
&NewDataReady));
ASSERT_FALSE(RecvBuf.HasUnreadData());

// place data to the front and drain
InOutWriteLength = DEF_TEST_BUFFER_LENGTH;
ASSERT_EQ(
QUIC_STATUS_SUCCESS,
RecvBuf.Write(
0,
1,
&InOutWriteLength,
&NewDataReady));
ASSERT_TRUE(NewDataReady);

uint64_t ReadOffset;
QUIC_BUFFER ReadBuffers[3];
uint32_t BufferCount = ARRAYSIZE(ReadBuffers);
RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers);
ASSERT_EQ(1ul, BufferCount);
ASSERT_EQ(1ul, ReadBuffers[0].Length);
RecvBuf.Drain(1);

// insert missing chunk and drain the rest
InOutWriteLength = DEF_TEST_BUFFER_LENGTH;
ASSERT_EQ(
QUIC_STATUS_SUCCESS,
RecvBuf.Write(
1,
1,
&InOutWriteLength,
&NewDataReady));
ASSERT_TRUE(NewDataReady);

BufferCount = ARRAYSIZE(ReadBuffers);
RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers);
uint64_t TotalRead = 0;
for (uint32_t i = 0; i < BufferCount; ++i) {
TotalRead += ReadBuffers[i].Length;
}
ASSERT_EQ(2ul, TotalRead);
}

INSTANTIATE_TEST_SUITE_P(
RecvBufferTest,
WithMode,
Expand Down
Loading