Split WT_INITIAL_MAX_STREAM_DATA_BIDI into LOCAL/REMOTE

Summary:
The h2 WebTransport draft defines three per-stream data limits (UNI, BIDI_LOCAL
and BIDI_REMOTE) but we only have one BIDI setting at `0x2b63`, which is really
BIDI_LOCAL under an older name. With a single value a bidi stream gets whichever
limit happens to be stored, regardless of which endpoint opened it. Rename
`0x2b63` to `WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL`, add `_BIDI_REMOTE` at
`0x2b66`, and split the `WtConfig` fields so `initStreamRecvFc` and
`initStreamSendFc` can pick based on the initiator. As a side effect
`QmuxConnector::makeWtConfig` becomes a straight mapping of the QUIC transport
params it was already reading, instead of discarding two of them.

Reviewed By: hanidamlaj

Differential Revision: D115270998

fbshipit-source-id: 0da72aa6eb1c184bf06f0f59b0c764fd2aa94e93

Author: afrind

proxygen/lib/http/codec/HTTP2Codec.cpp

@@ -762,7 +762,8 @@ ErrorCode HTTP2Codec::handleSettings(const std::deque<SettingPair>& settings) {
       case SettingsId::WT_ENABLED:
       case SettingsId::WT_INITIAL_MAX_DATA:
       case SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI:
-      case SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI:
+      case SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL:
+      case SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE:
       case SettingsId::WT_INITIAL_MAX_STREAMS_UNI:
       case SettingsId::WT_INITIAL_MAX_STREAMS_BIDI:
         break;
@@ -1455,7 +1456,8 @@ size_t HTTP2Codec::generateSettings(folly::IOBufQueue& writeBuf) {
       case SettingsId::MAX_FRAME_SIZE:
       case SettingsId::WT_INITIAL_MAX_DATA:
       case SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI:
-      case SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI:
+      case SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL:
+      case SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE:
       case SettingsId::WT_INITIAL_MAX_STREAMS_UNI:
       case SettingsId::WT_INITIAL_MAX_STREAMS_BIDI:
         break;

proxygen/lib/http/codec/SettingsId.h

@@ -29,9 +29,10 @@ enum class SettingsId : uint64_t {
   WT_ENABLED = 0x2b60,
   WT_INITIAL_MAX_DATA = 0x2b61,
   WT_INITIAL_MAX_STREAM_DATA_UNI = 0x2b62,
-  WT_INITIAL_MAX_STREAM_DATA_BIDI = 0x2b63,
+  WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL = 0x2b63,
   WT_INITIAL_MAX_STREAMS_UNI = 0x2b64,
   WT_INITIAL_MAX_STREAMS_BIDI = 0x2b65,
+  WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE = 0x2b66,
 
   // From HQ
   //_HQ_HEADER_TABLE_SIZE = HQ_SETTINGS_MASK | 1, -- use HEADER_TABLE_SIZE

proxygen/lib/http/codec/test/HTTP2CodecTest.cpp

@@ -1519,7 +1519,8 @@ TEST_F(HTTP2CodecTest, BasicSetting) {
 constexpr auto kWtFlowControlSettings =
     std::array{SettingsId::WT_INITIAL_MAX_DATA,
                SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI,
-               SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI,
+               SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL,
+               SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE,
                SettingsId::WT_INITIAL_MAX_STREAMS_UNI,
                SettingsId::WT_INITIAL_MAX_STREAMS_BIDI};
 
@@ -1600,7 +1601,8 @@ TEST(HTTP2CodecWebTransportTest, SetsEgressH2WebTransportSettings) {
   static constexpr auto kDefaultWtSettings = {
       SettingsId::WT_INITIAL_MAX_DATA,
       SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI,
-      SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI,
+      SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL,
+      SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE,
       SettingsId::WT_INITIAL_MAX_STREAMS_UNI,
       SettingsId::WT_INITIAL_MAX_STREAMS_BIDI};
 

proxygen/lib/http/webtransport/QuicWtSession.cpp

@@ -22,12 +22,14 @@ WtStreamManager::WtConfig createQuicConfig() {
   config.selfMaxStreamsBidi = kMaxVarint;
   config.selfMaxStreamsUni = kMaxVarint;
   config.selfMaxConnData = kMaxVarint;
-  config.selfMaxStreamDataBidi = kMaxWtIngressBuf;
+  config.selfMaxStreamDataBidiLocal = kMaxWtIngressBuf;
+  config.selfMaxStreamDataBidiRemote = kMaxWtIngressBuf;
   config.selfMaxStreamDataUni = kMaxWtIngressBuf;
   config.peerMaxStreamsBidi = kMaxVarint;
   config.peerMaxStreamsUni = kMaxVarint;
   config.peerMaxConnData = kMaxVarint;
-  config.peerMaxStreamDataBidi = kMaxVarint;
+  config.peerMaxStreamDataBidiLocal = kMaxVarint;
+  config.peerMaxStreamDataBidiRemote = kMaxVarint;
   config.peerMaxStreamDataUni = kMaxVarint;
   return config;
 }

proxygen/lib/http/webtransport/WtStreamManager.cpp

@@ -745,13 +745,22 @@ bool WtStreamManager::hasEvent() const noexcept {
 }
 
 uint64_t WtStreamManager::initStreamRecvFc(uint64_t streamId) const noexcept {
-  return isBidi(streamId) ? wtConfig_.selfMaxStreamDataBidi
-                          : wtConfig_.selfMaxStreamDataUni;
+  if (!isBidi(streamId)) {
+    return wtConfig_.selfMaxStreamDataUni;
+  }
+  // our advertised limits are Local/Remote relative to us
+  return isPeer(streamId) ? wtConfig_.selfMaxStreamDataBidiRemote
+                          : wtConfig_.selfMaxStreamDataBidiLocal;
 }
 
 uint64_t WtStreamManager::initStreamSendFc(uint64_t streamId) const noexcept {
-  return isBidi(streamId) ? wtConfig_.peerMaxStreamDataBidi
-                          : wtConfig_.peerMaxStreamDataUni;
+  if (!isBidi(streamId)) {
+    return wtConfig_.peerMaxStreamDataUni;
+  }
+  // the peer's limits are Local/Remote relative to the peer, so a stream the
+  // peer opened is Local to them and one we opened is Remote to them
+  return isPeer(streamId) ? wtConfig_.peerMaxStreamDataBidiLocal
+                          : wtConfig_.peerMaxStreamDataBidiRemote;
 }
 
 void WtStreamManager::erase(uint64_t streamId) noexcept {

proxygen/lib/http/webtransport/WtStreamManager.h

@@ -106,18 +106,21 @@ struct WtStreamManager {
 
   struct WtConfig {
     static constexpr auto kDefaultFc = std::numeric_limits<uint16_t>::max();
-    // values we've advertised to the peer
+    // values we've advertised to the peer; Local/Remote denote which endpoint
+    // opened the bidi stream the limit applies to, relative to the advertiser
     uint64_t selfMaxStreamsBidi{1};
     uint64_t selfMaxStreamsUni{1};
     uint64_t selfMaxConnData{kDefaultFc};
-    uint64_t selfMaxStreamDataBidi{kDefaultFc};
+    uint64_t selfMaxStreamDataBidiLocal{kDefaultFc};
+    uint64_t selfMaxStreamDataBidiRemote{kDefaultFc};
     uint64_t selfMaxStreamDataUni{kDefaultFc};
 
     // values peer has advertised to us
     uint64_t peerMaxStreamsBidi{1};
     uint64_t peerMaxStreamsUni{1};
     uint64_t peerMaxConnData{kDefaultFc};
-    uint64_t peerMaxStreamDataBidi{kDefaultFc};
+    uint64_t peerMaxStreamDataBidiLocal{kDefaultFc};
+    uint64_t peerMaxStreamDataBidiRemote{kDefaultFc};
     uint64_t peerMaxStreamDataUni{kDefaultFc};
 
     // move to a new struct options?

proxygen/lib/http/webtransport/WtUtils.cpp

@@ -74,7 +74,8 @@ void setEgressWtHttpSettings(TransportDirection dir,
   static constexpr auto kMaxDataSettings = {
       SettingsId::WT_INITIAL_MAX_DATA,
       SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI,
-      SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI};
+      SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL,
+      SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE};
   for (auto maxDataSetting : kMaxDataSettings) {
     settings->setIfNotPresent(maxDataSetting, kWtInitMaxData);
   }
@@ -115,8 +116,10 @@ WtStreamManager::WtConfig getWtConfig(const HTTPSettings* ingress,
         ingress->getSetting(SettingsId::WT_INITIAL_MAX_DATA, /*defaultVal=*/0);
     config.peerMaxStreamDataUni = ingress->getSetting(
         SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI, /*defaultVal=*/0);
-    config.peerMaxStreamDataBidi = ingress->getSetting(
-        SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI, /*defaultVal=*/0);
+    config.peerMaxStreamDataBidiLocal = ingress->getSetting(
+        SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL, /*defaultVal=*/0);
+    config.peerMaxStreamDataBidiRemote = ingress->getSetting(
+        SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE, /*defaultVal=*/0);
     config.peerMaxStreamsUni =
         ingress->getSetting(SettingsId::WT_INITIAL_MAX_STREAMS_UNI,
                             /*defaultVal=*/0);
@@ -129,8 +132,10 @@ WtStreamManager::WtConfig getWtConfig(const HTTPSettings* ingress,
         egress->getSetting(SettingsId::WT_INITIAL_MAX_DATA, /*defaultVal=*/0);
     config.selfMaxStreamDataUni = egress->getSetting(
         SettingsId::WT_INITIAL_MAX_STREAM_DATA_UNI, /*defaultVal=*/0);
-    config.selfMaxStreamDataBidi = egress->getSetting(
-        SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI, /*defaultVal=*/0);
+    config.selfMaxStreamDataBidiLocal = egress->getSetting(
+        SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_LOCAL, /*defaultVal=*/0);
+    config.selfMaxStreamDataBidiRemote = egress->getSetting(
+        SettingsId::WT_INITIAL_MAX_STREAM_DATA_BIDI_REMOTE, /*defaultVal=*/0);
     config.selfMaxStreamsUni =
         egress->getSetting(SettingsId::WT_INITIAL_MAX_STREAMS_UNI,
                            /*defaultVal=*/0);
@@ -139,13 +144,15 @@ WtStreamManager::WtConfig getWtConfig(const HTTPSettings* ingress,
                            /*defaultVal=*/0);
   }
 
-  XLOG(DBG6) << config.selfMaxStreamsBidi << "; " << config.selfMaxStreamsUni
-             << "; " << config.selfMaxConnData << "; "
-             << config.selfMaxStreamDataBidi << "; "
-             << config.selfMaxStreamDataUni << "; " << config.peerMaxStreamsBidi
-             << "; " << config.peerMaxStreamsUni << "; "
-             << config.peerMaxConnData << "; " << config.peerMaxStreamDataBidi
-             << "; " << config.peerMaxStreamDataUni;
+  XLOG(DBG6)
+      << config.selfMaxStreamsBidi << "; " << config.selfMaxStreamsUni << "; "
+      << config.selfMaxConnData << "; " << config.selfMaxStreamDataBidiLocal
+      << "; " << config.selfMaxStreamDataBidiRemote << "; "
+      << config.selfMaxStreamDataUni << "; " << config.peerMaxStreamsBidi
+      << "; " << config.peerMaxStreamsUni << "; " << config.peerMaxConnData
+      << "; " << config.peerMaxStreamDataBidiLocal << "; "
+      << config.peerMaxStreamDataBidiRemote << "; "
+      << config.peerMaxStreamDataUni;
 
   return config;
 }
@@ -155,8 +162,10 @@ WtStreamManager::WtConfig getH3WtConfig(const HTTPSettings* ingress,
   WtStreamManager::WtConfig config = getWtConfig(ingress, egress);
   // disables peer&self MaxStreamData(Uni|Bidi) as these are derived from quic
   // transport params
-  config.peerMaxStreamDataBidi = config.peerMaxStreamDataUni =
-      config.selfMaxStreamDataBidi = config.selfMaxStreamDataUni = kMaxVarint;
+  config.peerMaxStreamDataBidiLocal = config.peerMaxStreamDataBidiRemote =
+      config.peerMaxStreamDataUni = config.selfMaxStreamDataBidiLocal =
+          config.selfMaxStreamDataBidiRemote = config.selfMaxStreamDataUni =
+              kMaxVarint;
   return config;
 }
 

proxygen/lib/http/webtransport/test/WtStreamManagerTest.cpp

@@ -550,10 +550,11 @@ TEST(WtStreamManager, GrantConnFlowControlCreditAfterRead) {
 TEST(WtStreamManager, NonDefaultFlowControlValues) {
   WtConfig config{};
   config.peerMaxConnData = 100;
-  config.peerMaxStreamDataBidi = config.peerMaxStreamDataUni = 60;
+  config.peerMaxStreamDataBidiLocal = config.peerMaxStreamDataBidiRemote =
+      config.peerMaxStreamDataUni = 60;
 
-  config.selfMaxConnData = config.selfMaxStreamDataBidi =
-      config.selfMaxStreamDataUni = 100;
+  config.selfMaxConnData = config.selfMaxStreamDataBidiLocal =
+      config.selfMaxStreamDataBidiRemote = config.selfMaxStreamDataUni = 100;
   WtSmEgressCb egressCb;
   WtSmIngressCb ingressCb;
   auto priorityQueue = std::make_unique<quic::HTTPPriorityQueue>();
@@ -605,6 +606,60 @@ TEST(WtStreamManager, NonDefaultFlowControlValues) {
   EXPECT_FALSE(dequeue.fin);
 }
 
+// Bidi stream data limits are Local/Remote relative to whoever advertised
+// them, so which limit applies depends on who opened the stream.
+TEST(WtStreamManager, BidiStreamDataLimitsFollowStreamInitiator) {
+  constexpr uint64_t kSelfLocal = 50;
+  constexpr uint64_t kSelfRemote = 90;
+  constexpr uint64_t kPeerLocal = 30;
+  constexpr uint64_t kPeerRemote = 70;
+  constexpr uint64_t kNotTheLimit = 1000;
+
+  WtConfig config{};
+  config.selfMaxStreamsBidi = config.peerMaxStreamsBidi = 4;
+  config.selfMaxConnData = config.peerMaxConnData = kNotTheLimit;
+  config.selfMaxStreamDataBidiLocal = kSelfLocal;
+  config.selfMaxStreamDataBidiRemote = kSelfRemote;
+  config.peerMaxStreamDataBidiLocal = kPeerLocal;
+  config.peerMaxStreamDataBidiRemote = kPeerRemote;
+
+  WtSmEgressCb egressCb;
+  WtSmIngressCb ingressCb;
+  auto priorityQueue = std::make_unique<quic::HTTPPriorityQueue>();
+  WtStreamManager streamManager{
+      detail::WtDir::Client, config, egressCb, ingressCb, *priorityQueue};
+
+  auto selfBidi = streamManager.createBidiHandle(); // 0x00
+  CHECK(selfBidi.readHandle && selfBidi.writeHandle);
+  auto peerBidi = streamManager.getOrCreateBidiHandle(0x01);
+  CHECK(peerBidi.readHandle && peerBidi.writeHandle);
+
+  // egress is bounded by the peer's limits: the stream we opened is Remote to
+  // the peer, the one it opened is Local to it
+  constexpr auto kMoreThanAnyLimit = 200;
+  constexpr auto kNoCap = std::numeric_limits<uint64_t>::max();
+  selfBidi.writeHandle->writeStreamData(
+      makeBuf(kMoreThanAnyLimit), /*fin=*/false, /*byteEventCallback=*/nullptr);
+  auto dequeued = streamManager.dequeue(*selfBidi.writeHandle, kNoCap);
+  EXPECT_EQ(dequeued.data->computeChainDataLength(), kPeerRemote);
+
+  peerBidi.writeHandle->writeStreamData(
+      makeBuf(kMoreThanAnyLimit), /*fin=*/false, /*byteEventCallback=*/nullptr);
+  dequeued = streamManager.dequeue(*peerBidi.writeHandle, kNoCap);
+  EXPECT_EQ(dequeued.data->computeChainDataLength(), kPeerLocal);
+
+  // ingress is bounded by our own limits, mirrored: the stream we opened is
+  // Local to us
+  EXPECT_TRUE(streamManager.enqueue(*selfBidi.readHandle,
+                                    {makeBuf(kSelfLocal), /*fin=*/false}));
+  EXPECT_FALSE(
+      streamManager.enqueue(*selfBidi.readHandle, {makeBuf(1), /*fin=*/false}));
+  EXPECT_TRUE(streamManager.enqueue(*peerBidi.readHandle,
+                                    {makeBuf(kSelfRemote), /*fin=*/false}));
+  EXPECT_FALSE(
+      streamManager.enqueue(*peerBidi.readHandle, {makeBuf(1), /*fin=*/false}));
+}
+
 TEST(WtStreamManager, ResetStreamReleasesConnFlowControl) {
   WtConfig config{.selfMaxStreamsUni = 10};
   WtSmEgressCb egressCb;

proxygen/lib/transport/qmux/QmuxConnector.cpp

@@ -192,12 +192,14 @@ WtStreamManager::WtConfig makeWtConfig(
       .selfMaxStreamsBidi = selfParams.initialMaxStreamsBidi,
       .selfMaxStreamsUni = selfParams.initialMaxStreamsUni,
       .selfMaxConnData = selfParams.initialMaxData,
-      .selfMaxStreamDataBidi = selfParams.initialMaxStreamDataBidiLocal,
+      .selfMaxStreamDataBidiLocal = selfParams.initialMaxStreamDataBidiLocal,
+      .selfMaxStreamDataBidiRemote = selfParams.initialMaxStreamDataBidiRemote,
       .selfMaxStreamDataUni = selfParams.initialMaxStreamDataUni,
       .peerMaxStreamsBidi = peerParams.initialMaxStreamsBidi,
       .peerMaxStreamsUni = peerParams.initialMaxStreamsUni,
       .peerMaxConnData = peerParams.initialMaxData,
-      .peerMaxStreamDataBidi = peerParams.initialMaxStreamDataBidiRemote,
+      .peerMaxStreamDataBidiLocal = peerParams.initialMaxStreamDataBidiLocal,
+      .peerMaxStreamDataBidiRemote = peerParams.initialMaxStreamDataBidiRemote,
       .peerMaxStreamDataUni = peerParams.initialMaxStreamDataUni};
 }
 

proxygen/lib/transport/qmux/test/QmuxConnectorTest.cpp

@@ -163,11 +163,15 @@ TEST(MakeWtConfigTest, MapsParamsCorrectly) {
 
   EXPECT_EQ(cfg.selfMaxStreamsBidi, self.initialMaxStreamsBidi);
   EXPECT_EQ(cfg.selfMaxConnData, self.initialMaxData);
-  EXPECT_EQ(cfg.selfMaxStreamDataBidi, self.initialMaxStreamDataBidiLocal);
+  EXPECT_EQ(cfg.selfMaxStreamDataBidiLocal, self.initialMaxStreamDataBidiLocal);
+  EXPECT_EQ(cfg.selfMaxStreamDataBidiRemote,
+            self.initialMaxStreamDataBidiRemote);
   EXPECT_EQ(cfg.selfMaxStreamDataUni, self.initialMaxStreamDataUni);
   EXPECT_EQ(cfg.peerMaxStreamsBidi, peer.initialMaxStreamsBidi);
   EXPECT_EQ(cfg.peerMaxConnData, peer.initialMaxData);
-  EXPECT_EQ(cfg.peerMaxStreamDataBidi, peer.initialMaxStreamDataBidiRemote);
+  EXPECT_EQ(cfg.peerMaxStreamDataBidiLocal, peer.initialMaxStreamDataBidiLocal);
+  EXPECT_EQ(cfg.peerMaxStreamDataBidiRemote,
+            peer.initialMaxStreamDataBidiRemote);
   EXPECT_EQ(cfg.peerMaxStreamDataUni, peer.initialMaxStreamDataUni);
 }