Skip to content

Commit 912c44a

Browse files
hanidamlajfacebook-github-bot
authored andcommitted
WebTransportImpl fix StreamReadHandle::dataAvailable
Summary: grr, undo-ing a mistake in the previous diff (D83162391) the window was a session-level recv window, and it should be WebTransportImpl's job to ensure we don't invoke TransportProvider::sendWTMaxData() if only eof is read on the stream Reviewed By: afrind Differential Revision: D83355330 fbshipit-source-id: 8df6ff2518f04d5738d789011a0c131108bfb098
1 parent f8d47ff commit 912c44a

File tree

3 files changed

+8
-14
lines changed

3 files changed

+8
-14
lines changed

proxygen/lib/http/session/test/HTTPTransactionWebTransportTest.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -206,12 +206,8 @@ TEST_F(HTTPTransactionWebTransportTest, ReadStream) {
206206
EXPECT_CALL(transport_, readWebTransportData(_, _)).WillOnce(Invoke([] {
207207
return std::make_pair(makeBuf(32768), false);
208208
}));
209-
// hmm, should WebTransportImpl ensure this isn't invoked if we've rx'd eof or
210-
// is it the transport's responsiblity to not send WTMaxData capsule in this
211-
// case – leaning towards the latter
212209
EXPECT_CALL(transport_, sendWTMaxData(kDefaultWTReceiveWindow + 10 + 65536))
213-
.Times(2)
214-
.WillRepeatedly(Return(folly::unit));
210+
.WillOnce(Return(folly::unit));
215211
implHandle->readAvailable(0);
216212
EXPECT_CALL(transport_, resumeWebTransportIngress(0));
217213
fut = readHandle->readStreamData()
@@ -449,8 +445,7 @@ TEST_F(HTTPTransactionWebTransportTest, BidiStreamEdgeCases) {
449445
EXPECT_CALL(transport_,
450446
stopReadingWebTransportIngress(0, folly::Optional<uint32_t>()));
451447

452-
EXPECT_CALL(transport_, sendWTMaxData(kDefaultWTReceiveWindow))
453-
.WillOnce(Return(folly::unit));
448+
EXPECT_CALL(transport_, sendWTMaxData(kDefaultWTReceiveWindow)).Times(0);
454449
auto fut = streamHandle.readHandle->readStreamData()
455450
.via(&eventBase_)
456451
.thenTry([](auto streamData) {

proxygen/lib/http/webtransport/WebTransportImpl.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -363,8 +363,7 @@ WebTransportImpl::StreamReadHandle::readStreamData() {
363363
VLOG(4) << __func__ << " returning data len=" << buf_.chainLength();
364364
auto bufLen = buf_.chainLength();
365365
StreamData streamData({.data = buf_.move(), .fin = eof_});
366-
impl_.bytesRead_ += bufLen;
367-
impl_.maybeGrantFlowControl();
366+
impl_.maybeGrantFlowControl(bufLen);
368367

369368
if (eof_) {
370369
// unregister the read callback, but don't send STOP_SENDING
@@ -415,8 +414,7 @@ WebTransport::FCState WebTransportImpl::StreamReadHandle::dataAvailable(
415414
}
416415

417416
if (readPromise_.valid()) {
418-
impl_.bytesRead_ += len;
419-
impl_.maybeGrantFlowControl();
417+
impl_.maybeGrantFlowControl(len);
420418
readPromise_.setValue(StreamData({.data = std::move(data), .fin = eof}));
421419
readPromise_ = emptyReadPromise();
422420
if (eof) {
@@ -474,8 +472,9 @@ void WebTransportImpl::StreamReadHandle::deliverReadError(
474472
}
475473
}
476474

477-
void WebTransportImpl::maybeGrantFlowControl() {
478-
if (shouldGrantFlowControl()) {
475+
void WebTransportImpl::maybeGrantFlowControl(uint64_t bytesRead) {
476+
bytesRead_ += bytesRead;
477+
if (bytesRead && shouldGrantFlowControl()) {
479478
auto newMaxData = bytesRead_ + kDefaultWTReceiveWindow;
480479
recvFlowController_.grant(newMaxData);
481480
tp_.sendWTMaxData(newMaxData);

proxygen/lib/http/webtransport/WebTransportImpl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ class WebTransportImpl : public WebTransport {
370370

371371
void onWebTransportStopSending(HTTPCodec::StreamID id, uint32_t errorCode);
372372

373-
void maybeGrantFlowControl();
373+
void maybeGrantFlowControl(uint64_t bytesRead);
374374
[[nodiscard]] bool shouldGrantFlowControl() const;
375375

376376
void setFlowControlLimits(

0 commit comments

Comments
 (0)