diff --git a/quic/api/QuicAckScheduler.cpp b/quic/api/QuicAckScheduler.cpp index b867fd9a6..4d26f9feb 100644 --- a/quic/api/QuicAckScheduler.cpp +++ b/quic/api/QuicAckScheduler.cpp @@ -63,32 +63,13 @@ Optional AckScheduler::writeNextAcks( Optional ackWriteResult; - bool isAckReceiveTimestampsSupported = - conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer && - conn_.maybePeerAckReceiveTimestampsConfig; - uint64_t peerRequestedTimestampsCount = conn_.maybePeerAckReceiveTimestampsConfig.has_value() ? conn_.maybePeerAckReceiveTimestampsConfig.value() .maxReceiveTimestampsPerAck : 0; - uint64_t extendedAckSupportedAndEnabled = - conn_.peerAdvertisedExtendedAckFeatures & - conn_.transportSettings.enableExtendedAckFeatures; - // Disable the ECN fields if we are not reading them - if (!conn_.transportSettings.readEcnOnIngress) { - extendedAckSupportedAndEnabled &= ~static_cast( - ExtendedAckFeatureMask::ECN_COUNTS); - } - // Disable the receive timestamps fields if we have not regoatiated receive - // timestamps support - if (!isAckReceiveTimestampsSupported || (peerRequestedTimestampsCount == 0)) { - extendedAckSupportedAndEnabled &= ~static_cast( - ExtendedAckFeatureMask::RECEIVE_TIMESTAMPS); - } - - if (extendedAckSupportedAndEnabled > 0) { + if (conn_.negotiatedExtendedAckFeatures > 0) { // The peer supports extended ACKs and we have them enabled. ackWriteResult = writeAckFrame( meta, @@ -97,7 +78,7 @@ Optional AckScheduler::writeNextAcks( conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer .value_or(AckReceiveTimestampsConfig()), peerRequestedTimestampsCount, - extendedAckSupportedAndEnabled); + conn_.negotiatedExtendedAckFeatures); } else if ( conn_.transportSettings.readEcnOnIngress && (meta.ackState.ecnECT0CountReceived || @@ -107,8 +88,7 @@ Optional AckScheduler::writeNextAcks( // frame. In this case, we give ACK_ECN precedence over // ACK_RECEIVE_TIMESTAMPS. ackWriteResult = writeAckFrame(meta, builder, FrameType::ACK_ECN); - } else if ( - isAckReceiveTimestampsSupported && (peerRequestedTimestampsCount > 0)) { + } else if (conn_.negotiatedAckReceiveTimestampSupport) { // Use ACK_RECEIVE_TIMESTAMPS if its enabled on both endpoints AND the // peer requests at least 1 timestamp ackWriteResult = writeAckFrame( diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 29aa09174..7a9f3f1e2 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -2131,4 +2131,35 @@ void maybeScheduleAckForCongestionFeedback( } } +void updateNegotiatedAckFeatures(QuicConnectionStateBase& conn) { + bool isAckReceiveTimestampsSupported = + conn.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer && + conn.maybePeerAckReceiveTimestampsConfig; + + uint64_t peerRequestedTimestampsCount = + conn.maybePeerAckReceiveTimestampsConfig.has_value() + ? conn.maybePeerAckReceiveTimestampsConfig.value() + .maxReceiveTimestampsPerAck + : 0; + + conn.negotiatedAckReceiveTimestampSupport = + isAckReceiveTimestampsSupported && (peerRequestedTimestampsCount > 0); + + conn.negotiatedExtendedAckFeatures = conn.peerAdvertisedExtendedAckFeatures & + conn.transportSettings.enableExtendedAckFeatures; + // Disable the ECN fields if we are not reading them + if (!conn.transportSettings.readEcnOnIngress) { + conn.negotiatedExtendedAckFeatures &= + ~static_cast( + ExtendedAckFeatureMask::ECN_COUNTS); + } + // Disable the receive timestamps fields if we have not regoatiated receive + // timestamps support + if (!conn.negotiatedAckReceiveTimestampSupport) { + conn.negotiatedExtendedAckFeatures &= + ~static_cast( + ExtendedAckFeatureMask::RECEIVE_TIMESTAMPS); + } +} + } // namespace quic diff --git a/quic/api/QuicTransportFunctions.h b/quic/api/QuicTransportFunctions.h index d55d77115..f997b5ac1 100644 --- a/quic/api/QuicTransportFunctions.h +++ b/quic/api/QuicTransportFunctions.h @@ -345,4 +345,5 @@ void maybeAddPacketMark( void maybeScheduleAckForCongestionFeedback( const ReceivedUdpPacket& receivedPacket, AckState& ackState); +void updateNegotiatedAckFeatures(QuicConnectionStateBase& conn); } // namespace quic diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 214266da4..9dbff6e9c 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -2816,6 +2816,7 @@ TEST_F(QuicAckSchedulerTest, WriteAckReceiveTimestampsWhenEnabled) { conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer = AckReceiveTimestampsConfig(); + updateNegotiatedAckFeatures(*conn_); AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -2874,6 +2875,7 @@ TEST_F(QuicAckSchedulerTest, AckExtendedNotSentIfNotSupported) { conn_->transportSettings.enableExtendedAckFeatures = 3; // ECN + ReceiveTimestamps conn_->peerAdvertisedExtendedAckFeatures = 0; + updateNegotiatedAckFeatures(*conn_); AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -2902,6 +2904,7 @@ TEST_F(QuicAckSchedulerTest, AckExtendedNotSentIfNotEnabled) { conn_->transportSettings.enableExtendedAckFeatures = 0; conn_->peerAdvertisedExtendedAckFeatures = 3; // ECN + ReceiveTimestamps; + updateNegotiatedAckFeatures(*conn_); AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -2938,6 +2941,7 @@ TEST_F( conn_->maybePeerAckReceiveTimestampsConfig = AckReceiveTimestampsConfig(); // We don't have an ART config (i.e. we can't sent ART) conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer = none; + updateNegotiatedAckFeatures(*conn_); AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -2972,6 +2976,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedNotSentIfECNFeatureNotSupported) { conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer = AckReceiveTimestampsConfig(); + updateNegotiatedAckFeatures(*conn_); + AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -3009,6 +3015,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedWithAllFeatures) { // We can read ECN conn_->transportSettings.readEcnOnIngress = true; + updateNegotiatedAckFeatures(*conn_); + AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -3046,6 +3054,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedTakesPrecedenceOverECN) { // We can read ECN conn_->transportSettings.readEcnOnIngress = true; + updateNegotiatedAckFeatures(*conn_); + AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); @@ -3083,6 +3093,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedTakesPrecedenceOverReceiveTimestamps) { // We can read ECN conn_->transportSettings.readEcnOnIngress = true; + updateNegotiatedAckFeatures(*conn_); + AckScheduler ackScheduler(*conn_, ackState_); ASSERT_TRUE(ackScheduler.hasPendingAcks()); diff --git a/quic/client/QuicClientTransportLite.cpp b/quic/client/QuicClientTransportLite.cpp index d61169319..1d5ae5246 100644 --- a/quic/client/QuicClientTransportLite.cpp +++ b/quic/client/QuicClientTransportLite.cpp @@ -798,6 +798,8 @@ void QuicClientTransportLite::processUdpPacketData( } } } + updateNegotiatedAckFeatures(*conn_); + // TODO This sucks, but manually update the max packet size until we fix // 0-rtt transport parameters. if (conn_->transportSettings.canIgnorePathMTU && diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index dfab2e7bb..a3f6ce22e 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -445,6 +445,7 @@ void updateHandshakeState(QuicServerConnectionState& conn) { TransportErrorCode::TRANSPORT_PARAMETER_ERROR); } processClientInitialParams(conn, std::move(*clientParams)); + updateNegotiatedAckFeatures(conn); } if (oneRttReadCipher) { if (conn.qLogger) { diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 154d94b6b..a955e4157 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -705,6 +705,12 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { bool peerAdvertisedKnobFrameSupport{false}; ExtendedAckFeatureMaskType peerAdvertisedExtendedAckFeatures{0}; + + // Negotiated ACK related config. These don't change throughout the connection + // so cache them once we've receive the relevant transport parameters. + bool negotiatedAckReceiveTimestampSupport{false}; + ExtendedAckFeatureMaskType negotiatedExtendedAckFeatures{0}; + // Retransmission policies map. folly::F14FastMap retransmissionPolicies;