From d98c90c48d53ec581e4b0534adc2c24eaef43c52 Mon Sep 17 00:00:00 2001 From: Konstantin Tsoy Date: Fri, 14 Feb 2025 19:21:05 -0800 Subject: [PATCH] Change onReadData() to return an error Summary: All according to plan: https://fburl.com/gdoc/pebccgi1 Changing function definitions to return errors while still throwing. Reviewed By: sharmafb Differential Revision: D69567329 fbshipit-source-id: 5d40ee32fe185d5674785632a9a13e4cef996988 --- quic/api/QuicTransportBaseLite.cpp | 7 ++++++- quic/api/QuicTransportBaseLite.h | 2 +- quic/api/test/QuicTransportBaseTest.cpp | 8 +++++--- quic/api/test/TestQuicTransport.h | 6 ++++-- quic/client/QuicClientTransportLite.cpp | 5 +++-- quic/client/QuicClientTransportLite.h | 2 +- quic/server/QuicServerTransport.cpp | 6 ++++-- quic/server/QuicServerTransport.h | 2 +- 8 files changed, 25 insertions(+), 13 deletions(-) diff --git a/quic/api/QuicTransportBaseLite.cpp b/quic/api/QuicTransportBaseLite.cpp index 81f5f3561..65114b145 100644 --- a/quic/api/QuicTransportBaseLite.cpp +++ b/quic/api/QuicTransportBaseLite.cpp @@ -126,7 +126,12 @@ void QuicTransportBaseLite::onNetworkData( auto packets = std::move(networkData).movePackets(); for (auto& packet : packets) { - onReadData(peer, std::move(packet)); + auto res = onReadData(peer, std::move(packet)); + if (res.hasError()) { + VLOG(4) << __func__ << " " << res.error().message << " " << *this; + exceptionCloseWhat_ = res.error().message; + return closeImpl(res.error()); + } if (conn_->peerConnectionError) { closeImpl(QuicError( QuicErrorCode(TransportErrorCode::NO_ERROR), "Peer closed")); diff --git a/quic/api/QuicTransportBaseLite.h b/quic/api/QuicTransportBaseLite.h index b5e8b7138..f7701595c 100644 --- a/quic/api/QuicTransportBaseLite.h +++ b/quic/api/QuicTransportBaseLite.h @@ -43,7 +43,7 @@ class QuicTransportBaseLite : virtual public QuicSocketLite, * The sub-class may throw an exception if there was an error in processing * the packet in which case the connection will be closed. */ - virtual void onReadData( + virtual folly::Expected onReadData( const folly::SocketAddress& peer, ReceivedUdpPacket&& udpPacket) = 0; diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index 8442ae9e4..d40261019 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -283,10 +283,11 @@ class TestQuicTransport return lossTimeout_.getTimerCallbackTimeRemaining(); } - void onReadData(const folly::SocketAddress&, ReceivedUdpPacket&& udpPacket) - override { + folly::Expected onReadData( + const folly::SocketAddress&, + ReceivedUdpPacket&& udpPacket) override { if (udpPacket.buf.empty()) { - return; + return folly::unit; } folly::io::Cursor cursor(udpPacket.buf.front()); while (!cursor.isAtEnd()) { @@ -332,6 +333,7 @@ class TestQuicTransport conn_->streamManager->updatePeekableStreams(*stream); } } + return folly::unit; } void writeData() override { diff --git a/quic/api/test/TestQuicTransport.h b/quic/api/test/TestQuicTransport.h index 6aafd731d..59382f4b9 100644 --- a/quic/api/test/TestQuicTransport.h +++ b/quic/api/test/TestQuicTransport.h @@ -85,9 +85,11 @@ class TestQuicTransport return writeLooper_->isPacingScheduled(); } - void onReadData( + folly::Expected onReadData( const folly::SocketAddress& /* peer */, - ReceivedUdpPacket&& /* udpPacket */) noexcept override {} + ReceivedUdpPacket&& /* udpPacket */) noexcept override { + return folly::unit; + } void writeData() override { if (closed) { diff --git a/quic/client/QuicClientTransportLite.cpp b/quic/client/QuicClientTransportLite.cpp index f10026d9e..0a4fea23a 100644 --- a/quic/client/QuicClientTransportLite.cpp +++ b/quic/client/QuicClientTransportLite.cpp @@ -835,7 +835,7 @@ void QuicClientTransportLite::processUdpPacketData( } } -void QuicClientTransportLite::onReadData( +folly::Expected QuicClientTransportLite::onReadData( const folly::SocketAddress& peer, ReceivedUdpPacket&& udpPacket) { if (closeState_ == CloseState::CLOSED) { @@ -845,7 +845,7 @@ void QuicClientTransportLite::onReadData( if (conn_->qLogger) { conn_->qLogger->addPacketDrop(0, kAlreadyClosed); } - return; + return folly::unit; } bool waitingForFirstPacket = !hasReceivedUdpPackets(*conn_); processUdpPacket(peer, std::move(udpPacket)); @@ -873,6 +873,7 @@ void QuicClientTransportLite::onReadData( } maybeSendTransportKnobs(); + return folly::unit; } QuicSocketLite::WriteResult QuicClientTransportLite::writeBufMeta( diff --git a/quic/client/QuicClientTransportLite.h b/quic/client/QuicClientTransportLite.h index c55d4484d..bdd258287 100644 --- a/quic/client/QuicClientTransportLite.h +++ b/quic/client/QuicClientTransportLite.h @@ -156,7 +156,7 @@ class QuicClientTransportLite bool hasZeroRttWriteCipher() const; // From QuicTransportBase - void onReadData( + folly::Expected onReadData( const folly::SocketAddress& peer, ReceivedUdpPacket&& udpPacket) override; void writeData() override; diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 3501dc828..afbdf673d 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -156,7 +156,7 @@ void QuicServerTransport::setServerConnectionIdRejector( } } -void QuicServerTransport::onReadData( +folly::Expected QuicServerTransport::onReadData( const folly::SocketAddress& peer, ReceivedUdpPacket&& udpPacket) { ServerEvents::ReadData readData; @@ -170,7 +170,7 @@ void QuicServerTransport::onReadData( processPendingData(true); if (closeState_ == CloseState::CLOSED) { - return; + return folly::unit; } if (!notifiedRouting_ && routingCb_ && conn_->serverConnectionId) { notifiedRouting_ = true; @@ -202,6 +202,8 @@ void QuicServerTransport::onReadData( maybeIssueConnectionIds(); maybeNotifyTransportReady(); maybeUpdateCongestionControllerFromTicket(); + + return folly::unit; } void QuicServerTransport::accept() { diff --git a/quic/server/QuicServerTransport.h b/quic/server/QuicServerTransport.h index ba413c326..9a70726c4 100644 --- a/quic/server/QuicServerTransport.h +++ b/quic/server/QuicServerTransport.h @@ -137,7 +137,7 @@ class QuicServerTransport void verifiedClientAddress(); // From QuicTransportBase - void onReadData( + folly::Expected onReadData( const folly::SocketAddress& peer, ReceivedUdpPacket&& udpPacket) override; void writeData() override;