This is an automated email from the ASF dual-hosted git repository.
maskit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new aaff5a9c92 Remove H3 frame sequence number tracking (#11632)
aaff5a9c92 is described below
commit aaff5a9c922f4fbf1c3eacbf5864491d9cdd30c8
Author: Masakazu Kitajo <mas...@apache.org>
AuthorDate: Wed Jul 31 15:07:37 2024 -0600
Remove H3 frame sequence number tracking (#11632)
* Remove H3 frame sequence number tracking
* Remove redundant free_MIOBuffer call
---
include/proxy/http3/Http3FrameCounter.h | 3 +-
include/proxy/http3/Http3FrameHandler.h | 4 +-
include/proxy/http3/Http3HeaderVIOAdaptor.h | 3 +-
include/proxy/http3/Http3ProtocolEnforcer.h | 6 ++-
include/proxy/http3/Http3SettingsHandler.h | 3 +-
include/proxy/http3/Http3StreamDataVIOAdaptor.h | 3 +-
src/proxy/http3/Http3FrameCounter.cc | 3 +-
src/proxy/http3/Http3FrameDispatcher.cc | 4 +-
src/proxy/http3/Http3HeaderVIOAdaptor.cc | 2 +-
src/proxy/http3/Http3ProtocolEnforcer.cc | 9 ++--
src/proxy/http3/Http3SettingsHandler.cc | 2 +-
src/proxy/http3/Http3StreamDataVIOAdaptor.cc | 3 +-
src/proxy/http3/test/Mock.h | 3 +-
src/proxy/http3/test/test_Http3FrameDispatcher.cc | 54 +++++++++--------------
14 files changed, 42 insertions(+), 60 deletions(-)
diff --git a/include/proxy/http3/Http3FrameCounter.h
b/include/proxy/http3/Http3FrameCounter.h
index 4a03c87d7a..ead94eb28f 100644
--- a/include/proxy/http3/Http3FrameCounter.h
+++ b/include/proxy/http3/Http3FrameCounter.h
@@ -33,8 +33,7 @@ public:
// Http3FrameHandler
std::vector<Http3FrameType> interests() override;
- Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame>
frame, int32_t frame_seq = -1,
- Http3StreamType s_type =
Http3StreamType::UNKNOWN) override;
+ Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType s_type = Http3StreamType::UNKNOWN) override;
uint64_t get_count(uint64_t type) const;
diff --git a/include/proxy/http3/Http3FrameHandler.h
b/include/proxy/http3/Http3FrameHandler.h
index 8d297882ba..2a8bb3ff4b 100644
--- a/include/proxy/http3/Http3FrameHandler.h
+++ b/include/proxy/http3/Http3FrameHandler.h
@@ -32,6 +32,6 @@ class Http3FrameHandler
public:
virtual ~Http3FrameHandler(){};
virtual std::vector<Http3FrameType> interests()
= 0;
- virtual Http3ErrorUPtr handle_frame(std::shared_ptr<const
Http3Frame> frame, int32_t frame_seq = -1,
- Http3StreamType s_type =
Http3StreamType::UNKNOWN) = 0;
+ virtual Http3ErrorUPtr handle_frame(std::shared_ptr<const
Http3Frame> frame,
+ Http3StreamType
s_type = Http3StreamType::UNKNOWN) = 0;
};
diff --git a/include/proxy/http3/Http3HeaderVIOAdaptor.h
b/include/proxy/http3/Http3HeaderVIOAdaptor.h
index 24d2e79f04..ca8b3da0e6 100644
--- a/include/proxy/http3/Http3HeaderVIOAdaptor.h
+++ b/include/proxy/http3/Http3HeaderVIOAdaptor.h
@@ -35,8 +35,7 @@ public:
// Http3FrameHandler
std::vector<Http3FrameType> interests() override;
- Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame>
frame, int32_t frame_seq = -1,
- Http3StreamType s_type =
Http3StreamType::UNKNOWN) override;
+ Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType s_type = Http3StreamType::UNKNOWN) override;
bool is_complete();
int event_handler(int event, Event *data);
diff --git a/include/proxy/http3/Http3ProtocolEnforcer.h
b/include/proxy/http3/Http3ProtocolEnforcer.h
index 328143155e..ee291bdeca 100644
--- a/include/proxy/http3/Http3ProtocolEnforcer.h
+++ b/include/proxy/http3/Http3ProtocolEnforcer.h
@@ -33,6 +33,8 @@ public:
// Http3FrameHandler
std::vector<Http3FrameType> interests() override;
- Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame>
frame, int32_t frame_seq = -1,
- Http3StreamType s_type =
Http3StreamType::UNKNOWN) override;
+ Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType s_type = Http3StreamType::UNKNOWN) override;
+
+private:
+ bool _is_first_frame_received_on_control = false;
};
diff --git a/include/proxy/http3/Http3SettingsHandler.h
b/include/proxy/http3/Http3SettingsHandler.h
index 48340e7352..51545ee07f 100644
--- a/include/proxy/http3/Http3SettingsHandler.h
+++ b/include/proxy/http3/Http3SettingsHandler.h
@@ -33,8 +33,7 @@ public:
// Http3FrameHandler
std::vector<Http3FrameType> interests() override;
- Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame>
frame, int32_t frame_seq = -1,
- Http3StreamType s_type =
Http3StreamType::UNKNOWN) override;
+ Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType s_type = Http3StreamType::UNKNOWN) override;
private:
// TODO: clarify Http3Session I/F for Http3SettingsHandler and Http3App
diff --git a/include/proxy/http3/Http3StreamDataVIOAdaptor.h
b/include/proxy/http3/Http3StreamDataVIOAdaptor.h
index e13e61313c..4eff081d9d 100644
--- a/include/proxy/http3/Http3StreamDataVIOAdaptor.h
+++ b/include/proxy/http3/Http3StreamDataVIOAdaptor.h
@@ -35,8 +35,7 @@ public:
// Http3FrameHandler
std::vector<Http3FrameType> interests() override;
- Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame>
frame, int32_t frame_seq = -1,
- Http3StreamType s_type =
Http3StreamType::UNKNOWN) override;
+ Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType s_type = Http3StreamType::UNKNOWN) override;
// Http3StreamDataVIOAdaptor
void finalize();
diff --git a/src/proxy/http3/Http3FrameCounter.cc
b/src/proxy/http3/Http3FrameCounter.cc
index ae89868076..1ac83210d2 100644
--- a/src/proxy/http3/Http3FrameCounter.cc
+++ b/src/proxy/http3/Http3FrameCounter.cc
@@ -35,8 +35,7 @@ Http3FrameCounter::interests()
}
Http3ErrorUPtr
-Http3FrameCounter::handle_frame(std::shared_ptr<const Http3Frame> frame,
int32_t /* frame_seq ATS_UNUSED */,
- Http3StreamType /* s_type ATS_UNUSED */)
+Http3FrameCounter::handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType /* s_type ATS_UNUSED */)
{
Http3ErrorUPtr error = Http3ErrorUPtr(nullptr);
Http3FrameType f_type = frame->type();
diff --git a/src/proxy/http3/Http3FrameDispatcher.cc
b/src/proxy/http3/Http3FrameDispatcher.cc
index 1b6b3f32bb..bce8fefdcc 100644
--- a/src/proxy/http3/Http3FrameDispatcher.cc
+++ b/src/proxy/http3/Http3FrameDispatcher.cc
@@ -52,7 +52,6 @@ Http3FrameDispatcher::on_read_ready(QUICStreamId stream_id,
Http3StreamType stre
{
Http3ErrorUPtr error = Http3ErrorUPtr(nullptr);
nread = 0;
- uint32_t frame_count = 0;
while (true) {
// Read a length of Type field and hopefully a length of Length field too
@@ -110,7 +109,6 @@ Http3FrameDispatcher::on_read_ready(QUICStreamId stream_id,
Http3StreamType stre
break;
}
this->_bytes_to_skip = this->_current_frame->total_length();
- ++frame_count;
}
auto skip = std::min(static_cast<uint64_t>(reader.read_avail()),
this->_bytes_to_skip);
@@ -125,7 +123,7 @@ Http3FrameDispatcher::on_read_ready(QUICStreamId stream_id,
Http3StreamType stre
this->_current_frame->total_length() - _bytes_to_skip,
this->_current_frame->total_length());
std::vector<Http3FrameHandler *> handlers =
this->_handlers[static_cast<uint8_t>(type)];
for (auto h : handlers) {
- error = h->handle_frame(this->_current_frame, frame_count - 1,
stream_type);
+ error = h->handle_frame(this->_current_frame, stream_type);
if (error && error->cls != Http3ErrorClass::UNDEFINED) {
return error;
}
diff --git a/src/proxy/http3/Http3HeaderVIOAdaptor.cc
b/src/proxy/http3/Http3HeaderVIOAdaptor.cc
index 3fb501158c..8b9cac2992 100644
--- a/src/proxy/http3/Http3HeaderVIOAdaptor.cc
+++ b/src/proxy/http3/Http3HeaderVIOAdaptor.cc
@@ -54,7 +54,7 @@ Http3HeaderVIOAdaptor::interests()
}
Http3ErrorUPtr
-Http3HeaderVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame> frame,
int32_t /* frame_seq */, Http3StreamType /* s_type */)
+Http3HeaderVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType /* s_type */)
{
ink_assert(frame->type() == Http3FrameType::HEADERS);
const Http3HeadersFrame *hframe = dynamic_cast<const Http3HeadersFrame
*>(frame.get());
diff --git a/src/proxy/http3/Http3ProtocolEnforcer.cc
b/src/proxy/http3/Http3ProtocolEnforcer.cc
index ee2b26addf..9f7e8a8963 100644
--- a/src/proxy/http3/Http3ProtocolEnforcer.cc
+++ b/src/proxy/http3/Http3ProtocolEnforcer.cc
@@ -34,15 +34,15 @@ Http3ProtocolEnforcer::interests()
}
Http3ErrorUPtr
-Http3ProtocolEnforcer::handle_frame(std::shared_ptr<const Http3Frame> frame,
int32_t frame_seq, Http3StreamType s_type)
+Http3ProtocolEnforcer::handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType s_type)
{
Http3ErrorUPtr error = Http3ErrorUPtr(nullptr);
Http3FrameType f_type = frame->type();
if (s_type == Http3StreamType::CONTROL) {
- if (frame_seq == 0 && f_type != Http3FrameType::SETTINGS) {
+ if (!this->_is_first_frame_received_on_control && f_type !=
Http3FrameType::SETTINGS) {
error = std::make_unique<Http3Error>(Http3ErrorClass::CONNECTION,
Http3ErrorCode::H3_MISSING_SETTINGS,
"first frame of the control stream
must be SETTINGS frame");
- } else if (frame_seq != 0 && f_type == Http3FrameType::SETTINGS) {
+ } else if (this->_is_first_frame_received_on_control && f_type ==
Http3FrameType::SETTINGS) {
error = std::make_unique<Http3Error>(Http3ErrorClass::CONNECTION,
Http3ErrorCode::H3_FRAME_UNEXPECTED,
"only one SETTINGS frame is allowed
per the control stream");
} else if (f_type == Http3FrameType::DATA || f_type ==
Http3FrameType::HEADERS || f_type == Http3FrameType::X_RESERVED_1 ||
@@ -51,6 +51,9 @@ Http3ProtocolEnforcer::handle_frame(std::shared_ptr<const
Http3Frame> frame, int
error_msg.append(" frame is not allowed on control stream");
error = std::make_unique<Http3Error>(Http3ErrorClass::CONNECTION,
Http3ErrorCode::H3_FRAME_UNEXPECTED, error_msg.c_str());
}
+ if (!this->_is_first_frame_received_on_control) {
+ this->_is_first_frame_received_on_control = true;
+ }
} else {
if (f_type == Http3FrameType::X_RESERVED_1 || f_type ==
Http3FrameType::X_RESERVED_2 ||
f_type == Http3FrameType::X_RESERVED_3) {
diff --git a/src/proxy/http3/Http3SettingsHandler.cc
b/src/proxy/http3/Http3SettingsHandler.cc
index b00e1fb866..7bc4c7ede9 100644
--- a/src/proxy/http3/Http3SettingsHandler.cc
+++ b/src/proxy/http3/Http3SettingsHandler.cc
@@ -39,7 +39,7 @@ Http3SettingsHandler::interests()
}
Http3ErrorUPtr
-Http3SettingsHandler::handle_frame(std::shared_ptr<const Http3Frame> frame,
int32_t /* frame_seq */, Http3StreamType /* s_type */)
+Http3SettingsHandler::handle_frame(std::shared_ptr<const Http3Frame> frame,
Http3StreamType /* s_type */)
{
ink_assert(frame->type() == Http3FrameType::SETTINGS);
diff --git a/src/proxy/http3/Http3StreamDataVIOAdaptor.cc
b/src/proxy/http3/Http3StreamDataVIOAdaptor.cc
index d4ffcb3a21..7f19c277d9 100644
--- a/src/proxy/http3/Http3StreamDataVIOAdaptor.cc
+++ b/src/proxy/http3/Http3StreamDataVIOAdaptor.cc
@@ -38,8 +38,7 @@ Http3StreamDataVIOAdaptor::interests()
}
Http3ErrorUPtr
-Http3StreamDataVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame>
frame, int32_t /* frame_seq */,
- Http3StreamType /* s_type */)
+Http3StreamDataVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame>
frame, Http3StreamType /* s_type */)
{
ink_assert(frame->type() == Http3FrameType::DATA);
const Http3DataFrame *dframe = dynamic_cast<const Http3DataFrame
*>(frame.get());
diff --git a/src/proxy/http3/test/Mock.h b/src/proxy/http3/test/Mock.h
index 5c95c61cce..d31d50fb7e 100644
--- a/src/proxy/http3/test/Mock.h
+++ b/src/proxy/http3/test/Mock.h
@@ -39,8 +39,7 @@ public:
}
Http3ErrorUPtr
- handle_frame(std::shared_ptr<const Http3Frame> /* frame ATS_UNUSED */,
int32_t /* frame_seq */,
- Http3StreamType /* s_type */) override
+ handle_frame(std::shared_ptr<const Http3Frame> /* frame ATS_UNUSED */,
Http3StreamType /* s_type */) override
{
this->total_frame_received++;
return Http3ErrorUPtr(nullptr);
diff --git a/src/proxy/http3/test/test_Http3FrameDispatcher.cc
b/src/proxy/http3/test/test_Http3FrameDispatcher.cc
index e81175efe1..4062c73d89 100644
--- a/src/proxy/http3/test/test_Http3FrameDispatcher.cc
+++ b/src/proxy/http3/test/test_Http3FrameDispatcher.cc
@@ -258,45 +258,31 @@ TEST_CASE("control stream tests", "[http3]")
CHECK(nread == sizeof(input));
}
- free_MIOBuffer(buf);
-}
-
-// This test needs to run without an enforcer due to a frame counting bug.
-// Add a ProtocolEnforcer handler to reproduce.
-TEST_CASE("padding should not be interpreted as a DATA frame", "[http3]")
-{
- Http3FrameDispatcher http3FrameDispatcher;
- Http3MockFrameHandler handler;
+ SECTION("padding should not be interpreted as a DATA frame", "[http3]")
+ {
+ uint8_t input[] = {
+ 0x40, 0x04, // Type
+ 0x03, // Length
+ 0x06, // Identifier
+ 0x44, 0x00, // Value
+ };
- http3FrameDispatcher.add_handler(&handler);
+ // Initial state
+ CHECK(handler.total_frame_received == 0);
+ CHECK(nread == 0);
- MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512);
- IOBufferReader *reader = buf->alloc_reader();
- uint64_t nread = 0;
- Http3ErrorUPtr error = Http3ErrorUPtr(nullptr);
+ int total_nread{};
+ for (uint8_t *it{input}; it < input + sizeof(input); ++it) {
+ buf->write(it, 1);
+ error = http3FrameDispatcher.on_read_ready(0,
Http3StreamType::CONTROL, *reader, nread);
+ total_nread += nread;
+ CHECK(!error);
+ }
- uint8_t input[] = {
- 0x40, 0x04, // Type
- 0x03, // Length
- 0x06, // Identifier
- 0x44, 0x00, // Value
- };
-
- // Initial state
- CHECK(handler.total_frame_received == 0);
- CHECK(nread == 0);
-
- int total_nread{};
- for (uint8_t *it{input}; it < input + sizeof(input); ++it) {
- buf->write(it, 1);
- error = http3FrameDispatcher.on_read_ready(0,
Http3StreamType::CONTROL, *reader, nread);
- total_nread += nread;
- CHECK(!error);
+ CHECK(handler.total_frame_received == 1);
+ CHECK(total_nread == 6);
}
- CHECK(handler.total_frame_received == 1);
- CHECK(total_nread == 6);
-
free_MIOBuffer(buf);
}
