Always update max packet size after 0-rtt.

Summary: This is a temporary hack until we properly implement 0-rtt transport parameter updating. Now after 0-rtt we will use the packet size from the peer as the max packet size, if the can ignore setting is set.

Reviewed By: xttjsn

Differential Revision: D23690019

fbshipit-source-id: b4dbf5702e81e52ccd437e0fa68f4d156c7123be

quic/client/QuicClientTransport.cpp

@@ -10,6 +10,7 @@
 
 #include <folly/portability/Sockets.h>
 
+#include <quic/QuicConstants.h>
 #include <quic/api/LoopDetectorCallback.h>
 #include <quic/api/QuicTransportFunctions.h>
 #include <quic/client/handshake/ClientHandshakeFactory.h>
@@ -531,72 +532,89 @@ void QuicClientTransport::processPacketData(
     // We should get transport parameters if we've derived 1-rtt keys and 0-rtt
     // was rejected, or we have derived 1-rtt keys and 0-rtt was never
     // attempted.
-    if ((oneRttKeyDerivationTriggered &&
-         ((zeroRttRejected.has_value() && *zeroRttRejected) ||
-          !zeroRttRejected.has_value()))) {
-      auto originalPeerMaxOffset =
-          conn_->flowControlState.peerAdvertisedMaxOffset;
-      auto originalPeerInitialStreamOffsetBidiLocal =
-          conn_->flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiLocal;
-      auto originalPeerInitialStreamOffsetBidiRemote =
-          conn_->flowControlState
-              .peerAdvertisedInitialMaxStreamOffsetBidiRemote;
-      auto originalPeerInitialStreamOffsetUni =
-          conn_->flowControlState.peerAdvertisedInitialMaxStreamOffsetUni;
-      VLOG(10) << "Client negotiated transport params " << *this;
+    if (oneRttKeyDerivationTriggered) {
       auto serverParams = handshakeLayer->getServerTransportParams();
       if (!serverParams) {
         throw QuicTransportException(
             "No server transport params",
             TransportErrorCode::TRANSPORT_PARAMETER_ERROR);
       }
-      auto maxStreamsBidi = getIntegerParameter(
-          TransportParameterId::initial_max_streams_bidi,
-          serverParams->parameters);
-      auto maxStreamsUni = getIntegerParameter(
-          TransportParameterId::initial_max_streams_uni,
-          serverParams->parameters);
-      processServerInitialParams(
-          *clientConn_, std::move(*serverParams), packetNum);
+      if ((zeroRttRejected.has_value() && *zeroRttRejected) ||
+          !zeroRttRejected.has_value()) {
+        auto originalPeerMaxOffset =
+            conn_->flowControlState.peerAdvertisedMaxOffset;
+        auto originalPeerInitialStreamOffsetBidiLocal =
+            conn_->flowControlState
+                .peerAdvertisedInitialMaxStreamOffsetBidiLocal;
+        auto originalPeerInitialStreamOffsetBidiRemote =
+            conn_->flowControlState
+                .peerAdvertisedInitialMaxStreamOffsetBidiRemote;
+        auto originalPeerInitialStreamOffsetUni =
+            conn_->flowControlState.peerAdvertisedInitialMaxStreamOffsetUni;
+        VLOG(10) << "Client negotiated transport params " << *this;
+        auto maxStreamsBidi = getIntegerParameter(
+            TransportParameterId::initial_max_streams_bidi,
+            serverParams->parameters);
+        auto maxStreamsUni = getIntegerParameter(
+            TransportParameterId::initial_max_streams_uni,
+            serverParams->parameters);
+        processServerInitialParams(
+            *clientConn_, std::move(*serverParams), packetNum);
 
-      cacheServerInitialParams(
-          *clientConn_,
-          conn_->flowControlState.peerAdvertisedMaxOffset,
-          conn_->flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiLocal,
-          conn_->flowControlState
-              .peerAdvertisedInitialMaxStreamOffsetBidiRemote,
-          conn_->flowControlState.peerAdvertisedInitialMaxStreamOffsetUni,
-          maxStreamsBidi.value_or(0),
-          maxStreamsUni.value_or(0));
+        cacheServerInitialParams(
+            *clientConn_,
+            conn_->flowControlState.peerAdvertisedMaxOffset,
+            conn_->flowControlState
+                .peerAdvertisedInitialMaxStreamOffsetBidiLocal,
+            conn_->flowControlState
+                .peerAdvertisedInitialMaxStreamOffsetBidiRemote,
+            conn_->flowControlState.peerAdvertisedInitialMaxStreamOffsetUni,
+            maxStreamsBidi.value_or(0),
+            maxStreamsUni.value_or(0));
 
-      auto& statelessResetToken = clientConn_->statelessResetToken;
-      if (statelessResetToken) {
-        conn_->readCodec->setStatelessResetToken(*statelessResetToken);
-      }
-      if (zeroRttRejected.has_value() && *zeroRttRejected) {
-        // verify that the new flow control parameters are >= the original
-        // transport parameters that were use. This is the easy case. If the
-        // flow control decreases then we are just screwed and we need to have
-        // the app retry the connection. The other parameters can be updated.
-        // TODO: implement undo transport state on retry.
-        if (originalPeerMaxOffset >
-                conn_->flowControlState.peerAdvertisedMaxOffset ||
-            originalPeerInitialStreamOffsetBidiLocal >
-                conn_->flowControlState
-                    .peerAdvertisedInitialMaxStreamOffsetBidiLocal ||
-            originalPeerInitialStreamOffsetBidiRemote >
-                conn_->flowControlState
-                    .peerAdvertisedInitialMaxStreamOffsetBidiRemote ||
+        auto& statelessResetToken = clientConn_->statelessResetToken;
+        if (statelessResetToken) {
+          conn_->readCodec->setStatelessResetToken(*statelessResetToken);
+        }
+        if (zeroRttRejected.has_value() && *zeroRttRejected) {
+          // verify that the new flow control parameters are >= the original
+          // transport parameters that were use. This is the easy case. If the
+          // flow control decreases then we are just screwed and we need to have
+          // the app retry the connection. The other parameters can be updated.
+          // TODO: implement undo transport state on retry.
+          if (originalPeerMaxOffset >
+                  conn_->flowControlState.peerAdvertisedMaxOffset ||
+              originalPeerInitialStreamOffsetBidiLocal >
+                  conn_->flowControlState
+                      .peerAdvertisedInitialMaxStreamOffsetBidiLocal ||
+              originalPeerInitialStreamOffsetBidiRemote >
+                  conn_->flowControlState
+                      .peerAdvertisedInitialMaxStreamOffsetBidiRemote ||
 
-            originalPeerInitialStreamOffsetUni >
-                conn_->flowControlState
-                    .peerAdvertisedInitialMaxStreamOffsetUni) {
-          throw QuicTransportException(
-              "Rejection of zero rtt parameters unsupported",
-              TransportErrorCode::TRANSPORT_PARAMETER_ERROR);
+              originalPeerInitialStreamOffsetUni >
+                  conn_->flowControlState
+                      .peerAdvertisedInitialMaxStreamOffsetUni) {
+            throw QuicTransportException(
+                "Rejection of zero rtt parameters unsupported",
+                TransportErrorCode::TRANSPORT_PARAMETER_ERROR);
+          }
         }
       }
+      // TODO This sucks, but manually update the max packet size until we fix
+      // 0-rtt transport parameters.
+      if (conn_->transportSettings.canIgnorePathMTU &&
+          zeroRttRejected.has_value() && !*zeroRttRejected) {
+        auto updatedPacketSize = getIntegerParameter(
+            TransportParameterId::max_packet_size, serverParams->parameters);
+        updatedPacketSize = std::max<uint64_t>(
+            updatedPacketSize.value_or(kDefaultUDPSendPacketLen),
+            kDefaultUDPSendPacketLen);
+        updatedPacketSize =
+            std::min<uint64_t>(*updatedPacketSize, kDefaultMaxUDPPayload);
+        conn_->udpSendPacketLen = *updatedPacketSize;
+      }
     }
+
     if (zeroRttRejected.has_value() && *zeroRttRejected) {
       // TODO: Make sure the alpn is the same, if not then do a full undo of the
       // state.

quic/client/state/ClientStateMachine.cpp

@@ -234,9 +234,6 @@ void updateTransportParamsFromCachedEarlyParams(
     QuicClientConnectionState& conn,
     const CachedServerTransportParameters& transportParams) {
   conn.peerIdleTimeout = std::chrono::milliseconds(transportParams.idleTimeout);
-  if (conn.transportSettings.canIgnorePathMTU) {
-    conn.udpSendPacketLen = transportParams.maxRecvPacketSize;
-  }
   conn.flowControlState.peerAdvertisedMaxOffset =
       transportParams.initialMaxData;
   conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiLocal =

quic/client/test/ClientStateMachineTest.cpp

@@ -61,7 +61,7 @@ TEST_F(ClientStateMachineTest, TestUpdateTransportParamsFromCachedEarlyParams) {
 
   updateTransportParamsFromCachedEarlyParams(*client_, kParams);
   EXPECT_EQ(client_->peerIdleTimeout, idleTimeout);
-  EXPECT_EQ(client_->udpSendPacketLen, maxRecvPacketSize);
+  EXPECT_NE(client_->udpSendPacketLen, maxRecvPacketSize);
   EXPECT_EQ(client_->flowControlState.peerAdvertisedMaxOffset, initialMaxData);
   EXPECT_EQ(
       client_->flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiLocal,

quic/fizz/client/test/QuicClientTransportTest.cpp

@@ -35,6 +35,7 @@
 #include <quic/samples/echo/EchoHandler.h>
 #include <quic/samples/echo/EchoServer.h>
 #include <quic/state/test/MockQuicStats.h>
+#include "quic/QuicConstants.h"
 
 using namespace testing;
 using namespace folly;
@@ -5366,7 +5367,6 @@ TEST_F(QuicZeroRttClientTest, TestReplaySafeCallback) {
   startClient();
   EXPECT_TRUE(performedValidation);
 
-  auto initialUDPSendPacketLen = client->getConn().udpSendPacketLen;
   socketWrites.clear();
   auto streamId = client->createBidirectionalStream().value();
   client->writeChain(streamId, IOBuf::copyBuffer("hello"), true, false);
@@ -5382,12 +5382,13 @@ TEST_F(QuicZeroRttClientTest, TestReplaySafeCallback) {
   // All the data is still there.
   EXPECT_TRUE(zeroRttPacketsOutstanding());
   // Transport parameters did not change since zero rtt was accepted.
+  // Except for max packet size.
   verifyTransportParameters(
       kDefaultConnectionWindowSize,
       kDefaultStreamWindowSize,
       kDefaultIdleTimeout,
       kDefaultAckDelayExponent,
-      initialUDPSendPacketLen);
+      kDefaultMaxUDPPayload);
 
   EXPECT_CALL(*mockQuicPskCache_, putPsk(hostname_, _))
       .WillOnce(Invoke([=](const std::string&, QuicCachedPsk psk) {