remove SettingsId::ENABLE_EX_HEADERS

Summary: incremental stack of diffs [1/n] to remove ex_transactions from the codebase; this is fully deprecated and following up with code deletion

Reviewed By: jbeshay

Differential Revision: D90792711

fbshipit-source-id: 7d29629f3f3743c53a825894dec7292ec5c96366

Author: hanidamlaj

proxygen/httpserver/HTTPServerAcceptor.cpp

@@ -31,10 +31,6 @@ std::shared_ptr<const AcceptorConfiguration> HTTPServerAcceptor::makeConfig(
   conf->acceptBacklog = opts.listenBacklog;
   conf->maxConcurrentIncomingStreams = opts.maxConcurrentIncomingStreams;
 
-  if (opts.enableExHeaders) {
-    conf->egressSettings.emplace_back(SettingsId::ENABLE_EX_HEADERS, 1);
-  }
-
   if (ipConfig.protocol == HTTPServer::Protocol::HTTP2) {
     conf->plaintextProtocol = http2::kProtocolCleartextString;
   }

proxygen/lib/http/codec/HTTP2Codec.cpp

@@ -210,12 +210,8 @@ ErrorCode HTTP2Codec::parseFrame(folly::io::Cursor& cursor) {
     case http2::FrameType::PUSH_PROMISE:
       return parsePushPromise(cursor);
     case http2::FrameType::EX_HEADERS:
-      if (ingressSettings_.getSetting(SettingsId::ENABLE_EX_HEADERS, 0)) {
-        return parseExHeaders(cursor);
-      } else {
-        VLOG(2) << "EX_HEADERS not enabled, ignoring the frame";
-        break;
-      }
+      VLOG(2) << "EX_HEADERS not enabled, ignoring the frame";
+      break;
     case http2::FrameType::PING:
       return parsePing(cursor);
     case http2::FrameType::GOAWAY:
@@ -849,27 +845,6 @@ ErrorCode HTTP2Codec::handleSettings(const std::deque<SettingPair>& settings) {
         break;
       case SettingsId::MAX_HEADER_LIST_SIZE:
         break;
-      case SettingsId::ENABLE_EX_HEADERS: {
-        auto ptr = egressSettings_.getSetting(SettingsId::ENABLE_EX_HEADERS);
-        if (ptr && ptr->value > 0) {
-          VLOG(4) << getTransportDirectionString(getTransportDirection())
-                  << " got ENABLE_EX_HEADERS=" << setting.second;
-          if (setting.second != 0 && setting.second != 1) {
-            goawayErrorMessage_ =
-                folly::to<string>("GOAWAY error: invalid ENABLE_EX_HEADERS=",
-                                  setting.second,
-                                  " for streamID=",
-                                  curHeader_.stream);
-            VLOG(4) << goawayErrorMessage_;
-            return ErrorCode::PROTOCOL_ERROR;
-          }
-          break;
-        } else {
-          // egress ENABLE_EX_HEADERS is disabled, consider the ingress
-          // ENABLE_EX_HEADERS as unknown setting, and ignore it.
-          continue;
-        }
-      }
       case SettingsId::ENABLE_CONNECT_PROTOCOL:
         if (setting.second > 1) {
           goawayErrorMessage_ = folly::to<string>(
@@ -1650,14 +1625,6 @@ size_t HTTP2Codec::generateSettings(folly::IOBufQueue& writeBuf) {
       case SettingsId::MAX_HEADER_LIST_SIZE:
         headerCodec_.setMaxUncompressed(setting.value);
         break;
-      case SettingsId::ENABLE_EX_HEADERS:
-        CHECK(setting.value == 0 || setting.value == 1);
-        if (setting.value == 0) {
-          continue; // just skip the experimental setting if disabled
-        } else {
-          VLOG(4) << "generating ENABLE_EX_HEADERS=" << setting.value;
-        }
-        break;
       case SettingsId::ENABLE_CONNECT_PROTOCOL:
         if (setting.value == 0) {
           continue;

proxygen/lib/http/codec/HTTP2Codec.h

@@ -133,8 +133,7 @@ class HTTP2Codec
     return ingressSettings_.getSetting(SettingsId::ENABLE_CONNECT_PROTOCOL);
   }
   bool supportsExTransactions() const override {
-    return ingressSettings_.getSetting(SettingsId::ENABLE_EX_HEADERS, 0) &&
-           egressSettings_.getSetting(SettingsId::ENABLE_EX_HEADERS, 0);
+    return false;
   }
   void setHeaderCodecStats(HeaderCodec::Stats* hcStats) override {
     headerCodec_.setStats(hcStats);

proxygen/lib/http/codec/SettingsId.h

@@ -29,7 +29,6 @@ enum class SettingsId : uint64_t {
   THRIFT_CHANNEL_ID_DEPRECATED = 100,
 
   // 0xf000 and 0xffff being reserved for Experimental Use
-  ENABLE_EX_HEADERS = 0xfbfb,
   THRIFT_CHANNEL_ID = 0xf100,
 
   // For secondary authentication in HTTP/2

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

@@ -110,7 +110,6 @@ class HTTP2CodecTestOmitParsePreface : public HTTP2CodecTest {
 };
 
 TEST_F(HTTP2CodecTest, NoExHeaders) {
-  // do not emit ENABLE_EX_HEADERS setting, if disabled
   SetUpUpstreamTest();
 
   EXPECT_EQ(callbacks_.settings, 0);
@@ -125,98 +124,6 @@ TEST_F(HTTP2CodecTest, NoExHeaders) {
   EXPECT_EQ(false, downstreamCodec_.supportsExTransactions());
 }
 
-TEST_F(HTTP2CodecTest, IgnoreExHeadersSetting) {
-  // disable EX_HEADERS on egress
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 0);
-  auto ptr = downstreamCodec_.getEgressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(0, ptr->value);
-
-  ptr = downstreamCodec_.getIngressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(nullptr, ptr);
-  EXPECT_EQ(false, downstreamCodec_.supportsExTransactions());
-
-  // attempt to enable EX_HEADERS on ingress
-  http2::writeSettings(output_,
-                       {SettingPair(SettingsId::ENABLE_EX_HEADERS, 1)});
-  parse();
-
-  EXPECT_EQ(callbacks_.settings, 1);
-  EXPECT_EQ(callbacks_.sessionErrors, 0);
-  ptr = downstreamCodec_.getIngressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(nullptr, ptr);
-  EXPECT_EQ(false, downstreamCodec_.supportsExTransactions());
-
-  // attempt to disable EX_HEADERS on ingress
-  callbacks_.reset();
-  http2::writeSettings(output_,
-                       {SettingPair(SettingsId::ENABLE_EX_HEADERS, 0)});
-  parse();
-
-  EXPECT_EQ(callbacks_.settings, 1);
-  EXPECT_EQ(callbacks_.sessionErrors, 0);
-  ptr = downstreamCodec_.getIngressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(nullptr, ptr);
-  EXPECT_EQ(false, downstreamCodec_.supportsExTransactions());
-}
-
-TEST_F(HTTP2CodecTest, EnableExHeadersSetting) {
-  // enable EX_HEADERS on egress
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 1);
-
-  auto ptr = downstreamCodec_.getEgressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(1, ptr->value);
-
-  ptr = downstreamCodec_.getIngressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(nullptr, ptr);
-  EXPECT_EQ(false, downstreamCodec_.supportsExTransactions());
-
-  // attempt to enable EX_HEADERS on ingress
-  http2::writeSettings(output_,
-                       {SettingPair(SettingsId::ENABLE_EX_HEADERS, 1)});
-  parse();
-
-  EXPECT_EQ(callbacks_.settings, 1);
-  EXPECT_EQ(callbacks_.sessionErrors, 0);
-  ptr = downstreamCodec_.getIngressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(1, ptr->value);
-  EXPECT_EQ(true, downstreamCodec_.supportsExTransactions());
-
-  // attempt to disable EX_HEADERS on ingress
-  callbacks_.reset();
-  http2::writeSettings(output_,
-                       {SettingPair(SettingsId::ENABLE_EX_HEADERS, 0)});
-  parse();
-
-  EXPECT_EQ(callbacks_.settings, 1);
-  EXPECT_EQ(callbacks_.sessionErrors, 0);
-  ptr = downstreamCodec_.getIngressSettings()->getSetting(
-      SettingsId::ENABLE_EX_HEADERS);
-  EXPECT_EQ(0, ptr->value);
-  EXPECT_EQ(false, downstreamCodec_.supportsExTransactions());
-}
-
-TEST_F(HTTP2CodecTest, InvalidExHeadersSetting) {
-  // enable EX_HEADERS on egress
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 1);
-
-  // attempt to set a invalid ENABLE_EX_HEADERS value
-  http2::writeSettings(output_,
-                       {SettingPair(SettingsId::ENABLE_EX_HEADERS, 110)});
-  parse();
-
-  EXPECT_EQ(callbacks_.sessionErrors, 1);
-}
-
 TEST_F(HTTP2CodecTest, BasicHeader) {
   HTTPMessage req = getGetRequest("/guacamole");
   req.getHeaders().add(HTTP_HEADER_USER_AGENT, "coolio");
@@ -257,38 +164,6 @@ TEST_F(HTTP2CodecTest, GenerateExtraHeaders) {
   EXPECT_EQ("u=0", headers.getSingleOrEmpty(HTTP_HEADER_PRIORITY));
 }
 
-TEST_F(HTTP2CodecTest, RequestFromServer) {
-  // this is to test EX_HEADERS frame, which carrys the HTTP request initiated
-  // by server side
-  upstreamCodec_.getEgressSettings()->setSetting(SettingsId::ENABLE_EX_HEADERS,
-                                                 1);
-  SetUpUpstreamTest();
-  proxygen::http2::writeSettings(
-      output_, {{proxygen::SettingsId::ENABLE_EX_HEADERS, 1}});
-
-  HTTPMessage req = getGetRequest("/guacamole");
-  req.getHeaders().add(HTTP_HEADER_USER_AGENT, "coolio");
-  req.getHeaders().add("tab-hdr", "coolio\tv2");
-  // Connection header will get dropped
-  req.getHeaders().add(HTTP_HEADER_CONNECTION, "Love");
-  req.setSecure(true);
-
-  HTTPCodec::StreamID stream = folly::Random::rand32(10, 1024) * 2;
-  HTTPCodec::StreamID controlStream = folly::Random::rand32(10, 1024) * 2 + 1;
-  upstreamCodec_.generateExHeader(
-      output_, stream, req, HTTPCodec::ExAttributes(controlStream, true), true);
-
-  parseUpstream();
-  EXPECT_EQ(controlStream, callbacks_.controlStreamId);
-  EXPECT_TRUE(callbacks_.isUnidirectional);
-  callbacks_.expectMessage(true, 3, "/guacamole");
-  EXPECT_TRUE(callbacks_.msg->isSecure());
-  const auto& headers = callbacks_.msg->getHeaders();
-  EXPECT_EQ("coolio", headers.getSingleOrEmpty(HTTP_HEADER_USER_AGENT));
-  EXPECT_EQ("coolio\tv2", headers.getSingleOrEmpty("tab-hdr"));
-  EXPECT_EQ("www.foo.com", headers.getSingleOrEmpty(HTTP_HEADER_HOST));
-}
-
 TEST_F(HTTP2CodecTestOmitParsePreface, OmitSettingsAfterConnPrefaceError) {
   HTTPMessage req = getGetRequest("/test");
   req.getHeaders().add(HTTP_HEADER_USER_AGENT, "rand-user");
@@ -304,86 +179,6 @@ TEST_F(HTTP2CodecTestOmitParsePreface, OmitSettingsAfterConnPrefaceError) {
             ErrorCode::PROTOCOL_ERROR);
 }
 
-TEST_F(HTTP2CodecTest, ResponseFromClient) {
-  // this is to test EX_HEADERS frame, which carrys the HTTP response replied by
-  // client side
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 1);
-  proxygen::http2::writeSettings(
-      output_, {{proxygen::SettingsId::ENABLE_EX_HEADERS, 1}});
-
-  HTTPMessage resp;
-  resp.setStatusCode(200);
-  resp.setStatusMessage("nifty-nice");
-  resp.getHeaders().add(HTTP_HEADER_CONTENT_TYPE, "x-coolio");
-
-  HTTPCodec::StreamID stream = folly::Random::rand32(10, 1024) * 2;
-  HTTPCodec::StreamID controlStream = folly::Random::rand32(10, 1024) * 2 + 1;
-  downstreamCodec_.generateExHeader(
-      output_,
-      stream,
-      resp,
-      HTTPCodec::ExAttributes(controlStream, true),
-      true);
-
-  parse();
-  EXPECT_EQ(controlStream, callbacks_.controlStreamId);
-  EXPECT_TRUE(callbacks_.isUnidirectional);
-  EXPECT_EQ("OK", callbacks_.msg->getStatusMessage());
-  callbacks_.expectMessage(true, 2, 200);
-  const auto& headers = callbacks_.msg->getHeaders();
-  EXPECT_EQ("OK", callbacks_.msg->getStatusMessage());
-  EXPECT_TRUE(callbacks_.msg->getHeaders().exists(HTTP_HEADER_DATE));
-  EXPECT_EQ("x-coolio", headers.getSingleOrEmpty(HTTP_HEADER_CONTENT_TYPE));
-}
-
-TEST_F(HTTP2CodecTest, ExHeadersWithPriority) {
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 1);
-  proxygen::http2::writeSettings(
-      output_, {{proxygen::SettingsId::ENABLE_EX_HEADERS, 1}});
-
-  auto req = getGetRequest();
-  // Test empty path
-  req.setURL("");
-  upstreamCodec_.generateExHeader(
-      output_, 3, req, HTTPCodec::ExAttributes(1, true));
-
-  parse();
-  EXPECT_EQ(callbacks_.streamErrors, 0);
-  EXPECT_EQ(callbacks_.sessionErrors, 0);
-}
-
-TEST_F(HTTP2CodecTest, DuplicateExHeaders) {
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 1);
-  proxygen::http2::writeSettings(
-      output_, {{proxygen::SettingsId::ENABLE_EX_HEADERS, 1}});
-
-  auto req = getGetRequest();
-  upstreamCodec_.generateExHeader(
-      output_, 3, req, HTTPCodec::ExAttributes(1, true), /*eom=*/false);
-  upstreamCodec_.generateExHeader(
-      output_, 3, req, HTTPCodec::ExAttributes(1, true), /*eom=*/true);
-
-  parse();
-  EXPECT_EQ(callbacks_.streamErrors, 0);
-  EXPECT_EQ(callbacks_.sessionErrors, 1);
-}
-
-TEST_F(HTTP2CodecTest, IgnoreExHeadersIfNotEnabled) {
-  downstreamCodec_.getEgressSettings()->setSetting(
-      SettingsId::ENABLE_EX_HEADERS, 0);
-
-  HTTPMessage req = getGetRequest("/guacamole");
-  downstreamCodec_.generateExHeader(
-      output_, 3, req, HTTPCodec::ExAttributes(1, true));
-
-  parse();
-  EXPECT_EQ(callbacks_.streamErrors, 0);
-  EXPECT_EQ(callbacks_.sessionErrors, 0);
-}
-
 TEST_F(HTTP2CodecTest, BadHeaders) {
   static const std::string v1("GET");
   static const std::string v2("/");
@@ -2289,7 +2084,6 @@ TEST_F(HTTP2CodecTest, TestAllEgressFrameTypeCallbacks) {
           http2::FrameType::GOAWAY,
           http2::FrameType::WINDOW_UPDATE,
           http2::FrameType::CONTINUATION,
-          http2::FrameType::EX_HEADERS,
       };
 
       for (http2::FrameType type : expectedTypes) {
@@ -2333,11 +2127,6 @@ TEST_F(HTTP2CodecTest, TestAllEgressFrameTypeCallbacks) {
 
   upstreamCodec_.generateWindowUpdate(output_, 0, 10);
 
-  HTTPCodec::StreamID stream = folly::Random::rand32(10, 1024) * 2;
-  HTTPCodec::StreamID controlStream = folly::Random::rand32(10, 1024) * 2 + 1;
-  downstreamCodec_.generateExHeader(
-      output_, stream, req, HTTPCodec::ExAttributes(controlStream, true));
-
   // Tests the continuation frame
   req = getBigGetRequest();
   upstreamCodec_.generateHeader(output_, id, req, true /* eom */);

proxygen/lib/http/session/HTTPDownstreamSession.cpp

@@ -23,27 +23,16 @@ void HTTPDownstreamSession::startNow() {
 void HTTPDownstreamSession::setupOnHeadersComplete(HTTPTransaction* txn,
                                                    HTTPMessage* msg) {
   VLOG(5) << "setupOnHeadersComplete txn=" << txn << ", id=" << txn->getID()
-          << ", handlder=" << txn->getHandler() << ", msg=" << msg;
-  if (txn->getHandler()) {
-    // handler is installed before setupOnHeadersComplete callback. It must be
-    // an EX_HEADERS from client side, and ENABLE_EX_HEADERS == 1
-    const auto* settings = codec_->getIngressSettings();
-    CHECK(settings && settings->getSetting(SettingsId::ENABLE_EX_HEADERS, 0));
-    CHECK(txn->getControlStream());
-    return;
-  }
-
+          << ", handler=" << txn->getHandler() << ", msg=" << msg;
   // We need to find a Handler to process the transaction.
   // Note: The handler is responsible for freeing itself
   // when it has finished processing the transaction.  The
   // transaction is responsible for freeing itself when both the
   // ingress and egress messages have completed (or failed).
-  HTTPTransaction::Handler* handler = nullptr;
 
   // In the general case, delegate to the handler factory to generate
   // a handler for the transaction.
-  handler = getController()->getRequestHandler(*txn, msg);
-  CHECK(handler);
+  auto* handler = CHECK_NOTNULL(getController()->getRequestHandler(*txn, msg));
 
   DestructorGuard dg(this);
   txn->setHandler(handler);