From bc5f1f70e5672450615504bb7125342ad503abf7 Mon Sep 17 00:00:00 2001 From: Luca Niccolini Date: Thu, 10 Oct 2019 01:19:45 -0700 Subject: [PATCH] H3 priorities are no more Summary: https://github.com/quicwg/base-drafts/pull/2922 https://github.com/quicwg/base-drafts/issues/2924 Reviewed By: mjoras Differential Revision: D17835762 fbshipit-source-id: b972d08a117de0661b8dce288ac34227ddcd2b2e --- proxygen/lib/http/codec/HQControlCodec.cpp | 16 -- proxygen/lib/http/codec/HQControlCodec.h | 2 - proxygen/lib/http/codec/HQFramedCodec.cpp | 2 - proxygen/lib/http/codec/HQFramedCodec.h | 6 - proxygen/lib/http/codec/HQFramer.cpp | 113 ---------- proxygen/lib/http/codec/HQFramer.h | 80 ------- proxygen/lib/http/codec/HQStreamCodec.cpp | 1 - proxygen/lib/http/codec/HQUtils.cpp | 4 - proxygen/lib/http/codec/SPDYConstants.cpp | 1 - proxygen/lib/http/codec/SettingsId.h | 1 - proxygen/lib/http/codec/test/HQCodecTest.cpp | 74 ------ proxygen/lib/http/codec/test/HQFramerTest.cpp | 213 +----------------- proxygen/lib/http/codec/test/HQFramerTest.h | 12 - .../lib/http/session/HQDownstreamSession.h | 2 - proxygen/lib/http/session/HQSession.cpp | 7 - .../session/test/HQDownstreamSessionTest.cpp | 2 +- 16 files changed, 2 insertions(+), 534 deletions(-) diff --git a/proxygen/lib/http/codec/HQControlCodec.cpp b/proxygen/lib/http/codec/HQControlCodec.cpp index e8defaf4a5..87c48c8725 100644 --- a/proxygen/lib/http/codec/HQControlCodec.cpp +++ b/proxygen/lib/http/codec/HQControlCodec.cpp @@ -60,13 +60,6 @@ ParseResult HQControlCodec::checkFrameAllowed(FrameType type) { return folly::none; } -ParseResult HQControlCodec::parsePriority(Cursor& cursor, - const FrameHeader& header) { - PriorityUpdate outPriority; - auto res = hq::parsePriority(cursor, header, outPriority); - return res; -} - ParseResult HQControlCodec::parseCancelPush(Cursor& cursor, const FrameHeader& header) { PushId outPushId; @@ -91,11 +84,6 @@ ParseResult HQControlCodec::parseSettings(Cursor& cursor, case hq::SettingId::MAX_HEADER_LIST_SIZE: case hq::SettingId::QPACK_BLOCKED_STREAMS: break; - case hq::SettingId::NUM_PLACEHOLDERS: - if (transportDirection_ == TransportDirection::DOWNSTREAM) { - return HTTP3::ErrorCode::HTTP_WRONG_SETTING_DIRECTION; - } - break; default: continue; // ignore unknown settings } @@ -161,10 +149,6 @@ size_t HQControlCodec::generateSettings(folly::IOBufQueue& writeBuf) { case hq::SettingId::MAX_HEADER_LIST_SIZE: case hq::SettingId::QPACK_BLOCKED_STREAMS: break; - case hq::SettingId::NUM_PLACEHOLDERS: - CHECK_NE(setting.value, 0); - CHECK_EQ(transportDirection_, TransportDirection::DOWNSTREAM); - break; } settings.emplace_back(*id, (SettingValue)setting.value); } diff --git a/proxygen/lib/http/codec/HQControlCodec.h b/proxygen/lib/http/codec/HQControlCodec.h index 26d0b931d6..68d800fe38 100644 --- a/proxygen/lib/http/codec/HQControlCodec.h +++ b/proxygen/lib/http/codec/HQControlCodec.h @@ -126,8 +126,6 @@ class HQControlCodec protected: ParseResult checkFrameAllowed(FrameType type) override; - ParseResult parsePriority(folly::io::Cursor& cursor, - const FrameHeader& header) override; ParseResult parseCancelPush(folly::io::Cursor& cursor, const FrameHeader& header) override; ParseResult parseSettings(folly::io::Cursor& cursor, diff --git a/proxygen/lib/http/codec/HQFramedCodec.cpp b/proxygen/lib/http/codec/HQFramedCodec.cpp index 1ed09a22be..c26a3d92df 100644 --- a/proxygen/lib/http/codec/HQFramedCodec.cpp +++ b/proxygen/lib/http/codec/HQFramedCodec.cpp @@ -29,8 +29,6 @@ ParseResult HQFramedCodec::parseFrame(Cursor& cursor) { return parseData(cursor, curHeader_); case hq::FrameType::HEADERS: return parseHeaders(cursor, curHeader_); - case hq::FrameType::PRIORITY: - return parsePriority(cursor, curHeader_); case hq::FrameType::CANCEL_PUSH: return parseCancelPush(cursor, curHeader_); case hq::FrameType::SETTINGS: diff --git a/proxygen/lib/http/codec/HQFramedCodec.h b/proxygen/lib/http/codec/HQFramedCodec.h index b3d62836c7..d8f3c6b3a1 100644 --- a/proxygen/lib/http/codec/HQFramedCodec.h +++ b/proxygen/lib/http/codec/HQFramedCodec.h @@ -305,12 +305,6 @@ class HQFramedCodec : public HTTPCodec { folly::assume_unreachable(); } - virtual ParseResult parsePriority(folly::io::Cursor& /*cursor*/, - const FrameHeader& /*header*/) { - LOG(FATAL) << __func__ << " not supported on this codec"; - folly::assume_unreachable(); - } - virtual ParseResult parseCancelPush(folly::io::Cursor& /*cursor*/, const FrameHeader& /*header*/) { LOG(FATAL) << __func__ << " not supported on this codec"; diff --git a/proxygen/lib/http/codec/HQFramer.cpp b/proxygen/lib/http/codec/HQFramer.cpp index f127afd35b..781ece1a44 100644 --- a/proxygen/lib/http/codec/HQFramer.cpp +++ b/proxygen/lib/http/codec/HQFramer.cpp @@ -63,79 +63,6 @@ ParseResult parseHeaders(folly::io::Cursor& cursor, return folly::none; } -uint8_t encodePriorityFlags(PriorityUpdate priority) { - uint8_t flags = 0x00; - flags |= (priority.prioritizedType << PRIORITIZED_TYPE_POS); - flags |= (priority.dependencyType << DEPENDENCY_TYPE_POS); - if (priority.exclusive) { - flags |= PRIORITY_EXCLUSIVE_MASK; - } - return flags; -} - -bool decodePriorityFlags(uint8_t flags, PriorityUpdate& outPriority) { - outPriority.prioritizedType = static_cast( - (flags & (0x03 << PRIORITIZED_TYPE_POS)) >> PRIORITIZED_TYPE_POS); - outPriority.dependencyType = static_cast( - (flags & (0x03 << DEPENDENCY_TYPE_POS)) >> DEPENDENCY_TYPE_POS); - outPriority.exclusive = (flags & PRIORITY_EXCLUSIVE_MASK); - uint8_t empty = flags & (0x07 << PRIORITY_EMPTY_POS); - if (empty != 0) { - return false; - } - return true; -} - -ParseResult parsePriority(folly::io::Cursor& cursor, - const FrameHeader& header, - PriorityUpdate& outPriority) noexcept { - DCHECK_LE(header.length, cursor.totalLength()); - folly::IOBuf buf; - auto frameLength = header.length; - - if (!cursor.canAdvance(sizeof(uint8_t))) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - - uint8_t flags = cursor.readBE(); - frameLength -= sizeof(uint8_t); - bool res = decodePriorityFlags(flags, outPriority); - if (!res) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - - // A PRIORITY frame that prioritizes the root of the tree is not allowed - if (outPriority.prioritizedType == PriorityElementType::TREE_ROOT) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - - auto prioritizedElementId = quic::decodeQuicInteger(cursor, frameLength); - if (!prioritizedElementId) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - outPriority.prioritizedElementId = prioritizedElementId->first; - frameLength -= prioritizedElementId->second; - - if (outPriority.dependencyType != PriorityElementType::TREE_ROOT) { - auto elementDependencyId = quic::decodeQuicInteger(cursor, frameLength); - if (!elementDependencyId) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - outPriority.elementDependencyId = elementDependencyId->first; - frameLength -= elementDependencyId->second; - } - - if (!cursor.canAdvance(sizeof(uint8_t))) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - outPriority.weight = cursor.readBE(); - frameLength -= sizeof(uint8_t); - if (frameLength != 0) { - return HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY; - } - return folly::none; -} - ParseResult parseCancelPush(folly::io::Cursor& cursor, const FrameHeader& header, PushId& outPushId) noexcept { @@ -174,7 +101,6 @@ decodeSettingValue(folly::io::Cursor& cursor, // unknown ones switch (settingId) { case SettingId::HEADER_TABLE_SIZE: - case SettingId::NUM_PLACEHOLDERS: case SettingId::MAX_HEADER_LIST_SIZE: case SettingId::QPACK_BLOCKED_STREAMS: return value; @@ -317,43 +243,6 @@ WriteResult writeHeaders(IOBufQueue& queue, return writeSimpleFrame(queue, FrameType::HEADERS, std::move(data)); } -WriteResult writePriority(IOBufQueue& queue, PriorityUpdate priority) noexcept { - uint8_t flags = encodePriorityFlags(priority); - - auto prioritizedElementIdSize = - quic::getQuicIntegerSize(priority.prioritizedElementId); - if (prioritizedElementIdSize.hasError()) { - return prioritizedElementIdSize; - } - - - size_t payloadSize = *prioritizedElementIdSize + - 2 * sizeof(uint8_t); - - if (priority.dependencyType != PriorityElementType::TREE_ROOT) { - auto elementDependencyIdSize = - quic::getQuicIntegerSize(priority.elementDependencyId); - if (elementDependencyIdSize.hasError()) { - return elementDependencyIdSize; - } - payloadSize += *elementDependencyIdSize; - } - - const auto headerSize = - writeFrameHeader(queue, FrameType::PRIORITY, payloadSize); - if (headerSize.hasError()) { - return headerSize; - } - QueueAppender appender(&queue, payloadSize); - appender.writeBE(flags); - quic::encodeQuicInteger(priority.prioritizedElementId, appender); - if (priority.dependencyType != PriorityElementType::TREE_ROOT) { - quic::encodeQuicInteger(priority.elementDependencyId, appender); - } - appender.writeBE(priority.weight); - return *headerSize + payloadSize; -} - WriteResult writeCancelPush(folly::IOBufQueue& writeBuf, PushId pushId) noexcept { DCHECK(pushId & kPushIdMask); @@ -452,8 +341,6 @@ const char* getFrameTypeString(FrameType type) { return "DATA"; case FrameType::HEADERS: return "HEADERS"; - case FrameType::PRIORITY: - return "PRIORITY"; case FrameType::CANCEL_PUSH: return "CANCEL_PUSH"; case FrameType::SETTINGS: diff --git a/proxygen/lib/http/codec/HQFramer.h b/proxygen/lib/http/codec/HQFramer.h index 03440efc13..469394ea53 100644 --- a/proxygen/lib/http/codec/HQFramer.h +++ b/proxygen/lib/http/codec/HQFramer.h @@ -53,7 +53,6 @@ using WriteResult = folly::Expected; enum class FrameType : uint64_t { DATA = 0x00, HEADERS = 0x01, - PRIORITY = 0x02, CANCEL_PUSH = 0x03, SETTINGS = 0x04, PUSH_PROMISE = 0x05, @@ -69,60 +68,10 @@ struct FrameHeader { uint64_t length; }; -enum PriorityElementType { - REQUEST_STREAM = 0x00, - PUSH_STREAM = 0x01, - PLACEHOLDER = 0x02, - TREE_ROOT = 0x03, -}; - -// The first byte in a Priority Frame has multiple fields -const uint8_t PRIORITIZED_TYPE_POS = 6; -const uint8_t DEPENDENCY_TYPE_POS = 4; -const uint8_t PRIORITY_EMPTY_POS = 1; -const uint8_t PRIORITY_EXCLUSIVE_MASK = 0x01; - -struct PriorityUpdate { - PriorityUpdate(PriorityElementType pt, - PriorityElementType dt, - bool ex, - uint64_t pe, - uint64_t ed, - uint8_t wt) - : prioritizedType(pt), - dependencyType(dt), - prioritizedElementId(pe), - elementDependencyId(ed), - weight(wt), - exclusive(ex) { - } - - PriorityUpdate() - : prioritizedType(PriorityElementType::REQUEST_STREAM), - dependencyType(PriorityElementType::TREE_ROOT), - prioritizedElementId(0), - elementDependencyId(0), - weight(0), - exclusive(false) { - } - - PriorityElementType prioritizedType; - PriorityElementType dependencyType; - // the prioritized element ID can be a stream ID, a push ID or a placeholder - // ID, based on prioritized Type - uint64_t prioritizedElementId; - // the element dependency ID can be a stream ID, a push ID or a placeholder - // ID, based on dependency Type - uint64_t elementDependencyId; - uint8_t weight; - bool exclusive; -}; - enum class SettingId : uint64_t { HEADER_TABLE_SIZE = 0x01, MAX_HEADER_LIST_SIZE = 0x06, QPACK_BLOCKED_STREAMS = 0x07, - NUM_PLACEHOLDERS = 0x09, }; using SettingValue = uint64_t; @@ -165,23 +114,6 @@ ParseResult parseHeaders(folly::io::Cursor& cursor, const FrameHeader& header, std::unique_ptr& outBuf) noexcept; -/** - * This function parses the section of the PRIORITY frame after the common - * frame header. It pulls header.length bytes from the cursor, so it is the - * caller's responsibility to ensure there is enough data available. - * It fetches priority information both from the frame payload and from the - * frame header. - * - * @param cursor The cursor to pull data from. - * @param header The frame header for the frame being parsed. - * @param outPriority On success, filled with the priority information - * from this frame. - * @return folly::none for successful parse or the quic application error code. - */ -ParseResult parsePriority(folly::io::Cursor& cursor, - const FrameHeader& header, - PriorityUpdate& outPriority) noexcept; - /** * This function parses the section of the CANCEL_PUSH frame after the common * frame header. It pulls header.length bytes from the cursor, so it is the @@ -307,18 +239,6 @@ WriteResult writeUnframedBytes(folly::IOBufQueue& writeBuf, WriteResult writeHeaders(folly::IOBufQueue& writeBuf, std::unique_ptr data) noexcept; -/** - * Generate an entire PRIORITY frame, including the common frame header. - * - * @param writeBuf The output queue to write to. It may grow or add - * underlying buffers inside this function. - * @param priority The priority depedency information to update the stream with. - * @return The number of bytes written to writeBuf if successful, a quic error - * otherwise - */ -WriteResult writePriority(folly::IOBufQueue& writeBuf, - PriorityUpdate priority) noexcept; - /** * Generate an entire CANCEL_PUSH frame, including the common frame * header. diff --git a/proxygen/lib/http/codec/HQStreamCodec.cpp b/proxygen/lib/http/codec/HQStreamCodec.cpp index 5952570ab1..4e5c1335ea 100644 --- a/proxygen/lib/http/codec/HQStreamCodec.cpp +++ b/proxygen/lib/http/codec/HQStreamCodec.cpp @@ -53,7 +53,6 @@ ParseResult HQStreamCodec::checkFrameAllowed(FrameType type) { case hq::FrameType::SETTINGS: case hq::FrameType::GOAWAY: case hq::FrameType::MAX_PUSH_ID: - case hq::FrameType::PRIORITY: case hq::FrameType::CANCEL_PUSH: return HTTP3::ErrorCode::HTTP_WRONG_STREAM; case hq::FrameType::PUSH_PROMISE: diff --git a/proxygen/lib/http/codec/HQUtils.cpp b/proxygen/lib/http/codec/HQUtils.cpp index 959bda8baf..a89dfb0abb 100644 --- a/proxygen/lib/http/codec/HQUtils.cpp +++ b/proxygen/lib/http/codec/HQUtils.cpp @@ -138,8 +138,6 @@ folly::Optional httpToHqSettingsId(proxygen::SettingsId id) { return hq::SettingId::HEADER_TABLE_SIZE; case proxygen::SettingsId::MAX_HEADER_LIST_SIZE: return hq::SettingId::MAX_HEADER_LIST_SIZE; - case proxygen::SettingsId::_HQ_NUM_PLACEHOLDERS: - return hq::SettingId::NUM_PLACEHOLDERS; case proxygen::SettingsId::_HQ_QPACK_BLOCKED_STREAMS: return hq::SettingId::QPACK_BLOCKED_STREAMS; default: @@ -152,8 +150,6 @@ folly::Optional hqToHttpSettingsId(hq::SettingId id) { switch (id) { case hq::SettingId::HEADER_TABLE_SIZE: return proxygen::SettingsId::HEADER_TABLE_SIZE; - case hq::SettingId::NUM_PLACEHOLDERS: - return proxygen::SettingsId::_HQ_NUM_PLACEHOLDERS; case hq::SettingId::MAX_HEADER_LIST_SIZE: return proxygen::SettingsId::MAX_HEADER_LIST_SIZE; case hq::SettingId::QPACK_BLOCKED_STREAMS: diff --git a/proxygen/lib/http/codec/SPDYConstants.cpp b/proxygen/lib/http/codec/SPDYConstants.cpp index 661284f743..e02f6c472d 100644 --- a/proxygen/lib/http/codec/SPDYConstants.cpp +++ b/proxygen/lib/http/codec/SPDYConstants.cpp @@ -114,7 +114,6 @@ folly::Optional httpToSpdySettingsId( case proxygen::SettingsId::THRIFT_CHANNEL_ID_DEPRECATED: case proxygen::SettingsId::THRIFT_CHANNEL_ID: return folly::none; - case proxygen::SettingsId::_HQ_NUM_PLACEHOLDERS: case proxygen::SettingsId::_HQ_QPACK_BLOCKED_STREAMS: case proxygen::SettingsId::SETTINGS_HTTP_CERT_AUTH: return folly::none; diff --git a/proxygen/lib/http/codec/SettingsId.h b/proxygen/lib/http/codec/SettingsId.h index 26760d5267..2dabb0f756 100644 --- a/proxygen/lib/http/codec/SettingsId.h +++ b/proxygen/lib/http/codec/SettingsId.h @@ -52,7 +52,6 @@ enum class SettingsId : uint64_t { //_HQ_HEADER_TABLE_SIZE = HQ_SETTINGS_MASK | 1, -- use HEADER_TABLE_SIZE //_HQ_MAX_HEADER_LIST_SIZE = HQ_SETTINGS_MASK | 6, -- use MAX_HEADER_LIST_SIZE _HQ_QPACK_BLOCKED_STREAMS = HQ_SETTINGS_MASK | 7, - _HQ_NUM_PLACEHOLDERS = HQ_SETTINGS_MASK | 8, }; using SettingPair = std::pair; diff --git a/proxygen/lib/http/codec/test/HQCodecTest.cpp b/proxygen/lib/http/codec/test/HQCodecTest.cpp index da8cbfad3c..f6a31b6f4a 100644 --- a/proxygen/lib/http/codec/test/HQCodecTest.cpp +++ b/proxygen/lib/http/codec/test/HQCodecTest.cpp @@ -901,9 +901,6 @@ std::string frameParamsToTestName( case FrameType::HEADERS: testName += "Headers"; break; - case FrameType::PRIORITY: - testName += "Priority"; - break; case FrameType::CANCEL_PUSH: testName += "CancelPush"; break; @@ -1008,7 +1005,6 @@ INSTANTIATE_TEST_CASE_P( Values( (FrameAllowedParams){CodecType::DOWNSTREAM, FrameType::DATA, true}, (FrameAllowedParams){CodecType::DOWNSTREAM, FrameType::HEADERS, true}, - (FrameAllowedParams){CodecType::DOWNSTREAM, FrameType::PRIORITY, false}, (FrameAllowedParams){ CodecType::DOWNSTREAM, FrameType::CANCEL_PUSH, false}, (FrameAllowedParams){CodecType::DOWNSTREAM, FrameType::SETTINGS, false}, @@ -1029,8 +1025,6 @@ INSTANTIATE_TEST_CASE_P( CodecType::CONTROL_UPSTREAM, FrameType::DATA, false}, (FrameAllowedParams){ CodecType::CONTROL_UPSTREAM, FrameType::HEADERS, false}, - (FrameAllowedParams){ - CodecType::CONTROL_UPSTREAM, FrameType::PRIORITY, true}, (FrameAllowedParams){ CodecType::CONTROL_UPSTREAM, FrameType::CANCEL_PUSH, true}, (FrameAllowedParams){ @@ -1050,8 +1044,6 @@ INSTANTIATE_TEST_CASE_P( CodecType::CONTROL_DOWNSTREAM, FrameType::DATA, false}, (FrameAllowedParams){ CodecType::CONTROL_DOWNSTREAM, FrameType::HEADERS, false}, - (FrameAllowedParams){ - CodecType::CONTROL_DOWNSTREAM, FrameType::PRIORITY, true}, (FrameAllowedParams){ CodecType::CONTROL_DOWNSTREAM, FrameType::CANCEL_PUSH, true}, (FrameAllowedParams){ @@ -1090,8 +1082,6 @@ INSTANTIATE_TEST_CASE_P( CodecType::H1Q_CONTROL_UPSTREAM, FrameType::DATA, false}, (FrameAllowedParams){ CodecType::H1Q_CONTROL_UPSTREAM, FrameType::HEADERS, false}, - (FrameAllowedParams){ - CodecType::H1Q_CONTROL_UPSTREAM, FrameType::PRIORITY, false}, (FrameAllowedParams){ CodecType::H1Q_CONTROL_UPSTREAM, FrameType::CANCEL_PUSH, false}, (FrameAllowedParams){ @@ -1113,8 +1103,6 @@ INSTANTIATE_TEST_CASE_P( CodecType::H1Q_CONTROL_DOWNSTREAM, FrameType::DATA, false}, (FrameAllowedParams){ CodecType::H1Q_CONTROL_DOWNSTREAM, FrameType::HEADERS, false}, - (FrameAllowedParams){ - CodecType::H1Q_CONTROL_DOWNSTREAM, FrameType::PRIORITY, false}, (FrameAllowedParams){ CodecType::H1Q_CONTROL_DOWNSTREAM, FrameType::CANCEL_PUSH, false}, (FrameAllowedParams){ @@ -1153,8 +1141,6 @@ INSTANTIATE_TEST_CASE_P( CodecType::CONTROL_UPSTREAM, FrameType::DATA, false}, (FrameAllowedParams){ CodecType::CONTROL_UPSTREAM, FrameType::HEADERS, false}, - (FrameAllowedParams){ - CodecType::CONTROL_UPSTREAM, FrameType::PRIORITY, false}, (FrameAllowedParams){ CodecType::CONTROL_UPSTREAM, FrameType::CANCEL_PUSH, false}, (FrameAllowedParams){ @@ -1172,8 +1158,6 @@ INSTANTIATE_TEST_CASE_P( CodecType::CONTROL_DOWNSTREAM, FrameType::DATA, false}, (FrameAllowedParams){ CodecType::CONTROL_DOWNSTREAM, FrameType::HEADERS, false}, - (FrameAllowedParams){ - CodecType::CONTROL_DOWNSTREAM, FrameType::PRIORITY, false}, (FrameAllowedParams){ CodecType::CONTROL_DOWNSTREAM, FrameType::CANCEL_PUSH, false}, (FrameAllowedParams){ @@ -1189,61 +1173,3 @@ INSTANTIATE_TEST_CASE_P( FrameType(*getGreaseId(3434343434)), false}), frameParamsToTestName); - -using HQCodecDeathTest = HQCodecTest; - -TEST_F(HQCodecDeathTest, SettingsNumPlaceholdersZeroWriteFails) { - HTTPSettings settings; - settings.setSetting(SettingsId::_HQ_NUM_PLACEHOLDERS, 0); - HQControlCodec codec(0x1111, - TransportDirection::UPSTREAM, - StreamDirection::EGRESS, - settings, - UnidirectionalStreamType::CONTROL); - EXPECT_EXIT(codec.generateSettings(queueCtrl_), - ::testing::KilledBySignal(SIGABRT), - "Check failed: setting.value != 0"); -} - -TEST_F(HQCodecDeathTest, SettingsNumPlaceholdersUpstreamWriteFails) { - HTTPSettings settings; - settings.setSetting(SettingsId::_HQ_NUM_PLACEHOLDERS, - hq::kDefaultEgressNumPlaceHolders); - HQControlCodec codec(0x1111, - TransportDirection::UPSTREAM, - StreamDirection::EGRESS, - settings, - UnidirectionalStreamType::CONTROL); - EXPECT_EXIT( - codec.generateSettings(queueCtrl_), - ::testing::KilledBySignal(SIGABRT), - "Check failed: transportDirection_ == TransportDirection::DOWNSTREAM"); -} - -TEST_F(HQCodecTest, SettingsNumPlaceholdersDownstreamParseFails) { - HTTPSettings settings; - settings.setSetting(SettingsId::_HQ_NUM_PLACEHOLDERS, - hq::kDefaultEgressNumPlaceHolders); - HQControlCodec codec(0x1111, - TransportDirection::DOWNSTREAM, - StreamDirection::EGRESS, - settings, - UnidirectionalStreamType::CONTROL); - codec.generateSettings(queueCtrl_); - parseControl(CodecType::CONTROL_DOWNSTREAM); - EXPECT_EQ(callbacks_.headerFrames, 1); - EXPECT_EQ(callbacks_.sessionErrors, 1); -} - -TEST_F(HQCodecTest, SettingsNumPlaceholdersDownstreamWriteUpstreamParseOk) { - HTTPSettings settings; - settings.setSetting(SettingsId::_HQ_NUM_PLACEHOLDERS, - hq::kDefaultEgressNumPlaceHolders); - HQControlCodec codec(0x1111, - TransportDirection::DOWNSTREAM, - StreamDirection::EGRESS, - settings, - UnidirectionalStreamType::CONTROL); - codec.generateSettings(queueCtrl_); - parseControl(CodecType::CONTROL_UPSTREAM); -} diff --git a/proxygen/lib/http/codec/test/HQFramerTest.cpp b/proxygen/lib/http/codec/test/HQFramerTest.cpp index 0ba5900e10..dc07eb47de 100644 --- a/proxygen/lib/http/codec/test/HQFramerTest.cpp +++ b/proxygen/lib/http/codec/test/HQFramerTest.cpp @@ -137,211 +137,6 @@ INSTANTIATE_TEST_CASE_P( parseHeaders, HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_HEADERS})); -TEST_F(HQFramerTest, ParsePriorityFrameOk) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::REQUEST_STREAM, - // dependencyType - PriorityElementType::REQUEST_STREAM, - true, // exclusive - 123, // prioritizedElementId - 234, // elementDependencyId - 30, // weight - }); - EXPECT_FALSE(result.hasError()); - - FrameHeader header; - PriorityUpdate priority; - parse(folly::none, &parsePriority, header, priority); - - EXPECT_EQ(FrameType::PRIORITY, header.type); - EXPECT_EQ(priority.prioritizedType, PriorityElementType::REQUEST_STREAM); - EXPECT_EQ(priority.dependencyType, PriorityElementType::REQUEST_STREAM); - EXPECT_TRUE(priority.exclusive); - EXPECT_EQ(123, priority.prioritizedElementId); - EXPECT_EQ(234, priority.elementDependencyId); - EXPECT_EQ(30, priority.weight); -} - -TEST_F(HQFramerTest, ParsePriorityFramePrioritizeRoot) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::TREE_ROOT, - // dependencyType - PriorityElementType::REQUEST_STREAM, - true, // exclusive - 123, // prioritizedElementId - 234, // elementDependencyId - 30, // weight - }); - EXPECT_FALSE(result.hasError()); - - FrameHeader header; - PriorityUpdate priority; - parse(HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY, - &parsePriority, - header, - priority); -} - -TEST_F(HQFramerTest, ParsePriorityFramePrioritizedIdOptional) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::REQUEST_STREAM, - // dependencyType - PriorityElementType::TREE_ROOT, - true, // exclusive - 123, // prioritizedElementId - 234, // elementDependencyId (ignored!) - 30, // weight - }); - EXPECT_FALSE(result.hasError()); - - FrameHeader header; - PriorityUpdate priority; - - parse(folly::none, &parsePriority, header, priority); - EXPECT_EQ(FrameType::PRIORITY, header.type); - EXPECT_EQ(priority.prioritizedType, PriorityElementType::REQUEST_STREAM); - EXPECT_EQ(priority.dependencyType, PriorityElementType::TREE_ROOT); - EXPECT_TRUE(priority.exclusive); - EXPECT_EQ(123, priority.prioritizedElementId); - EXPECT_EQ(0, priority.elementDependencyId); - EXPECT_EQ(30, priority.weight); -} - -TEST_F(HQFramerTest, ParsePriorityWrongWeight) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::REQUEST_STREAM, - // dependencyType - PriorityElementType::REQUEST_STREAM, - true, // exclusive - 1, // prioritizedElementId - 2, // elementDependencyId - 30, // weight - }); - EXPECT_FALSE(result.hasError()); - // Flip a bit in the buffer so to force the varlength integer parsing logic - // to read extra bytes for prioritizedElementId. - auto buf = queue_.move(); - buf->coalesce(); - RWPrivateCursor cursor(buf.get()); - // 2 bytes frame header (payload length is 4) + 1 byte for flags - cursor.skip(3); - // This will cause the parser to not have enough data to read for the - // weight field - cursor.writeBE(0x42); - queue_.append(std::move(buf)); - - FrameHeader header; - PriorityUpdate priority; - parse(HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY, - &parsePriority, - header, - priority); -} - -TEST_F(HQFramerTest, ParsePriorityWrongElementDependency) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::REQUEST_STREAM, - // dependencyType - PriorityElementType::REQUEST_STREAM, - true, // exclusive - 1, // prioritizedElementId - 2, // elementDependencyId - 255, // weight - }); - EXPECT_FALSE(result.hasError()); - // Flip a bit in the buffer so to force the varlength integer parsing logic - // to read extra bytes for prioritizedElementId. - auto buf = queue_.move(); - buf->coalesce(); - RWPrivateCursor cursor(buf.get()); - // 2 bytes frame header (payload length is 4) + 1 byte for flags - cursor.skip(3); - // This will cause the parser to read two bytes (instead of 1) for the - // prioritizedElementId, then one byte for the elementDependencyId from what - // was written as the weight field. weight being all 1s the parser will try to - // read an 8-byte quic integer and there are not enough bytes available - cursor.writeBE(0x42); - queue_.append(std::move(buf)); - - FrameHeader header; - PriorityUpdate priority; - parse(HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY, - &parsePriority, - header, - priority); -} - -TEST_F(HQFramerTest, ParsePriorityTrailingJunk) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::REQUEST_STREAM, - // dependencyType - PriorityElementType::REQUEST_STREAM, - true, // exclusive - 1, // prioritizedElementId - 2, // elementDependencyId - 255, // weight - }); - EXPECT_FALSE(result.hasError()); - // Trim the frame header off - queue_.trimStart(2); - auto buf = queue_.move(); - // Put in a new frame header (too long) - auto badLength = buf->computeChainDataLength() + 4; - writeFrameHeaderManual( - queue_, static_cast(FrameType::PRIORITY), badLength); - queue_.append(std::move(buf)); - queue_.append(IOBuf::copyBuffer("junk")); - - FrameHeader header; - PriorityUpdate priority; - parse(HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY, - &parsePriority, - header, - priority); -} - -TEST_F(HQFramerTest, ParsePriorityFrameMalformedEmpty) { - auto result = writePriority(queue_, - { - // prioritizedType - PriorityElementType::REQUEST_STREAM, - // dependencyType - PriorityElementType::REQUEST_STREAM, - true, // exclusive - 1, // prioritizedElementId - 2, // elementDependencyId - 30, // weight - }); - EXPECT_FALSE(result.hasError()); - // modify the flags field so that the 'empty' field is not zero - auto buf = queue_.move(); - buf->coalesce(); - RWPrivateCursor cursor(buf.get()); - // 2 bytes frame header (payload length is 4) - cursor.skip(2); - cursor.writeBE(0x0E); - queue_.append(std::move(buf)); - - FrameHeader header; - PriorityUpdate priority; - parse(HTTP3::ErrorCode::HTTP_MALFORMED_FRAME_PRIORITY, - &parsePriority, - header, - priority); -} - TEST_F(HQFramerTest, ParsePushPromiseFrameOK) { auto data = makeBuf(1000); PushId inPushId = 4563 | kPushIdMask; @@ -466,7 +261,6 @@ INSTANTIATE_TEST_CASE_P( TEST_F(HQFramerTest, SettingsFrameOK) { deque settings = { - {hq::SettingId::NUM_PLACEHOLDERS, (SettingValue)3}, {hq::SettingId::MAX_HEADER_LIST_SIZE, (SettingValue)4}, // Unknown IDs get ignored, and identifiers of the format // "0x1f * N + 0x21" are reserved exactly for this @@ -517,11 +311,7 @@ TEST_P(HQFramerTestSettingsValues, ValueAllowed) { INSTANTIATE_TEST_CASE_P( SettingsValuesAllowedTests, HQFramerTestSettingsValues, - Values((SettingsValuesParams){hq::SettingId::NUM_PLACEHOLDERS, 0, true}, - (SettingsValuesParams){hq::SettingId::NUM_PLACEHOLDERS, - std::numeric_limits::max(), - true}, - (SettingsValuesParams){hq::SettingId::MAX_HEADER_LIST_SIZE, 0, true}, + Values((SettingsValuesParams){hq::SettingId::MAX_HEADER_LIST_SIZE, 0, true}, (SettingsValuesParams){hq::SettingId::MAX_HEADER_LIST_SIZE, std::numeric_limits::max(), true}, @@ -549,7 +339,6 @@ TEST_F(HQFramerTest, SettingsFrameEmpty) { TEST_F(HQFramerTest, SettingsFrameTrailingJunk) { deque settings = { - {hq::SettingId::NUM_PLACEHOLDERS, (SettingValue)3}, {hq::SettingId::MAX_HEADER_LIST_SIZE, (SettingValue)4}, // Unknown IDs get ignored, and identifiers of the format // "0x1f * N + 0x21" are reserved exactly for this diff --git a/proxygen/lib/http/codec/test/HQFramerTest.h b/proxygen/lib/http/codec/test/HQFramerTest.h index 635ac25ef6..21b13dc4aa 100644 --- a/proxygen/lib/http/codec/test/HQFramerTest.h +++ b/proxygen/lib/http/codec/test/HQFramerTest.h @@ -29,18 +29,6 @@ size_t writeFrameHeaderManual(folly::IOBufQueue& queue, // Write a valid frame for each frame type void writeValidFrame(folly::IOBufQueue& queue, proxygen::hq::FrameType type) { switch (type) { - case proxygen::hq::FrameType::PRIORITY: - proxygen::hq::writePriority( - queue, - { - proxygen::hq::PriorityElementType::REQUEST_STREAM, - proxygen::hq::PriorityElementType::REQUEST_STREAM, - true, - 123, - 234, - 30, - }); - break; case proxygen::hq::FrameType::SETTINGS: proxygen::hq::writeSettings( queue, diff --git a/proxygen/lib/http/session/HQDownstreamSession.h b/proxygen/lib/http/session/HQDownstreamSession.h index 74b15845fc..effdbdff82 100644 --- a/proxygen/lib/http/session/HQDownstreamSession.h +++ b/proxygen/lib/http/session/HQDownstreamSession.h @@ -29,8 +29,6 @@ class HQDownstreamSession : public HQSession { proxygen::TransportDirection::DOWNSTREAM, tinfo, sessionInfoCb) { - egressSettings_.setSetting(SettingsId::_HQ_NUM_PLACEHOLDERS, - hq::kDefaultEgressNumPlaceHolders); } void onTransportReady() noexcept override; diff --git a/proxygen/lib/http/session/HQSession.cpp b/proxygen/lib/http/session/HQSession.cpp index e72f0b2d33..01aab8ebd7 100644 --- a/proxygen/lib/http/session/HQSession.cpp +++ b/proxygen/lib/http/session/HQSession.cpp @@ -797,9 +797,6 @@ size_t HQSession::HQVersionUtils::sendSettings() { break; case hq::SettingId::MAX_HEADER_LIST_SIZE: break; - case hq::SettingId::NUM_PLACEHOLDERS: - // TODO: priorities not implemented yet - break; } } } @@ -1880,15 +1877,11 @@ void HQSession::HQVersionUtils::applySettings(const SettingsList& settings) { // this setting is stored in ingressSettings_ and enforced in the // StreamCodec break; - case hq::SettingId::NUM_PLACEHOLDERS: - numPlaceholders = setting.value; - break; } } } qpackCodec_.setEncoderHeaderTableSize(tableSize); qpackCodec_.setMaxVulnerable(blocked); - // TODO: set the num placeholder value VLOG(3) << "Applied SETTINGS sess=" << session_ << " size=" << tableSize << " blocked=" << blocked; } diff --git a/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp index d228b83490..d2e8dd44c0 100644 --- a/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp +++ b/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp @@ -1232,7 +1232,7 @@ TEST_P(HQDownstreamSessionTestH1q, BadHttpHeaders) { TEST_P(HQDownstreamSessionTestHQ, BadHttpHeaders) { auto id = nextStreamId(); - std::array badHeaders{0x02, 0x01, 0x00, 0x81}; + std::array badHeaders{0x01, 0x02, 0x00, 0x81}; auto buf = folly::IOBuf::copyBuffer(badHeaders.data(), badHeaders.size()); socketDriver_->addReadEvent(id, std::move(buf), milliseconds(0)); socketDriver_->addReadEOF(id);