Skip to content

Fix WebTransportImpl session flow control granting too frequently #613

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

Closed
afrind wants to merge 1 commit into from
+50 −28
Closed

Fix WebTransportImpl session flow control granting too frequently #613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
70 changes: 46 additions & 24 deletions proxygen/lib/http/session/test/HTTPTransactionWebTransportTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,6 @@ TEST_F(HTTPTransactionWebTransportTest, ReadStream) {
});
EXPECT_FALSE(fut.isReady());

EXPECT_CALL(transport_, sendWTMaxData(kDefaultWTReceiveWindow + 10))
.WillOnce(Return(folly::unit));
implHandle->dataAvailable(makeBuf(10), false);
EXPECT_FALSE(fut.isReady());
eventBase_.loopOnce();
Expand All @@ -243,8 +241,6 @@ TEST_F(HTTPTransactionWebTransportTest, ReadStream) {
EXPECT_CALL(transport_, readWebTransportData(_, _)).WillOnce(Invoke([] {
return std::make_pair(makeBuf(32768), false);
}));
EXPECT_CALL(transport_, sendWTMaxData(kDefaultWTReceiveWindow + 10 + 65536))
.WillOnce(Return(folly::unit));
implHandle->readAvailable(0);
EXPECT_CALL(transport_, resumeWebTransportIngress(0));
fut = readHandle->readStreamData()
Expand Down Expand Up @@ -624,8 +620,6 @@ TEST_F(HTTPTransactionWebTransportTest, StreamIDAPIs) {
EXPECT_CALL(transport_, readWebTransportData(_, _)).WillOnce(Invoke([] {
return std::make_pair(makeBuf(10), false);
}));
EXPECT_CALL(transport_, sendWTMaxData(kDefaultWTReceiveWindow + 10))
.WillOnce(Return(folly::unit));
quicReadCallback->readAvailable(id);
eventBase_.loopOnce();
EXPECT_TRUE(fut.isReady());
Expand Down Expand Up @@ -1328,17 +1322,19 @@ TEST_F(HTTPTransactionWebTransportTest, ReceiveWTDataBlockedCapsule) {
dynamic_cast<WebTransportImpl::StreamReadHandle*>(readHandle);
ASSERT_NE(implHandle, nullptr);

// Deliver data close to our window limit to put peer in blocked state.
// We'll deliver (kDefaultWTReceiveWindow - 100) bytes.
EXPECT_CALL(
transport_,
sendWTMaxData(kDefaultWTReceiveWindow + kDefaultWTReceiveWindow - 100))
.WillOnce(Return(folly::unit));
// Deliver (kDefaultWTReceiveWindow - 100) bytes — fills the window.
// No grant fires yet: no pending readStreamData() promise.
auto fcState =
implHandle->dataAvailable(makeBuf(kDefaultWTReceiveWindow - 100), false);
EXPECT_EQ(fcState, WebTransport::FCState::BLOCKED);

// Read the data to consume it, making room in our flow control window.
// Read the data. maybeGrantFlowControl fires here because
// getMaxOffset() - bytesRead_ = 100 < kDefaultWTReceiveWindow/2.
// newMaxData = (kDefaultWTReceiveWindow - 100) + kDefaultWTReceiveWindow.
EXPECT_CALL(
transport_,
sendWTMaxData(kDefaultWTReceiveWindow + kDefaultWTReceiveWindow - 100))
.WillOnce(Return(folly::unit));
EXPECT_CALL(transport_, resumeWebTransportIngress(0));
auto fut = readHandle->readStreamData()
.via(&eventBase_)
Expand All @@ -1351,23 +1347,49 @@ TEST_F(HTTPTransactionWebTransportTest, ReceiveWTDataBlockedCapsule) {
eventBase_.loopOnce();
EXPECT_TRUE(fut.isReady());

// Now buffered bytes should be low enough (< kDefaultWTReceiveWindow / 2)
// for shouldGrantFlowControl() to return true.
EXPECT_TRUE(wtImpl->shouldGrantFlowControl());
// After the grant, maxOffset_ = 2*kDefaultWTReceiveWindow - 100.
// maxOffset_ - bytesRead_ = kDefaultWTReceiveWindow > window/2: no more grants.
EXPECT_FALSE(wtImpl->shouldGrantFlowControl());

// When we receive a WT_DATA_BLOCKED capsule with maxData matching our current
// limit (which is now kDefaultWTReceiveWindow + kDefaultWTReceiveWindow -
// 100), we should send WT_MAX_DATA with increased limit newMaxData = (2 *
// kDefaultWTReceiveWindow - 100) + kDefaultWTReceiveWindow
// onDataBlocked with the current maxData: shouldGrantFlowControl() is false,
// so no redundant WT_MAX_DATA is sent.
const uint64_t currentMaxData = 2 * kDefaultWTReceiveWindow - 100;
EXPECT_CALL(transport_,
sendWTMaxData(currentMaxData + kDefaultWTReceiveWindow))
.WillOnce(Return(folly::unit));

wtImpl->onDataBlocked(currentMaxData);

EXPECT_CALL(transport_, stopReadingWebTransportIngress(0, _))
.WillRepeatedly(Return(folly::unit));
}

TEST_F(HTTPTransactionWebTransportTest, FrequentFlowControlGrants) {
Copy link
Copy Markdown

@joanna-jo joanna-jo Apr 20, 2026

Choose a reason for hiding this comment

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

redundant test? i think this is already tested above?

// shouldGrantFlowControl() should only send WT_MAX_DATA when the app has
// consumed at least half the granted window (getMaxOffset() - bytesRead_ <
// window/2). Reading 500 bytes out of a 1 MB window leaves the peer with
// ~1 MB of headroom, so no grant should be sent.
WebTransport::StreamReadHandle* readHandle{nullptr};
EXPECT_CALL(handler_, onWebTransportUniStream(_, _))
.WillOnce(SaveArg<1>(&readHandle));
txn_->onWebTransportUniStream(0);
ASSERT_NE(readHandle, nullptr);
auto* implHandle =
dynamic_cast<WebTransportImpl::StreamReadHandle*>(readHandle);
ASSERT_NE(implHandle, nullptr);

// 5 x 100 bytes = 500 bytes consumed, far below kDefaultWTReceiveWindow/2
// (512 KB). sendWTMaxData must not fire — the peer has ample window left.
constexpr uint64_t kReadSize = 100;
constexpr int kNumReads = 5;
EXPECT_CALL(transport_, sendWTMaxData(_)).Times(0);
for (int i = 0; i < kNumReads; i++) {
auto fut =
readHandle->readStreamData().via(&eventBase_).thenTry([](auto) {});
EXPECT_FALSE(fut.isReady());
implHandle->dataAvailable(makeBuf(kReadSize), false);
eventBase_.loopOnce();
EXPECT_TRUE(fut.isReady());
}

EXPECT_CALL(transport_, stopReadingWebTransportIngress(0, _))
.WillRepeatedly(Return(folly::unit));
}

} // namespace proxygen::test
8 changes: 4 additions & 4 deletions proxygen/lib/http/webtransport/WebTransportImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,8 +555,9 @@ void WebTransportImpl::maybeGrantFlowControl(uint64_t bytesRead) {
bytesRead_ += bytesRead;
if (bytesRead && shouldGrantFlowControl()) {
auto newMaxData = bytesRead_ + kDefaultWTReceiveWindow;
recvFlowController_.grant(newMaxData);
tp_.sendWTMaxData(newMaxData);
if (recvFlowController_.grant(newMaxData)) {
tp_.sendWTMaxData(newMaxData);
}
}
}

Expand Down Expand Up @@ -590,8 +591,7 @@ void WebTransportImpl::maybeGrantStreamCredit(HTTPCodec::StreamID id,
}

bool WebTransportImpl::shouldGrantFlowControl() const {
auto bufferedBytes = recvFlowController_.getCurrentOffset() - bytesRead_;
return bufferedBytes < kDefaultWTReceiveWindow / 2;
return recvFlowController_.getMaxOffset() - bytesRead_ < kDefaultWTReceiveWindow / 2;
}

bool WebTransportImpl::shouldGrantStreamCredit(bool isBidi) const {
Expand Down
Loading