From d3e8fe246ae9f1e2d6bd2c35d68bc25ef931811b Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Tue, 19 Aug 2025 10:47:24 -0700 Subject: [PATCH] Convert IntervalSet from throwing exceptions to using CHECKs with Expected error handling Summary: This commit converts IntervalSet to use CHECKs instead of throwing exceptions and provides safe tryInsert methods that return quic::Expected for error handling. **Core Problem Solved:** IntervalSet was throwing `std::invalid_argument` exceptions in two scenarios: 1. When constructing an Interval with `start > end` 2. When interval bounds exceed the maximum allowed value This change eliminates exceptions in favor of CHECKs (for internal validation) and Expected-based error handling (for caller validation). **Implementation Details:** **1. IntervalSet Core Changes:** - Replaced `throw std::invalid_argument` with `CHECK_LE` in Interval constructor - Replaced `throw std::invalid_argument` with `CHECK_LE` in `insert(start, end)` - Added `IntervalSetError` enum for error classification - Added `folly::Expected` include **2. Safe API Layer:** - Added `tryInsert(interval)` method returning `Expected` - Added `tryInsert(start, end)` method with pre-validation - Added `tryInsert(point)` method - Added static `Interval::tryCreate()` method for safe interval construction **3. Updated Code:** - **QuicWriteCodec.cpp**: Updated `fillFrameWithPacketReceiveTimestamps` to use `tryInsert` - Returns `QuicError` if interval validation fails - Maintains existing error handling patterns - **QuicTransportFunctions.cpp**: Updated `implicitAckCryptoStream` to use `tryInsert` - Logs errors and continues processing other packets - Robust error handling for crypto stream implicit acks Reviewed By: kvtsoy Differential Revision: D76792362 fbshipit-source-id: 5bd7c22e69a91d60cc41c603a1f2380893f4c8a0 --- quic/api/QuicTransportFunctions.cpp | 10 ++- quic/client/QuicClientTransportLite.cpp | 9 ++- quic/codec/QuicWriteCodec.cpp | 7 +- quic/common/BUCK | 4 + quic/common/IntervalSet-inl.h | 37 ++++++++- quic/common/IntervalSet.h | 39 +++++++-- quic/common/test/IntervalSetTest.cpp | 81 ++++++++++++++++++- quic/common/test/TestUtils.cpp | 7 +- quic/server/state/ServerStateMachine.cpp | 8 +- quic/state/AckHandlers.cpp | 16 +++- quic/state/AckHandlers.h | 3 +- quic/state/BUCK | 4 + quic/state/QuicStateFunctions.cpp | 9 ++- quic/state/QuicStateFunctions.h | 4 +- quic/state/StreamData.h | 7 +- quic/state/stream/StreamSendHandlers.cpp | 14 +++- .../stream/test/StreamStateMachineTest.cpp | 8 +- quic/state/test/QuicStateFunctionsTest.cpp | 36 ++++++--- quic/state/test/StreamDataTest.cpp | 10 +-- 19 files changed, 262 insertions(+), 51 deletions(-) diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index c79168ecf..cbdcd8345 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -2147,7 +2147,15 @@ void implicitAckCryptoStream( implicitAck.implicit = true; for (const auto& op : conn.outstandings.packets) { if (op.packet.header.getPacketNumberSpace() == packetNumSpace) { - ackBlocks.insert(op.packet.header.getPacketSequenceNum()); + auto insertResult = + ackBlocks.tryInsert(op.packet.header.getPacketSequenceNum()); + if (insertResult.hasError()) { + LOG(ERROR) << "Failed to insert packet number into ack blocks: " + << static_cast(insertResult.error()); + // Continue processing other packets - this shouldn't happen in normal + // operation + continue; + } } } if (ackBlocks.empty()) { diff --git a/quic/client/QuicClientTransportLite.cpp b/quic/client/QuicClientTransportLite.cpp index 31f2f8890..ddd1dadc1 100644 --- a/quic/client/QuicClientTransportLite.cpp +++ b/quic/client/QuicClientTransportLite.cpp @@ -410,8 +410,13 @@ quic::Expected QuicClientTransportLite::processUdpPacketData( // Add the packet to the AckState associated with the packet number space. auto& ackState = getAckState(*conn_, pnSpace); - uint64_t distanceFromExpectedPacketNum = - addPacketToAckState(*conn_, ackState, packetNum, udpPacket); + auto addResult = addPacketToAckState(*conn_, ackState, packetNum, udpPacket); + if (!addResult.has_value()) { + return quic::make_unexpected(QuicError( + TransportErrorCode::INTERNAL_ERROR, + "Failed to add packet to ack state")); + } + uint64_t distanceFromExpectedPacketNum = addResult.value(); if (distanceFromExpectedPacketNum > 0) { QUIC_STATS(conn_->statsCallback, onOutOfOrderPacketReceived); } diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index 0b5c29e1d..a3b0ad811 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -347,7 +347,12 @@ fillFrameWithPacketReceiveTimestamps( if (pktsAdded == maxRecvTimestampsToSend) { break; } - receivedPktNumsIntervalSet.insert(recvdPkt.pktNum); + auto insertResult = receivedPktNumsIntervalSet.tryInsert(recvdPkt.pktNum); + if (insertResult.hasError()) { + return quic::make_unexpected(QuicError( + QuicErrorCode(TransportErrorCode::INTERNAL_ERROR), + "Failed to insert packet number into interval set")); + } pktsAdded++; } auto prevPktNum = largestAckedPacketNum; diff --git a/quic/common/BUCK b/quic/common/BUCK index 870b60c73..55e4a2210 100644 --- a/quic/common/BUCK +++ b/quic/common/BUCK @@ -9,9 +9,13 @@ mvfst_cpp_library( "IntervalSet-inl.h", ], exported_deps = [ + ":expected", ":optional", "//folly:likely", ], + exported_external_deps = [ + "glog", + ], ) mvfst_cpp_library( diff --git a/quic/common/IntervalSet-inl.h b/quic/common/IntervalSet-inl.h index fd36ac525..75e1652d3 100644 --- a/quic/common/IntervalSet-inl.h +++ b/quic/common/IntervalSet-inl.h @@ -94,9 +94,7 @@ bool IntervalSet::contains(const T& start, const T& end) template class Container> void IntervalSet::insert(const T& startIt, const T& endIt) { - if (startIt > endIt) { - throw std::invalid_argument("Trying to insert invalid interval"); - } + CHECK_LE(startIt, endIt) << "Trying to insert invalid interval"; insert(Interval(startIt, endIt)); } @@ -128,4 +126,37 @@ template class Container> uint64_t IntervalSet::insertVersion() const { return insertVersion_; } + +template class Container> +Expected IntervalSet::tryInsert( + const Interval& interval) { + // The interval constructor already validated the bounds, so we can safely + // insert + insert(interval); + return {}; +} + +template class Container> +Expected IntervalSet::tryInsert( + const T& start, + const T& end) { + // Validate the bounds before creating the interval + if (start > end) { + return quic::make_unexpected(IntervalSetError::InvalidInterval); + } + if (end > std::numeric_limits::max() - interval_type::unitValue()) { + return quic::make_unexpected(IntervalSetError::IntervalBoundTooLarge); + } + + // Safe to create and insert the interval + insert(Interval(start, end)); + return {}; +} + +template class Container> +Expected IntervalSet::tryInsert( + const T& point) { + return tryInsert(point, point); +} + } // namespace quic diff --git a/quic/common/IntervalSet.h b/quic/common/IntervalSet.h index 9a817d134..338ad98d9 100644 --- a/quic/common/IntervalSet.h +++ b/quic/common/IntervalSet.h @@ -9,17 +9,25 @@ #include #include -#include #include #include -#include #include +#include +#include namespace quic { constexpr uint64_t kDefaultIntervalSetVersion = 0; +/** + * Error codes for IntervalSet operations + */ +enum class IntervalSetError : uint8_t { + InvalidInterval, // start > end + IntervalBoundTooLarge // interval bound exceeds max allowed value +}; + template struct Interval { T start; @@ -30,12 +38,21 @@ struct Interval { } Interval(const T& s, const T& e) : start(s), end(e) { - if (start > end) { - throw std::invalid_argument("Trying to construct invalid interval"); + CHECK_LE(start, end) << "Trying to construct invalid interval"; + CHECK_LE(end, std::numeric_limits::max() - unitValue()) + << "Interval bound too large"; + } + + // Safe constructor that returns Expected instead of CHECKing + [[nodiscard]] static quic::Expected, IntervalSetError> + tryCreate(const T& s, const T& e) { + if (s > e) { + return quic::make_unexpected(IntervalSetError::InvalidInterval); } - if (end > std::numeric_limits::max() - unitValue()) { - throw std::invalid_argument("Interval bound too large"); + if (e > std::numeric_limits::max() - unitValue()) { + return quic::make_unexpected(IntervalSetError::IntervalBoundTooLarge); } + return Interval{s, e}; } bool operator==(Interval& rhs) const { @@ -95,6 +112,16 @@ class IntervalSet : private Container> { void insert(const T& point); + // Safe versions that return Expected instead of CHECKing + [[nodiscard]] Expected tryInsert( + const Interval& interval); + + [[nodiscard]] Expected tryInsert( + const T& start, + const T& end); + + [[nodiscard]] Expected tryInsert(const T& point); + void withdraw(const Interval& interval); [[nodiscard]] bool contains(const T& start, const T& end) const; diff --git a/quic/common/test/IntervalSetTest.cpp b/quic/common/test/IntervalSetTest.cpp index 78ec10b91..ff5cc9e92 100644 --- a/quic/common/test/IntervalSetTest.cpp +++ b/quic/common/test/IntervalSetTest.cpp @@ -158,9 +158,10 @@ TEST(IntervalSet, insertWithMergeAtEdge) { TEST(IntervalSet, insertBoundTooLarge) { IntervalSet set; - EXPECT_THROW( - set.insert(0, std::numeric_limits::max() - 9), - std::invalid_argument); + // This should CHECK-fail since end - start > 10 + EXPECT_DEATH( + set.insert(0, std::numeric_limits::max() - 9), "Check failed"); + // This should work fine since end - start == 10 set.insert(0, std::numeric_limits::max() - 10); } @@ -402,3 +403,77 @@ TEST(IntervalSet, equalityComparatorNotEqualDiffIntervals2) { EXPECT_FALSE(set1 == set2); EXPECT_TRUE(set1 != set2); } + +TEST(IntervalSet, tryInsertValidInterval) { + IntervalSet set; + auto result = set.tryInsert(1, 5); + EXPECT_TRUE(result.has_value()); + EXPECT_EQ(1, set.size()); + EXPECT_TRUE(set.contains(1, 5)); +} + +TEST(IntervalSet, tryInsertValidPoint) { + IntervalSet set; + auto result = set.tryInsert(10); + EXPECT_TRUE(result.has_value()); + EXPECT_EQ(1, set.size()); + EXPECT_TRUE(set.contains(10, 10)); +} + +TEST(IntervalSet, tryInsertInvalidInterval) { + IntervalSet set; + // start > end should return error + auto result = set.tryInsert(10, 5); + EXPECT_TRUE(result.hasError()); + EXPECT_EQ(IntervalSetError::InvalidInterval, result.error()); + EXPECT_EQ(0, set.size()); +} + +TEST(IntervalSet, tryInsertBoundTooLarge) { + IntervalSet set; + // This should return error instead of CHECK-failing + auto result = set.tryInsert(0, std::numeric_limits::max() - 9); + EXPECT_TRUE(result.hasError()); + EXPECT_EQ(IntervalSetError::IntervalBoundTooLarge, result.error()); + EXPECT_EQ(0, set.size()); + + // This should succeed + auto result2 = set.tryInsert(0, std::numeric_limits::max() - 10); + EXPECT_TRUE(result2.has_value()); + EXPECT_EQ(1, set.size()); +} + +TEST(IntervalSet, tryInsertWithMerging) { + IntervalSet set; + auto result1 = set.tryInsert(1, 3); + EXPECT_TRUE(result1.has_value()); + + auto result2 = set.tryInsert(5, 7); + EXPECT_TRUE(result2.has_value()); + + // Insert overlapping interval + auto result3 = set.tryInsert(2, 6); + EXPECT_TRUE(result3.has_value()); + + EXPECT_EQ(1, set.size()); + EXPECT_TRUE(set.contains(1, 7)); +} + +TEST(IntervalSet, tryCreateInterval) { + // Test the static tryCreate method + auto result1 = Interval::tryCreate(1, 5); + EXPECT_TRUE(result1.has_value()); + EXPECT_EQ(1, result1->start); + EXPECT_EQ(5, result1->end); + + // Test invalid interval + auto result2 = Interval::tryCreate(10, 5); + EXPECT_TRUE(result2.hasError()); + EXPECT_EQ(IntervalSetError::InvalidInterval, result2.error()); + + // Test bound too large + auto result3 = Interval::tryCreate( + 0, std::numeric_limits::max() - 9); + EXPECT_TRUE(result3.hasError()); + EXPECT_EQ(IntervalSetError::IntervalBoundTooLarge, result3.error()); +} diff --git a/quic/common/test/TestUtils.cpp b/quic/common/test/TestUtils.cpp index c012ff73d..df90ec719 100644 --- a/quic/common/test/TestUtils.cpp +++ b/quic/common/test/TestUtils.cpp @@ -541,8 +541,13 @@ void updateAckState( TimePoint receiveTimePoint) { ReceivedUdpPacket packet; packet.timings.receiveTimePoint = receiveTimePoint; - uint64_t distance = + auto addResult = addPacketToAckState(conn, getAckState(conn, pnSpace), packetNum, packet); + if (!addResult.has_value()) { + LOG(FATAL) << "Failed to add packet to ack state in test: " + << static_cast(addResult.error()); + } + uint64_t distance = addResult.value(); updateAckSendStateOnRecvPacket( conn, getAckState(conn, pnSpace), diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index d6eb2a64b..bcdfb8ff1 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -1262,8 +1262,14 @@ quic::Expected onServerReadDataFromOpen( } auto& ackState = getAckState(conn, packetNumberSpace); - uint64_t distanceFromExpectedPacketNum = + auto addResult = addPacketToAckState(conn, ackState, packetNum, readData.udpPacket); + if (!addResult.has_value()) { + return quic::make_unexpected(QuicError( + TransportErrorCode::INTERNAL_ERROR, + "Failed to add packet to ack state")); + } + uint64_t distanceFromExpectedPacketNum = addResult.value(); if (distanceFromExpectedPacketNum > 0) { QUIC_STATS(conn.statsCallback, onOutOfOrderPacketReceived); } diff --git a/quic/state/AckHandlers.cpp b/quic/state/AckHandlers.cpp index 15dcdd57e..02bf05c3c 100644 --- a/quic/state/AckHandlers.cpp +++ b/quic/state/AckHandlers.cpp @@ -210,7 +210,13 @@ quic::Expected processAckFrame( // If we hit a packet which has been declared lost we need to count the // spurious loss and ignore all other processing. if (ackedPacketIterator->declaredLost) { - modifyStateForSpuriousLoss(conn, *ackedPacketIterator); + auto modifyResult = + modifyStateForSpuriousLoss(conn, *ackedPacketIterator); + if (!modifyResult.has_value()) { + return quic::make_unexpected(QuicError( + TransportErrorCode::INTERNAL_ERROR, + "Failed to modify state for spurious loss")); + } QUIC_STATS(conn.statsCallback, onPacketSpuriousLoss); if (spuriousLossEvent) { spuriousLossEvent->addSpuriousPacket( @@ -705,7 +711,7 @@ void updateEcnCountEchoed( std::max(ackState.ecnCECountEchoed, readAckFrame.ecnCECount); } -void modifyStateForSpuriousLoss( +Expected modifyStateForSpuriousLoss( QuicConnectionStateBase& conn, OutstandingPacketWrapper& spuriouslyLostPacket) { CHECK_GT(conn.outstandings.declaredLostCount, 0); @@ -734,8 +740,11 @@ void modifyStateForSpuriousLoss( if (stream) { stream->removeFromLossBuffer( streamFrame->offset, streamFrame->len, streamFrame->fin); - stream->updateAckedIntervals( + auto updateResult = stream->updateAckedIntervals( streamFrame->offset, streamFrame->len, streamFrame->fin); + if (!updateResult.has_value()) { + return quic::make_unexpected(updateResult.error()); + } conn.streamManager->updateWritableStreams(*stream); } } @@ -743,5 +752,6 @@ void modifyStateForSpuriousLoss( } CHECK_GT(conn.outstandings.declaredLostCount, 0); conn.outstandings.declaredLostCount--; + return {}; } } // namespace quic diff --git a/quic/state/AckHandlers.h b/quic/state/AckHandlers.h index 70532e6fa..078a9ecf6 100644 --- a/quic/state/AckHandlers.h +++ b/quic/state/AckHandlers.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -124,7 +125,7 @@ void updateEcnCountEchoed( * Modifies the state in the QuicConnectionStateBase when a packet that * was marked as lost is acked. */ -void modifyStateForSpuriousLoss( +[[nodiscard]] Expected modifyStateForSpuriousLoss( QuicConnectionStateBase& conn, OutstandingPacketWrapper& spuriouslyLostPacket); } // namespace quic diff --git a/quic/state/BUCK b/quic/state/BUCK index f9a7e3fb5..008a05ed6 100644 --- a/quic/state/BUCK +++ b/quic/state/BUCK @@ -132,6 +132,7 @@ mvfst_cpp_library( "//quic/common:buf_accessor", "//quic/common:circular_deque", "//quic/common:expected", + "//quic/common:interval_set", "//quic/common:optional", "//quic/congestion_control:congestion_controller", "//quic/congestion_control:packet_processor", @@ -167,6 +168,7 @@ mvfst_cpp_library( "//quic:constants", "//quic/codec:types", "//quic/common:expected", + "//quic/common:interval_set", "//quic/common:optional", "//quic/congestion_control:congestion_controller", ], @@ -207,6 +209,8 @@ mvfst_cpp_library( "//quic:constants", "//quic/codec:pktbuilder", "//quic/codec:types", + "//quic/common:expected", + "//quic/common:interval_set", "//quic/common:network_data", ], ) diff --git a/quic/state/QuicStateFunctions.cpp b/quic/state/QuicStateFunctions.cpp index 3c0b4fcf7..c05d37c9b 100644 --- a/quic/state/QuicStateFunctions.cpp +++ b/quic/state/QuicStateFunctions.cpp @@ -395,7 +395,7 @@ uint64_t maximumConnectionIdsToIssue(const QuicConnectionStateBase& conn) { return maximumIdsToIssue; } -uint64_t addPacketToAckState( +Expected addPacketToAckState( QuicConnectionStateBase& conn, AckState& ackState, const PacketNum packetNum, @@ -407,7 +407,10 @@ uint64_t addPacketToAckState( ackState.largestRecvdPacketNum = std::max( ackState.largestRecvdPacketNum.value_or(packetNum), packetNum); auto preInsertVersion = ackState.acks.insertVersion(); - ackState.acks.insert(packetNum); + auto insertResult = ackState.acks.tryInsert(packetNum); + if (!insertResult.has_value()) { + return quic::make_unexpected(insertResult.error()); + } if (preInsertVersion == ackState.acks.insertVersion()) { QUIC_STATS(conn.statsCallback, onDuplicatedPacketReceived); } @@ -446,7 +449,7 @@ uint64_t addPacketToAckState( return (packetNum > expectedNextPacket) ? packetNum - expectedNextPacket : expectedNextPacket - packetNum; } else { - return 0; + return uint64_t{0}; } } diff --git a/quic/state/QuicStateFunctions.h b/quic/state/QuicStateFunctions.h index ca9654b79..a1f6aa751 100644 --- a/quic/state/QuicStateFunctions.h +++ b/quic/state/QuicStateFunctions.h @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include @@ -65,7 +67,7 @@ void increaseNextPacketNum( * Update largestReceivedUdpPacketNum in ackState with packetNum. Return the * distance from the next packet number we expect to receive. */ -uint64_t addPacketToAckState( +[[nodiscard]] Expected addPacketToAckState( QuicConnectionStateBase& conn, AckState& ackState, const PacketNum packetNum, diff --git a/quic/state/StreamData.h b/quic/state/StreamData.h index b5e4a827f..6943242e2 100644 --- a/quic/state/StreamData.h +++ b/quic/state/StreamData.h @@ -11,6 +11,8 @@ #include #include +#include +#include #include #include #include @@ -211,7 +213,8 @@ struct QuicStreamLike { // egress packets that contains a *new* STREAM frame for this stream. uint64_t numPacketsTxWithNewData{0}; - void updateAckedIntervals(uint64_t offset, uint64_t len, bool eof) { + [[nodiscard]] Expected + updateAckedIntervals(uint64_t offset, uint64_t len, bool eof) { // When there's an EOF we count the byte of 1 past the end as having been // ACKed, since this is useful for delivery APIs. int lenAdjustment = [eof]() { @@ -224,7 +227,7 @@ struct QuicStreamLike { if (lenAdjustment && len == 0) { LOG(FATAL) << "ACK for empty stream frame with no fin."; } - ackedIntervals.insert(offset, offset + len - lenAdjustment); + return ackedIntervals.tryInsert(offset, offset + len - lenAdjustment); } /* diff --git a/quic/state/stream/StreamSendHandlers.cpp b/quic/state/stream/StreamSendHandlers.cpp index c744c12da..b7f334c0c 100644 --- a/quic/state/stream/StreamSendHandlers.cpp +++ b/quic/state/stream/StreamSendHandlers.cpp @@ -145,10 +145,15 @@ quic::Expected sendAckSMHandler( << " offset=" << ackedBuffer->second->offset << " len=" << ackedBuffer->second->data.chainLength() << " eof=" << ackedBuffer->second->eof << " " << stream.conn; - stream.updateAckedIntervals( + auto updateResult = stream.updateAckedIntervals( ackedBuffer->second->offset, ackedBuffer->second->data.chainLength(), ackedBuffer->second->eof); + if (!updateResult.has_value()) { + return quic::make_unexpected(QuicError( + TransportErrorCode::INTERNAL_ERROR, + "Failed to update acked intervals")); + } stream.retransmissionBuffer.erase(ackedBuffer); } } else { @@ -162,10 +167,15 @@ quic::Expected sendAckSMHandler( << " offset=" << ackedBuffer->second.offset << " len=" << ackedBuffer->second.length << " eof=" << ackedBuffer->second.eof << " " << stream.conn; - stream.updateAckedIntervals( + auto updateResult = stream.updateAckedIntervals( ackedBuffer->second.offset, ackedBuffer->second.length, ackedBuffer->second.eof); + if (!updateResult.has_value()) { + return quic::make_unexpected(QuicError( + TransportErrorCode::INTERNAL_ERROR, + "Failed to update acked intervals")); + } stream.retransmissionBufMetas.erase(ackedBuffer); } } diff --git a/quic/state/stream/test/StreamStateMachineTest.cpp b/quic/state/stream/test/StreamStateMachineTest.cpp index 4241c7a1b..d4568489a 100644 --- a/quic/state/stream/test/StreamStateMachineTest.cpp +++ b/quic/state/stream/test/StreamStateMachineTest.cpp @@ -345,7 +345,7 @@ TEST_F(QuicResetSentStateTest, ReliableRstAckNoReduction) { stream.readBuffer.emplace_back( folly::IOBuf::copyBuffer("One more thing"), 0xABCD, false); RstStreamFrame frame(id, GenericApplicationErrorCode::UNKNOWN, 0); - stream.updateAckedIntervals(0, 3, false); + ASSERT_TRUE(stream.updateAckedIntervals(0, 3, false).has_value()); auto result = sendRstAckSMHandler(stream, 5); ASSERT_FALSE(result.hasError()); @@ -370,7 +370,7 @@ TEST_F(QuicResetSentStateTest, ReliableRstAckReduction) { stream.readBuffer.emplace_back( folly::IOBuf::copyBuffer("One more thing"), 0xABCD, false); RstStreamFrame frame(id, GenericApplicationErrorCode::UNKNOWN, 0); - stream.updateAckedIntervals(0, 1, false); + ASSERT_TRUE(stream.updateAckedIntervals(0, 1, false).has_value()); auto result = sendRstAckSMHandler(stream, 1); ASSERT_FALSE(result.hasError()); @@ -434,7 +434,7 @@ TEST_F(QuicResetSentStateTest, ResetSentToClosedTransition1) { StreamId id = 5; QuicStreamState stream(id, *conn); stream.sendState = StreamSendState::ResetSent; - stream.updateAckedIntervals(0, 5, false); + ASSERT_TRUE(stream.updateAckedIntervals(0, 5, false).has_value()); auto result = sendRstAckSMHandler(stream, 5); ASSERT_FALSE(result.hasError()); EXPECT_EQ(stream.sendState, StreamSendState::Closed); @@ -447,7 +447,7 @@ TEST_F(QuicResetSentStateTest, ResetSentToClosedTransition2) { StreamId id = 5; QuicStreamState stream(id, *conn); stream.sendState = StreamSendState::ResetSent; - stream.updateAckedIntervals(0, 4, false); + ASSERT_TRUE(stream.updateAckedIntervals(0, 4, false).has_value()); auto result = sendRstAckSMHandler(stream, 5); ASSERT_FALSE(result.hasError()); EXPECT_EQ(stream.sendState, StreamSendState::ResetSent); diff --git a/quic/state/test/QuicStateFunctionsTest.cpp b/quic/state/test/QuicStateFunctionsTest.cpp index 874fcfef2..6cd10e5d3 100644 --- a/quic/state/test/QuicStateFunctionsTest.cpp +++ b/quic/state/test/QuicStateFunctionsTest.cpp @@ -73,12 +73,14 @@ TEST_P(AddPacketToAckStateTest, FirstPacketNotOutOfOrder) { /** * We skip setting the getAckState(conn, * GetParam()).largestReceivedUdpPacketNum to simulate that we haven't - * received any packets yet. `addPacketToAckState()` should return false for - * the first packet received. + * received any packets yet. `addPacketToAckState()` should return distance 0 + * for the first packet received. */ PacketNum firstPacket = folly::Random::rand32(1, 100); - EXPECT_FALSE(addPacketToAckState( - conn, getAckState(conn, GetParam()), firstPacket, buildPacketMinimal())); + auto result = addPacketToAckState( + conn, getAckState(conn, GetParam()), firstPacket, buildPacketMinimal()); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result.value(), 0); } TEST_P(AddPacketToAckStateTest, ReceiveNew) { @@ -90,7 +92,8 @@ TEST_P(AddPacketToAckStateTest, ReceiveNew) { PacketNum newReceived = currentLargestReceived + 1; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), newReceived, buildPacketMinimal()); - EXPECT_EQ(distance, 0); + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ(distance.value(), 0); EXPECT_GT( *getAckState(conn, GetParam()).largestRecvdPacketNum, currentLargestReceived); @@ -105,7 +108,8 @@ TEST_P(AddPacketToAckStateTest, ReceiveNewWithGap) { PacketNum newReceived = currentLargestReceived + 3; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), newReceived, buildPacketMinimal()); - EXPECT_EQ(distance, 2); // newReceived is 2 after the expected pkt num + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ(distance.value(), 2); // newReceived is 2 after the expected pkt num EXPECT_GT( *getAckState(conn, GetParam()).largestRecvdPacketNum, currentLargestReceived); @@ -120,7 +124,9 @@ TEST_P(AddPacketToAckStateTest, ReceiveOld) { PacketNum newReceived = currentLargestReceived - 1; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), newReceived, buildPacketMinimal()); - EXPECT_EQ(distance, 2); // newReceived is 2 before the expected pkt num + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ( + distance.value(), 2); // newReceived is 2 before the expected pkt num EXPECT_EQ( *getAckState(conn, GetParam()).largestRecvdPacketNum, currentLargestReceived); @@ -135,7 +141,9 @@ TEST_P(AddPacketToAckStateTest, ReceiveOldWithGap) { PacketNum newReceived = currentLargestReceived - 5; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), newReceived, buildPacketMinimal()); - EXPECT_EQ(distance, 6); // newReceived is 6 before the expected pkt num + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ( + distance.value(), 6); // newReceived is 6 before the expected pkt num EXPECT_EQ( *getAckState(conn, GetParam()).largestRecvdPacketNum, currentLargestReceived); @@ -159,7 +167,8 @@ TEST_P(AddPacketToAckStateTest, ReceiveWithECN) { packet.tosValue = kEcnECT0; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), ++nextPacketNum, packet); - EXPECT_EQ(distance, 0); + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ(distance.value(), 0); // Seen 1 ECT0, 0 ECT1, 0 CE. EXPECT_EQ(getAckState(conn, GetParam()).ecnCECountReceived, 0); @@ -172,7 +181,8 @@ TEST_P(AddPacketToAckStateTest, ReceiveWithECN) { packet.tosValue = kEcnECT1; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), ++nextPacketNum, packet); - EXPECT_EQ(distance, 0); + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ(distance.value(), 0); // Seen 1 ECT0, 1 ECT1, 0 CE. EXPECT_EQ(getAckState(conn, GetParam()).ecnCECountReceived, 0); @@ -185,7 +195,8 @@ TEST_P(AddPacketToAckStateTest, ReceiveWithECN) { packet.tosValue = kEcnCE; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), ++nextPacketNum, packet); - EXPECT_EQ(distance, 0); + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ(distance.value(), 0); // Seen 1 ECT0, 1 ECT1, 1 CE. EXPECT_EQ(getAckState(conn, GetParam()).ecnCECountReceived, 1); @@ -198,7 +209,8 @@ TEST_P(AddPacketToAckStateTest, ReceiveWithECN) { packet.tosValue = kEcnCE; auto distance = addPacketToAckState( conn, getAckState(conn, GetParam()), ++nextPacketNum, packet); - EXPECT_EQ(distance, 0); + ASSERT_TRUE(distance.has_value()); + EXPECT_EQ(distance.value(), 0); // Seen 1 ECT0, 1 ECT1, 2 CE. EXPECT_EQ(getAckState(conn, GetParam()).ecnCECountReceived, 2); diff --git a/quic/state/test/StreamDataTest.cpp b/quic/state/test/StreamDataTest.cpp index 6a799b6a8..d645c1e39 100644 --- a/quic/state/test/StreamDataTest.cpp +++ b/quic/state/test/StreamDataTest.cpp @@ -703,7 +703,7 @@ TEST(StreamDataTest, AllBytesAckedTillNotStartAtZero) { QuicStreamState state(0, qcsb); EXPECT_TRUE(state.ackedIntervals.empty()); - state.updateAckedIntervals(1, 5, false); + ASSERT_TRUE(state.updateAckedIntervals(1, 5, false).has_value()); EXPECT_EQ(state.allBytesAckedTill(5), false); } @@ -712,7 +712,7 @@ TEST(StreamDataTest, AllBytesAckedTillNotEnoughLength) { QuicStreamState state(0, qcsb); EXPECT_TRUE(state.ackedIntervals.empty()); - state.updateAckedIntervals(0, 5, false); + ASSERT_TRUE(state.updateAckedIntervals(0, 5, false).has_value()); EXPECT_EQ(state.allBytesAckedTill(5), false); } @@ -721,7 +721,7 @@ TEST(StreamDataTest, AllBytesAckedPass) { QuicStreamState state(0, qcsb); EXPECT_TRUE(state.ackedIntervals.empty()); - state.updateAckedIntervals(0, 6, false); + ASSERT_TRUE(state.updateAckedIntervals(0, 6, false).has_value()); EXPECT_EQ(state.allBytesAckedTill(5), true); } @@ -730,8 +730,8 @@ TEST(StreamDataTest, AllBytesAckedDisjointIntervals) { QuicStreamState state(0, qcsb); EXPECT_TRUE(state.ackedIntervals.empty()); - state.updateAckedIntervals(0, 2, false); - state.updateAckedIntervals(3, 5, false); + ASSERT_TRUE(state.updateAckedIntervals(0, 2, false).has_value()); + ASSERT_TRUE(state.updateAckedIntervals(3, 5, false).has_value()); EXPECT_EQ(state.allBytesAckedTill(5), false); }