From 6418d9a528eb9f1d428340ace3a276c0d4b26e44 Mon Sep 17 00:00:00 2001 From: Denis Yaroshevskiy Date: Fri, 18 Sep 2026 01:29:49 -0700 Subject: [PATCH] Fix multi-layer virtual function calls. (#643) Summary: Pull Request resolved: https://github.com/facebook/proxygen/pull/643 Differential Revision: D120517427 --- proxygen/lib/http/codec/HQControlCodec.h | 5 +- proxygen/lib/http/codec/HQFramedCodec.h | 14 --- proxygen/lib/http/codec/HQMultiCodec.h | 6 +- proxygen/lib/http/codec/HQStreamCodec.h | 5 +- proxygen/lib/http/codec/HTTP1xCodec.h | 11 +- proxygen/lib/http/codec/HTTP2Codec.h | 8 +- proxygen/lib/http/codec/HTTPBinaryCodec.h | 11 +- proxygen/lib/http/codec/HTTPCodec.h | 35 ++++-- proxygen/lib/http/codec/HTTPCodecFilter.cpp | 20 +--- proxygen/lib/http/codec/HTTPCodecFilter.h | 107 +++++++++++++++--- proxygen/lib/http/codec/HTTPParallelCodec.h | 13 --- proxygen/lib/http/codec/test/MockHTTPCodec.h | 15 ++- proxygen/lib/http/codec/test/TestUtils.cpp | 11 +- .../http/connpool/test/SessionPoolTest.cpp | 9 +- .../connpool/test/SessionPoolTestFixture.h | 25 ++-- proxygen/lib/http/session/CMakeLists.txt | 1 + proxygen/lib/http/session/HTTPSession.cpp | 9 +- .../session/test/HTTPSessionBenchmark.cpp | 14 +-- .../http/session/test/HTTPTransactionMocks.h | 19 ++-- .../session/test/HTTPUpstreamSessionTest.cpp | 43 +++---- .../session/test/MockCodecDownstreamTest.cpp | 17 +-- 21 files changed, 224 insertions(+), 174 deletions(-) diff --git a/proxygen/lib/http/codec/HQControlCodec.h b/proxygen/lib/http/codec/HQControlCodec.h index 75d8469f2e..85234603e1 100644 --- a/proxygen/lib/http/codec/HQControlCodec.h +++ b/proxygen/lib/http/codec/HQControlCodec.h @@ -67,8 +67,9 @@ class HQControlCodec // HTTPCodec API bool isWaitingToDrain() const override; - CodecProtocol getProtocol() const override { - return CodecProtocol::HQ; + HTTPCodecTraits getTraits() const override { + return HTTPCodecTraits{.protocol = CodecProtocol::HQ, + .direction = transportDirection_}; } size_t onIngress(const folly::IOBuf& /*buf*/) override { diff --git a/proxygen/lib/http/codec/HQFramedCodec.h b/proxygen/lib/http/codec/HQFramedCodec.h index 8f5870f57a..95591d8620 100644 --- a/proxygen/lib/http/codec/HQFramedCodec.h +++ b/proxygen/lib/http/codec/HQFramedCodec.h @@ -36,22 +36,12 @@ class HQFramedCodec : public HTTPCodec { // HTTPCodec API - // Only implemented in the Stream Codec - CodecProtocol getProtocol() const override { - LOG(FATAL) << __func__ << " not supported on this codec"; - folly::assume_unreachable(); - } - // Only implemented in the Stream Codec const std::string& getUserAgent() const override { LOG(FATAL) << __func__ << " not supported on this codec"; folly::assume_unreachable(); } - TransportDirection getTransportDirection() const override { - return transportDirection_; - } - // Stream multiplexing handled at the transport HTTPCodec::StreamID createStream() override { LOG(FATAL) << __func__ << " not supported on this codec"; @@ -93,10 +83,6 @@ class HQFramedCodec : public HTTPCodec { return false; } - bool supportsParallelRequests() const override { - return false; - } - // no connection preface for HQ size_t generateConnectionPreface(folly::IOBufQueue& /*writeBuf*/) override { LOG(FATAL) << __func__ << " not supported on this codec"; diff --git a/proxygen/lib/http/codec/HQMultiCodec.h b/proxygen/lib/http/codec/HQMultiCodec.h index 04c2bdad7f..2e18b60e2c 100644 --- a/proxygen/lib/http/codec/HQMultiCodec.h +++ b/proxygen/lib/http/codec/HQMultiCodec.h @@ -164,8 +164,10 @@ class HQMultiCodec : public HQControlCodec { return res; } - bool supportsParallelRequests() const override { - return true; + HTTPCodecTraits getTraits() const override { + return HTTPCodecTraits{.protocol = CodecProtocol::HQ, + .direction = transportDirection_, + .supportsParallelRequests = true}; } size_t generateConnectionPreface(folly::IOBufQueue& /*writeBuf*/) override { diff --git a/proxygen/lib/http/codec/HQStreamCodec.h b/proxygen/lib/http/codec/HQStreamCodec.h index 82a4a1dcb5..889601e80b 100644 --- a/proxygen/lib/http/codec/HQStreamCodec.h +++ b/proxygen/lib/http/codec/HQStreamCodec.h @@ -58,8 +58,9 @@ class HQStreamCodec return streamId_; } - CodecProtocol getProtocol() const override { - return CodecProtocol::HQ; + HTTPCodecTraits getTraits() const override { + return HTTPCodecTraits{.protocol = CodecProtocol::HQ, + .direction = transportDirection_}; } const std::string& getUserAgent() const override { diff --git a/proxygen/lib/http/codec/HTTP1xCodec.h b/proxygen/lib/http/codec/HTTP1xCodec.h index ab1b1f1cad..4b9b80ccb3 100644 --- a/proxygen/lib/http/codec/HTTP1xCodec.h +++ b/proxygen/lib/http/codec/HTTP1xCodec.h @@ -37,17 +37,15 @@ class HTTP1xCodec : public HTTPCodec { static HTTP1xCodec makeResponseCodec(bool mayChunkEgress); // HTTPCodec API - CodecProtocol getProtocol() const override { - return CodecProtocol::HTTP_1_1; + HTTPCodecTraits getTraits() const override { + return HTTPCodecTraits{.protocol = CodecProtocol::HTTP_1_1, + .direction = transportDirection_}; } const std::string& getUserAgent() const override { return userAgent_; } - TransportDirection getTransportDirection() const override { - return transportDirection_; - } StreamID createStream() override; void setCallback(Callback* callback) override { callback_ = callback; @@ -80,9 +78,6 @@ class HTTP1xCodec : public HTTPCodec { bool closeOnEgressComplete() const override { return !isEgressBusy() && !isReusable(); } - bool supportsParallelRequests() const override { - return false; - } bool supportsPushTransactions() const override { return false; } diff --git a/proxygen/lib/http/codec/HTTP2Codec.h b/proxygen/lib/http/codec/HTTP2Codec.h index bf24a9f08a..896d02a0d3 100644 --- a/proxygen/lib/http/codec/HTTP2Codec.h +++ b/proxygen/lib/http/codec/HTTP2Codec.h @@ -35,8 +35,12 @@ class HTTP2Codec ~HTTP2Codec() override; // HTTPCodec API - CodecProtocol getProtocol() const override { - return CodecProtocol::HTTP_2; + HTTPCodecTraits getTraits() const override { + return HTTPCodecTraits{.protocol = CodecProtocol::HTTP_2, + .direction = transportDirection_, + .supportsParallelRequests = true, + .supportsSessionFlowControl = true, + .supportsStreamFlowControl = true}; } const std::string& getUserAgent() const override { diff --git a/proxygen/lib/http/codec/HTTPBinaryCodec.h b/proxygen/lib/http/codec/HTTPBinaryCodec.h index c1f0615047..1850588955 100644 --- a/proxygen/lib/http/codec/HTTPBinaryCodec.h +++ b/proxygen/lib/http/codec/HTTPBinaryCodec.h @@ -64,17 +64,15 @@ class HTTPBinaryCodec : public HTTPCodec { HTTPBinaryCodec(HTTPBinaryCodec&&) = default; // HTTPCodec API - CodecProtocol getProtocol() const override { - return CodecProtocol::HTTP_BINARY; + HTTPCodecTraits getTraits() const override { + return HTTPCodecTraits{.protocol = CodecProtocol::HTTP_BINARY, + .direction = transportDirection_}; } const std::string& getUserAgent() const override { return userAgent_; } - TransportDirection getTransportDirection() const override { - return transportDirection_; - } StreamID createStream() override { return 0; } @@ -111,9 +109,6 @@ class HTTPBinaryCodec : public HTTPCodec { bool closeOnEgressComplete() const override { return !isEgressBusy() && !isReusable(); } - bool supportsParallelRequests() const override { - return false; - } bool supportsPushTransactions() const override { return false; } diff --git a/proxygen/lib/http/codec/HTTPCodec.h b/proxygen/lib/http/codec/HTTPCodec.h index 34d680fdcd..9c076963ee 100644 --- a/proxygen/lib/http/codec/HTTPCodec.h +++ b/proxygen/lib/http/codec/HTTPCodec.h @@ -28,6 +28,19 @@ class HTTPMessage; class HTTPTransactionHandler; class HTTPErrorPage; +/** + * Codec properties that never change after construction. + */ +struct HTTPCodecTraits { + CodecProtocol protocol{}; + TransportDirection direction{}; + bool supportsParallelRequests{false}; + bool supportsSessionFlowControl{false}; + bool supportsStreamFlowControl{false}; + + bool operator==(const HTTPCodecTraits&) const = default; +}; + /** * Interface for a parser&generator that can translate between an internal * representation of an HTTP request and a wire format. The details of the @@ -334,11 +347,15 @@ class HTTPCodec { return defaultCompressionInfo; } + [[nodiscard]] virtual HTTPCodecTraits getTraits() const = 0; + /** * Gets the session protocol currently used by the codec. This can be * mapped to a string for logging and diagnostic use. */ - [[nodiscard]] virtual CodecProtocol getProtocol() const = 0; + [[nodiscard]] CodecProtocol getProtocol() const { + return getTraits().protocol; + } /** * Gets the user agent string of the client. Thus, it is only meaningful for a @@ -353,20 +370,22 @@ class HTTPCodec { * DOWNSTREAM if the codec receives requests from clients or * UPSTREAM if the codec sends requests to servers. */ - [[nodiscard]] virtual TransportDirection getTransportDirection() const = 0; + [[nodiscard]] TransportDirection getTransportDirection() const { + return getTraits().direction; + } /** * Returns true iff this codec supports per stream flow control */ - [[nodiscard]] virtual bool supportsStreamFlowControl() const { - return false; + [[nodiscard]] bool supportsStreamFlowControl() const { + return getTraits().supportsStreamFlowControl; } /** * Returns true iff this codec supports session level flow control */ - [[nodiscard]] virtual bool supportsSessionFlowControl() const { - return false; + [[nodiscard]] bool supportsSessionFlowControl() const { + return getTraits().supportsSessionFlowControl; } /** @@ -442,7 +461,9 @@ class HTTPCodec { * Check whether the codec supports the processing of multiple * requests in parallel. */ - [[nodiscard]] virtual bool supportsParallelRequests() const = 0; + [[nodiscard]] bool supportsParallelRequests() const { + return getTraits().supportsParallelRequests; + } /** * Check whether the codec supports pushing resources from server to diff --git a/proxygen/lib/http/codec/HTTPCodecFilter.cpp b/proxygen/lib/http/codec/HTTPCodecFilter.cpp index 073dc00f79..ef6e90ba4a 100644 --- a/proxygen/lib/http/codec/HTTPCodecFilter.cpp +++ b/proxygen/lib/http/codec/HTTPCodecFilter.cpp @@ -127,26 +127,14 @@ CompressionInfo PassThroughHTTPCodecFilter::getCompressionInfo() const { return call_->getCompressionInfo(); } -CodecProtocol PassThroughHTTPCodecFilter::getProtocol() const { - return call_->getProtocol(); +HTTPCodecTraits PassThroughHTTPCodecFilter::getTraits() const { + return call_->getTraits(); } const std::string& PassThroughHTTPCodecFilter::getUserAgent() const { return call_->getUserAgent(); } -TransportDirection PassThroughHTTPCodecFilter::getTransportDirection() const { - return call_->getTransportDirection(); -} - -bool PassThroughHTTPCodecFilter::supportsStreamFlowControl() const { - return call_->supportsStreamFlowControl(); -} - -bool PassThroughHTTPCodecFilter::supportsSessionFlowControl() const { - return call_->supportsSessionFlowControl(); -} - HTTPCodec::StreamID PassThroughHTTPCodecFilter::createStream() { return call_->createStream(); } @@ -187,10 +175,6 @@ bool PassThroughHTTPCodecFilter::closeOnEgressComplete() const { return call_->closeOnEgressComplete(); } -bool PassThroughHTTPCodecFilter::supportsParallelRequests() const { - return call_->supportsParallelRequests(); -} - bool PassThroughHTTPCodecFilter::supportsPushTransactions() const { return call_->supportsPushTransactions(); } diff --git a/proxygen/lib/http/codec/HTTPCodecFilter.h b/proxygen/lib/http/codec/HTTPCodecFilter.h index bebfd2ad9a..5337eebd62 100644 --- a/proxygen/lib/http/codec/HTTPCodecFilter.h +++ b/proxygen/lib/http/codec/HTTPCodecFilter.h @@ -97,16 +97,10 @@ class PassThroughHTTPCodecFilter : public HTTPCodecFilter { // HTTPCodec methods [[nodiscard]] CompressionInfo getCompressionInfo() const override; - [[nodiscard]] CodecProtocol getProtocol() const override; + [[nodiscard]] HTTPCodecTraits getTraits() const override; [[nodiscard]] const std::string& getUserAgent() const override; - [[nodiscard]] TransportDirection getTransportDirection() const override; - - [[nodiscard]] bool supportsStreamFlowControl() const override; - - [[nodiscard]] bool supportsSessionFlowControl() const override; - StreamID createStream() override; void setCallback(HTTPCodec::Callback* callback) override; @@ -127,8 +121,6 @@ class PassThroughHTTPCodecFilter : public HTTPCodecFilter { [[nodiscard]] bool closeOnEgressComplete() const override; - [[nodiscard]] bool supportsParallelRequests() const override; - [[nodiscard]] bool supportsPushTransactions() const override; size_t generateConnectionPreface(folly::IOBufQueue& writeBuf) override; @@ -218,10 +210,97 @@ class PassThroughHTTPCodecFilter : public HTTPCodecFilter { [[nodiscard]] uint32_t getDefaultWindowSize() const override; }; -using HTTPCodecFilterChain = FilterChain; +class HTTPCodecFilterChain { + using Chain = FilterChain; + + public: + explicit HTTPCodecFilterChain(std::unique_ptr codec) + : chain_(std::move(codec)), traits_(chain_.getChainEnd().getTraits()) { + } + + HTTPCodecFilterChain(const HTTPCodecFilterChain&) = delete; + HTTPCodecFilterChain& operator=(const HTTPCodecFilterChain&) = delete; + HTTPCodecFilterChain(HTTPCodecFilterChain&&) = delete; + HTTPCodecFilterChain& operator=(HTTPCodecFilterChain&&) = delete; + ~HTTPCodecFilterChain() = default; + + [[nodiscard]] CodecProtocol getProtocol() const { + return traits().protocol; + } + + [[nodiscard]] TransportDirection getTransportDirection() const { + return traits().direction; + } + + [[nodiscard]] bool supportsParallelRequests() const { + return traits().supportsParallelRequests; + } + + [[nodiscard]] bool supportsSessionFlowControl() const { + return traits().supportsSessionFlowControl; + } + + [[nodiscard]] bool supportsStreamFlowControl() const { + return traits().supportsStreamFlowControl; + } + + HTTPCodec* operator->() { + return chain_.operator->(); + } + const HTTPCodec* operator->() const { + return chain_.operator->(); + } + + HTTPCodec* call() { + return chain_.call(); + } + + [[nodiscard]] const HTTPCodec& getChainEnd() const { + return chain_.getChainEnd(); + } + + HTTPCodec* getChainEndPtr() { + return chain_.getChainEndPtr(); + } + + void setCallback(HTTPCodec::Callback* callback) { + chain_.setCallback(callback); + } + + template + void add(Args&&... args) { + chain_.add(std::forward(args)...); + } + + template + void addFilters(Filters&&... filters) { + chain_.addFilters(std::forward(filters)...); + } + + template + void foreach (Fn&& fn) { + chain_.foreach (std::forward(fn)); + } + + std::unique_ptr setDestination(std::unique_ptr dest) { + auto old = chain_.setDestination(std::move(dest)); + traits_ = chain_.getChainEnd().getTraits(); + return old; + } + + private: + [[nodiscard]] const HTTPCodecTraits& traits() const { + DCHECK(chain_.getChainEnd().getTraits() == traits_) + << "codec traits changed after the chain cached them"; + return traits_; + } + + Chain chain_; + HTTPCodecTraits traits_; +}; } // namespace proxygen diff --git a/proxygen/lib/http/codec/HTTPParallelCodec.h b/proxygen/lib/http/codec/HTTPParallelCodec.h index aab31b1903..7bbb37b558 100644 --- a/proxygen/lib/http/codec/HTTPParallelCodec.h +++ b/proxygen/lib/http/codec/HTTPParallelCodec.h @@ -30,23 +30,10 @@ class HTTPParallelCodec : public HTTPCodec { public: explicit HTTPParallelCodec(TransportDirection direction); - [[nodiscard]] TransportDirection getTransportDirection() const override { - return transportDirection_; - } - StreamID createStream() override; [[nodiscard]] bool isBusy() const override { return false; } - [[nodiscard]] bool supportsStreamFlowControl() const override { - return true; - } - [[nodiscard]] bool supportsSessionFlowControl() const override { - return true; - } - [[nodiscard]] bool supportsParallelRequests() const override { - return true; - } [[nodiscard]] bool closeOnEgressComplete() const override { return false; } diff --git a/proxygen/lib/http/codec/test/MockHTTPCodec.h b/proxygen/lib/http/codec/test/MockHTTPCodec.h index 6179248dba..3039994ae4 100644 --- a/proxygen/lib/http/codec/test/MockHTTPCodec.h +++ b/proxygen/lib/http/codec/test/MockHTTPCodec.h @@ -22,11 +22,14 @@ namespace proxygen { class MockHTTPCodec : public HTTPCodec { public: - MOCK_METHOD(CodecProtocol, getProtocol, (), (const)); + explicit MockHTTPCodec(HTTPCodecTraits traits = {}) : traits_(traits) { + } + + HTTPCodecTraits getTraits() const override { + return traits_; + } + MOCK_METHOD(const std::string&, getUserAgent, (), (const)); - MOCK_METHOD(TransportDirection, getTransportDirection, (), (const)); - MOCK_METHOD(bool, supportsStreamFlowControl, (), (const)); - MOCK_METHOD(bool, supportsSessionFlowControl, (), (const)); MOCK_METHOD(HTTPCodec::StreamID, createStream, ()); MOCK_METHOD(void, setCallback, (Callback*)); MOCK_METHOD(bool, isBusy, (), (const)); @@ -38,7 +41,6 @@ class MockHTTPCodec : public HTTPCodec { MOCK_METHOD(bool, isReusable, (), (const)); MOCK_METHOD(bool, isWaitingToDrain, (), (const)); MOCK_METHOD(bool, closeOnEgressComplete, (), (const)); - MOCK_METHOD(bool, supportsParallelRequests, (), (const)); MOCK_METHOD(bool, supportsPushTransactions, (), (const)); MOCK_METHOD(void, generateHeader, @@ -124,6 +126,9 @@ class MockHTTPCodec : public HTTPCodec { addPriorityNodes, (PriorityQueue&, folly::IOBufQueue&, uint8_t)); MOCK_METHOD(HTTPCodec::StreamID, mapPriorityToDependency, (uint8_t), (const)); + + private: + HTTPCodecTraits traits_{}; }; class MockHTTPCodecCallback : public HTTPCodec::Callback { diff --git a/proxygen/lib/http/codec/test/TestUtils.cpp b/proxygen/lib/http/codec/test/TestUtils.cpp index dfca9b8ea5..ed7724dec1 100644 --- a/proxygen/lib/http/codec/test/TestUtils.cpp +++ b/proxygen/lib/http/codec/test/TestUtils.cpp @@ -40,14 +40,11 @@ std::unique_ptr makeBuf(uint32_t size) { std::unique_ptr> makeMockParallelCodec( TransportDirection dir) { - auto codec = std::make_unique>(); - EXPECT_CALL(*codec, supportsParallelRequests()) - .WillRepeatedly(testing::Return(true)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(testing::Return(CodecProtocol::HTTP_2)); + auto codec = std::make_unique>( + HTTPCodecTraits{.protocol = CodecProtocol::HTTP_2, + .direction = dir, + .supportsParallelRequests = true}); EXPECT_CALL(*codec, isReusable()).WillRepeatedly(testing::Return(true)); - EXPECT_CALL(*codec, getTransportDirection()) - .WillRepeatedly(testing::Return(dir)); EXPECT_CALL(*codec, getIngressSettings()) .WillRepeatedly(testing::Return(&kDefaultIngressSettings)); return codec; diff --git a/proxygen/lib/http/connpool/test/SessionPoolTest.cpp b/proxygen/lib/http/connpool/test/SessionPoolTest.cpp index f23fb61d71..48674ddb4c 100644 --- a/proxygen/lib/http/connpool/test/SessionPoolTest.cpp +++ b/proxygen/lib/http/connpool/test/SessionPoolTest.cpp @@ -397,14 +397,11 @@ TEST_F(SessionPoolFixture, CloseNotReusable) { // Codec expectations bool reusable = true; - auto codec = std::make_unique>(); - EXPECT_CALL(*codec, getTransportDirection()) - .WillRepeatedly(Return(TransportDirection::UPSTREAM)); + auto codec = std::make_unique>( + HTTPCodecTraits{.protocol = CodecProtocol::HTTP_2, + .direction = TransportDirection::UPSTREAM}); EXPECT_CALL(*codec, createStream()).WillOnce(Return(1)); EXPECT_CALL(*codec, isReusable()).WillRepeatedly(ReturnPointee(&reusable)); - EXPECT_CALL(*codec, supportsParallelRequests()).WillRepeatedly(Return(false)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(Return(CodecProtocol::HTTP_2)); p.putSession(makeSession(std::move(codec))); ASSERT_EQ(p.getNumSessions(), 1); diff --git a/proxygen/lib/http/connpool/test/SessionPoolTestFixture.h b/proxygen/lib/http/connpool/test/SessionPoolTestFixture.h index 0769657958..c63f93240b 100644 --- a/proxygen/lib/http/connpool/test/SessionPoolTestFixture.h +++ b/proxygen/lib/http/connpool/test/SessionPoolTestFixture.h @@ -34,36 +34,27 @@ class MockSessionHolderCallback : public SessionHolder::Callback { MOCK_METHOD(void, addDrainingSession, (HTTPSessionBase*), ()); }; -std::unique_ptr> makeCodecCommon() { +std::unique_ptr> makeCodecCommon( + HTTPCodecTraits traits) { static int txnIdx = 1; - auto codec = std::make_unique>(); - EXPECT_CALL(*codec, getTransportDirection()) - .WillRepeatedly(testing::Return(TransportDirection::UPSTREAM)); + auto codec = std::make_unique>(traits); EXPECT_CALL(*codec, createStream()) .WillRepeatedly(testing::InvokeWithoutArgs([&]() { return txnIdx++; })); EXPECT_CALL(*codec, isReusable()).WillRepeatedly(testing::Return(true)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(testing::Return(CodecProtocol::HTTP_2)); return codec; } std::unique_ptr> makeSerialCodec() { - auto codec = makeCodecCommon(); - EXPECT_CALL(*codec, supportsParallelRequests()) - .WillRepeatedly(testing::Return(false)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(testing::Return(CodecProtocol::HTTP_1_1)); - return codec; + return makeCodecCommon({.protocol = CodecProtocol::HTTP_1_1, + .direction = TransportDirection::UPSTREAM}); } std::unique_ptr> makeParallelCodec() { - auto codec = makeCodecCommon(); - EXPECT_CALL(*codec, supportsParallelRequests()) - .WillRepeatedly(testing::Return(true)); + auto codec = makeCodecCommon({.protocol = CodecProtocol::HTTP_2, + .direction = TransportDirection::UPSTREAM, + .supportsParallelRequests = true}); EXPECT_CALL(*codec, generateRstStream(testing::_, testing::_, testing::_)) .WillRepeatedly(testing::Return(1)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(testing::Return(CodecProtocol::HTTP_2)); return codec; } diff --git a/proxygen/lib/http/session/CMakeLists.txt b/proxygen/lib/http/session/CMakeLists.txt index 8cecd8819b..acf9c30076 100644 --- a/proxygen/lib/http/session/CMakeLists.txt +++ b/proxygen/lib/http/session/CMakeLists.txt @@ -40,6 +40,7 @@ proxygen_add_library(proxygen_http_session_session wangle::wangle_acceptor_managed fizz::fizz Folly::folly_container_f14_hash + Folly::folly_function Folly::folly_intrusive_list Folly::folly_io_async_async_base Folly::folly_io_async_async_socket diff --git a/proxygen/lib/http/session/HTTPSession.cpp b/proxygen/lib/http/session/HTTPSession.cpp index 11d38b52ca..7e7f9f5a9c 100644 --- a/proxygen/lib/http/session/HTTPSession.cpp +++ b/proxygen/lib/http/session/HTTPSession.cpp @@ -134,7 +134,7 @@ HTTPSession::HTTPSession(const WheelTimerInstance& wheelTimer, } void HTTPSession::setupCodec() { - if (!codec_->supportsParallelRequests()) { + if (!codec_.supportsParallelRequests()) { // until we support upstream pipelining maxConcurrentIncomingStreams_ = 1; maxConcurrentOutgoingStreamsRemote_ = isDownstream() ? 0 : 1; @@ -502,8 +502,11 @@ void HTTPSession::processReadData() { readBuf_.pop_front(); } - // We're about to parse, make sure the parser is not paused - codec_->setParserPaused(false); + // Ensure the parser isn't paused before parsing. Parallel codecs never + // pause it, so skip the call for them. + if (!codec_.supportsParallelRequests()) { + codec_->setParserPaused(false); + } size_t bytesParsed = codec_->onIngress(*readBuf_.front()); if (bytesParsed == 0) { // If the codec didn't make any progress with current input, we diff --git a/proxygen/lib/http/session/test/HTTPSessionBenchmark.cpp b/proxygen/lib/http/session/test/HTTPSessionBenchmark.cpp index 307bc3dbf3..149d3fbb63 100644 --- a/proxygen/lib/http/session/test/HTTPSessionBenchmark.cpp +++ b/proxygen/lib/http/session/test/HTTPSessionBenchmark.cpp @@ -96,7 +96,12 @@ class SessionBenchmarkHelper { private: void setup() { - codec_ = new NiceMock(); + codec_ = new NiceMock( + HTTPCodecTraits{.protocol = CodecProtocol::HTTP_2, + .direction = TransportDirection::DOWNSTREAM, + .supportsParallelRequests = true, + .supportsSessionFlowControl = true, + .supportsStreamFlowControl = true}); transport_ = new NiceMock(); transactionTimeouts_ = makeTimeoutSet(&eventBase_); @@ -116,14 +121,7 @@ class SessionBenchmarkHelper { EXPECT_CALL(mockController_, onTransportReady(_)).Times(1); ON_CALL(*codec_, setCallback(_)).WillByDefault(SaveArg<0>(&codecCallback_)); - ON_CALL(*codec_, supportsParallelRequests()).WillByDefault(Return(true)); ON_CALL(*codec_, supportsPushTransactions()).WillByDefault(Return(true)); - ON_CALL(*codec_, getTransportDirection()) - .WillByDefault(Return(TransportDirection::DOWNSTREAM)); - ON_CALL(*codec_, supportsStreamFlowControl()).WillByDefault(Return(true)); - ON_CALL(*codec_, getProtocol()) - .WillByDefault(Return(CodecProtocol::HTTP_2)); - ON_CALL(*codec_, supportsSessionFlowControl()).WillByDefault(Return(true)); ON_CALL(*codec_, getIngressSettings()) .WillByDefault(Return(&ingressSettings_)); ON_CALL(*codec_, isReusable()).WillByDefault(Return(true)); diff --git a/proxygen/lib/http/session/test/HTTPTransactionMocks.h b/proxygen/lib/http/session/test/HTTPTransactionMocks.h index b84c4c2bae..3c191dccf1 100644 --- a/proxygen/lib/http/session/test/HTTPTransactionMocks.h +++ b/proxygen/lib/http/session/test/HTTPTransactionMocks.h @@ -14,6 +14,9 @@ #include #include +#include +#include + namespace proxygen { #if defined(__clang__) && __clang_major__ >= 3 && __clang_minor__ >= 6 @@ -25,7 +28,7 @@ class MockHTTPTransactionTransport : public HTTPTransaction::Transport { public: MockHTTPTransactionTransport() { EXPECT_CALL(*this, getCodecNonConst()) - .WillRepeatedly(testing::ReturnRef(mockCodec_)); + .WillRepeatedly(testing::ReturnRef(*mockCodec_)); } MOCK_METHOD((void), pauseIngress, (HTTPTransaction*), (noexcept)); @@ -196,7 +199,7 @@ class MockHTTPTransactionTransport : public HTTPTransaction::Transport { void setConnectionToken(HTTPTransaction::ConnectionToken token) { EXPECT_CALL(*this, getConnectionTokenNonConst()) - .WillRepeatedly(testing::Return(token)); + .WillRepeatedly(testing::Return(std::move(token))); } MOCK_METHOD((folly::Expected), @@ -281,7 +284,7 @@ class MockHTTPTransactionTransport : public HTTPTransaction::Transport { MOCK_METHOD(void, trackEgressBodyOffset, (uint64_t, ByteEvent::EventFlags)); - MockHTTPCodec mockCodec_; + std::unique_ptr mockCodec_{std::make_unique()}; }; class MockHTTPTransaction : public HTTPTransaction { @@ -320,7 +323,7 @@ class MockHTTPTransaction : public HTTPTransaction { EXPECT_CALL(mockTransport_, getPeerAddressNonConst()) .WillRepeatedly(testing::ReturnRef(defaultAddress_)); EXPECT_CALL(mockTransport_, getCodecNonConst()) - .WillRepeatedly(testing::ReturnRef(mockTransport_.mockCodec_)); + .WillRepeatedly(testing::ReturnRef(*mockTransport_.mockCodec_)); EXPECT_CALL(mockTransport_, getSetupTransportInfoNonConst()) .WillRepeatedly(testing::ReturnRef(setupTransportInfo_)); EXPECT_CALL(mockTransport_, getUnderlyingTransportNonConst()) @@ -425,13 +428,15 @@ class MockHTTPTransaction : public HTTPTransaction { (folly::AsyncTransport::ReplaySafetyCallback*)); MOCK_METHOD(void, updateAndSendPriority, (HTTPPriority)); void enablePush() { - EXPECT_CALL(mockTransport_.mockCodec_, supportsPushTransactions()) + EXPECT_CALL(*mockTransport_.mockCodec_, supportsPushTransactions()) .WillRepeatedly(testing::Return(true)); } void setupCodec(CodecProtocol protocol) { - EXPECT_CALL(mockTransport_.mockCodec_, getProtocol()) - .WillRepeatedly(testing::Return(protocol)); + mockTransport_.mockCodec_ = + std::make_unique(HTTPCodecTraits{.protocol = protocol}); + EXPECT_CALL(mockTransport_, getCodecNonConst()) + .WillRepeatedly(testing::ReturnRef(*mockTransport_.mockCodec_)); } testing::NiceMock mockTransport_; testing::NiceMock mockAsyncTransport_; diff --git a/proxygen/lib/http/session/test/HTTPUpstreamSessionTest.cpp b/proxygen/lib/http/session/test/HTTPUpstreamSessionTest.cpp index 2ae69ffa1b..51700b2d49 100644 --- a/proxygen/lib/http/session/test/HTTPUpstreamSessionTest.cpp +++ b/proxygen/lib/http/session/test/HTTPUpstreamSessionTest.cpp @@ -766,14 +766,11 @@ class HTTP2UpstreamSessionWithVirtualNodesTest : public HTTPUpstreamTest { public: void SetUp() override { - auto codec = std::make_unique>(); + auto codec = std::make_unique>( + HTTPCodecTraits{.protocol = CodecProtocol::HTTP_2, + .direction = TransportDirection::UPSTREAM, + .supportsParallelRequests = true}); codecPtr_ = codec.get(); - EXPECT_CALL(*codec, supportsParallelRequests()) - .WillRepeatedly(Return(true)); - EXPECT_CALL(*codec, getTransportDirection()) - .WillRepeatedly(Return(TransportDirection::UPSTREAM)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(Return(CodecProtocol::HTTP_2)); EXPECT_CALL(*codec, setCallback(_)).WillRepeatedly(SaveArg<0>(&codecCb_)); EXPECT_CALL(*codec, createStream()).WillRepeatedly(Invoke([&] { auto ret = nextOutgoingTxn_; @@ -1524,20 +1521,18 @@ TEST_F(NoFlushUpstreamSessionTest, DeleteTxnOnUnpause) { class MockHTTPUpstreamTest : public HTTPUpstreamTest { public: + HTTPCodecTraits codecTraits_{.protocol = CodecProtocol::HTTP_2, + .direction = TransportDirection::UPSTREAM, + .supportsParallelRequests = true}; + void SetUp() override { - auto codec = std::make_unique>(); + auto codec = std::make_unique>(codecTraits_); codecPtr_ = codec.get(); - EXPECT_CALL(*codec, supportsParallelRequests()) - .WillRepeatedly(Return(true)); - EXPECT_CALL(*codec, getTransportDirection()) - .WillRepeatedly(Return(TransportDirection::UPSTREAM)); EXPECT_CALL(*codec, setCallback(_)).WillRepeatedly(SaveArg<0>(&codecCb_)); EXPECT_CALL(*codec, isReusable()).WillRepeatedly(ReturnPointee(&reusable_)); EXPECT_CALL(*codec, isWaitingToDrain()) .WillRepeatedly(ReturnPointee(&reusable_)); EXPECT_CALL(*codec, getDefaultWindowSize()).WillRepeatedly(Return(65536)); - EXPECT_CALL(*codec, getProtocol()) - .WillRepeatedly(Return(CodecProtocol::HTTP_2)); EXPECT_CALL(*codec, generateGoaway(_, _, _, _)) .WillRepeatedly(Invoke([&](folly::IOBufQueue& writeBuf, HTTPCodec::StreamID lastStream, @@ -1778,10 +1773,14 @@ TEST_F(MockHTTPUpstreamTest, GoawayPreHeaders) { // Session will delete itself after drain completes } -TEST_F(MockHTTPUpstreamTest, NoWindowUpdateOnDrain) { - EXPECT_CALL(*codecPtr_, supportsStreamFlowControl()) - .WillRepeatedly(Return(true)); +class MockHTTPUpstreamFlowControlTest : public MockHTTPUpstreamTest { + public: + MockHTTPUpstreamFlowControlTest() { + codecTraits_.supportsStreamFlowControl = true; + } +}; +TEST_F(MockHTTPUpstreamFlowControlTest, NoWindowUpdateOnDrain) { auto handler = openTransaction(); handler->sendRequest(); @@ -2154,10 +2153,14 @@ TEST_F(MockHTTPUpstreamTest, HeadersThenBodyThenHeaders) { eventBase_.loop(); } -TEST_F(MockHTTP2UpstreamTest, DelayUpstreamWindowUpdate) { - EXPECT_CALL(*codecPtr_, supportsStreamFlowControl()) - .WillRepeatedly(Return(true)); +class MockHTTP2UpstreamFlowControlTest : public MockHTTP2UpstreamTest { + public: + MockHTTP2UpstreamFlowControlTest() { + codecTraits_.supportsStreamFlowControl = true; + } +}; +TEST_F(MockHTTP2UpstreamFlowControlTest, DelayUpstreamWindowUpdate) { auto handler = openTransaction(); handler->txn_->setReceiveWindow(1000000); // One miiiillion diff --git a/proxygen/lib/http/session/test/MockCodecDownstreamTest.cpp b/proxygen/lib/http/session/test/MockCodecDownstreamTest.cpp index 49ea94687d..52b72f764a 100644 --- a/proxygen/lib/http/session/test/MockCodecDownstreamTest.cpp +++ b/proxygen/lib/http/session/test/MockCodecDownstreamTest.cpp @@ -35,7 +35,12 @@ class MockCodecDownstreamTest : public testing::Test { public: MockCodecDownstreamTest() : eventBase_(), - codec_(new StrictMock()), + codec_(new StrictMock( + HTTPCodecTraits{.protocol = CodecProtocol::HTTP_2, + .direction = TransportDirection::DOWNSTREAM, + .supportsParallelRequests = true, + .supportsSessionFlowControl = true, + .supportsStreamFlowControl = true})), transport_(new NiceMock()), transactionTimeouts_(makeTimeoutSet(&eventBase_)) { @@ -58,21 +63,11 @@ class MockCodecDownstreamTest : public testing::Test { EXPECT_CALL(mockController_, onTransportReady(_)); EXPECT_CALL(*codec_, setCallback(_)) .WillRepeatedly(SaveArg<0>(&codecCallback_)); - EXPECT_CALL(*codec_, supportsParallelRequests()) - .WillRepeatedly(Return(true)); EXPECT_CALL(*codec_, supportsPushTransactions()) .WillRepeatedly(Return(true)); - EXPECT_CALL(*codec_, getTransportDirection()) - .WillRepeatedly(Return(TransportDirection::DOWNSTREAM)); EXPECT_CALL(*codec_, getEgressSettings()).Times(AtLeast(1)); - EXPECT_CALL(*codec_, supportsStreamFlowControl()) - .WillRepeatedly(Return(true)); - EXPECT_CALL(*codec_, getProtocol()) - .WillRepeatedly(Return(CodecProtocol::HTTP_2)); EXPECT_CALL(*codec_, getUserAgent()).WillRepeatedly(ReturnRef(userAgent_)); EXPECT_CALL(*codec_, setParserPaused(_)).WillRepeatedly(Return()); - EXPECT_CALL(*codec_, supportsSessionFlowControl()) - .WillRepeatedly(Return(true)); EXPECT_CALL(*codec_, getIngressSettings()) .WillRepeatedly(Return(&kDefaultIngressSettings)); EXPECT_CALL(*codec_, isReusable())