Copilot commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r4358341373
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -512,3 +535,266 @@ TEST_CASE("Decoding", "[qpack-decode]")
}
}
}
+
+// QPACK::_arena and the encoder stream handler are private. This reaches them
so
+// a test can tell whether the arena got back everything QPACK allocated out
of it.
+struct QPACKTestAccess {
+ // Arena::free() never inspects the last block on its list, so nothing
rewinds
+ // while the arena holds a single block. Push it past that one. This leaves
about
+ // a third of the head block free, and the allocations under test must fit
in it,
+ // otherwise they open a new head block and the water level moves for that
reason.
+ static void
+ skip_first_block(QPACK &qpack)
+ {
+ for (int i = 0; i < 40; ++i) {
+ qpack._arena.str_alloc(64);
+ }
Review Comment:
The warm-up uses hard-coded iteration/count sizes (40×64) that assume
specific Arena block sizing. If Arena block sizes/configuration differ, this
may not reliably push the arena past its first block and can make tests brittle
across builds. If there’s a block-size constant available, derive the warm-up
from it; otherwise consider allocating a single clearly-large chunk (or looping
until an observable condition changes) so the intent doesn’t depend on magic
numbers.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -512,3 +535,266 @@ TEST_CASE("Decoding", "[qpack-decode]")
}
}
}
+
+// QPACK::_arena and the encoder stream handler are private. This reaches them
so
+// a test can tell whether the arena got back everything QPACK allocated out
of it.
+struct QPACKTestAccess {
+ // Arena::free() never inspects the last block on its list, so nothing
rewinds
+ // while the arena holds a single block. Push it past that one. This leaves
about
+ // a third of the head block free, and the allocations under test must fit
in it,
+ // otherwise they open a new head block and the water level moves for that
reason.
+ static void
+ skip_first_block(QPACK &qpack)
+ {
+ for (int i = 0; i < 40; ++i) {
+ qpack._arena.str_alloc(64);
+ }
+ }
+
+ // Arena has no water level accessor. The address the next allocation gets
+ // stands in for it.
+ static void *
+ water_level(QPACK &qpack)
+ {
+ char *probe = qpack._arena.str_alloc(1);
+
+ qpack._arena.str_free(probe);
+ return probe;
+ }
+
+ // Hands encoder stream instructions straight to the handler a read on the
+ // encoder stream ends up in, on the calling thread. That skips event
dispatch,
+ // the stream id routing in _on_read_ready() and QPACK's mutex, none of which
+ // touch the arena. Nothing else runs QPACK while a test calls this.
+ static void
+ read_encoder_stream(QPACK &qpack, const uint8_t *instructions, size_t len)
+ {
+ MIOBuffer *buffer = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);
+ IOBufferReader *reader = buffer->alloc_reader();
+
+ buffer->write(instructions, len);
+ qpack._on_encoder_stream_read_ready(*reader);
+ free_MIOBuffer(buffer);
+ }
+};
+
+// Decodes one Literal Header Field Without Name Reference. That is the field
+// representation whose name and value are both allocated out of QPACK's
+// per-connection arena and released again once the header is attached, so it
is
+// the path that cares about the order those releases happen in.
+TEST_CASE("Decoding a literal header field without name reference",
"[qpack-literal-decode]")
+{
+ QUICApplicationDriver driver;
+ // decode() schedules its completion event on an event thread and hands it
+ // this handler, so every decode below is waited on before anything it can
+ // reach goes out of scope.
+ TestQPACKEventHandler event_handler;
+ auto qpack =
std::make_unique<QPACK>(driver.get_connection(), UINT32_MAX, 0, 0,
MAX_FIELD_SIZE);
+
+ // Header Data Prefix: Required Insert Count 0, Delta Base 0.
+ // Then 0x23: 001 N H <Name Length (3+)>, no never-index, no huffman, name of
+ // 3 bytes. Then 0x03: H <Value Length (7+)>, no huffman, value of 3 bytes.
+ // clang-format off
+ const uint8_t header_block[] = {
+ 0x00, 0x00,
+ 0x23, 'a', 'b', 'c',
+ 0x03, 'x', 'y', 'z',
+ };
+ // clang-format on
+
+ SECTION("the name and value survive the decode")
+ {
+ HTTPHdr hdr;
+ hdr.create(HTTPType::REQUEST);
+
+ REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr,
&event_handler, eventProcessor.all_ethreads[0]) == 0);
+ REQUIRE(event_handler.wait_for_events(1));
+ CHECK(event_handler.last_event() == QPACK_EVENT_DECODE_COMPLETE);
+
+ MIMEField *field = hdr.field_find("abc");
+
+ REQUIRE(field != nullptr);
+
+ auto value = field->value_get();
+
+ CHECK(value.length() == 3);
+ CHECK(memcmp(value.data(), "xyz", 3) == 0);
+
+ hdr.destroy();
+ }
+
+ SECTION("the arena gets back what the decode allocated")
+ {
+ QPACKTestAccess::skip_first_block(*qpack);
+ void *water_level = QPACKTestAccess::water_level(*qpack);
+
+ HTTPHdr hdr;
+ hdr.create(HTTPType::REQUEST);
+
+ REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr,
&event_handler, eventProcessor.all_ethreads[0]) == 0);
+ REQUIRE(event_handler.wait_for_events(1));
+ CHECK(event_handler.last_event() == QPACK_EVENT_DECODE_COMPLETE);
+ CHECK(QPACKTestAccess::water_level(*qpack) == water_level);
+
+ hdr.destroy();
+ }
+
+ SECTION("the arena gets back the name when the value fails to decode")
+ {
+ // The value carries the huffman flag and a length of 8, and an all-ones
+ // payload is not a decodable huffman sequence.
+ // clang-format off
+ const uint8_t bad_value_block[] = {
+ 0x00, 0x00,
+ 0x23, 'a', 'b', 'c',
+ 0x88, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ };
+ // clang-format on
+
+ QPACKTestAccess::skip_first_block(*qpack);
+ void *water_level = QPACKTestAccess::water_level(*qpack);
+
+ HTTPHdr hdr;
+ hdr.create(HTTPType::REQUEST);
+
+ REQUIRE(qpack->decode(1, bad_value_block, sizeof(bad_value_block), hdr,
&event_handler, eventProcessor.all_ethreads[0]) == 0);
+ REQUIRE(event_handler.wait_for_events(1));
+ CHECK(event_handler.last_event() == QPACK_EVENT_DECODE_FAILED);
+ CHECK(QPACKTestAccess::water_level(*qpack) == water_level);
+
+ hdr.destroy();
+ }
+
+ SECTION("repeated decodes keep returning the same field")
+ {
+ for (int i = 0; i < 200; i++) {
+ HTTPHdr hdr;
+ hdr.create(HTTPType::REQUEST);
+
+ REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr,
&event_handler, eventProcessor.all_ethreads[0]) == 0);
+ REQUIRE(event_handler.wait_for_events(i + 1));
+
+ MIMEField *field = hdr.field_find("abc");
+
+ REQUIRE(field != nullptr);
+
+ auto value = field->value_get();
+
+ REQUIRE(value.length() == 3);
+ REQUIRE(memcmp(value.data(), "xyz", 3) == 0);
+
+ hdr.destroy();
+ }
+ }
+}
+
+// Insert Without Name Reference: 01 H <Name Length (5+)> <Name> H <Value
Length (7+)> <Value>, neither string huffman coded.
+// The lengths are written as single prefix bytes, so names must be shorter
than 31 bytes and values shorter than 127.
+static size_t
+append_insert_without_name_ref(uint8_t *buf, std::string_view name,
std::string_view value)
+{
+ uint8_t *p = buf;
+
+ *p++ = 0x40 | name.size();
+ memcpy(p, name.data(), name.size());
+ p += name.size();
+ *p++ = value.size();
+ memcpy(p, value.data(), value.size());
+ p += value.size();
+
+ return p - buf;
+}
+
Review Comment:
This helper writes into `buf` without any capacity parameter/checks, so it
can overflow if the caller’s buffer is smaller than the encoded instruction. It
also doesn’t enforce the representation’s prefix-length constraints (per the
comment) so longer `name/value` will silently generate invalid encodings.
Consider passing `(uint8_t* buf, size_t cap, ...)` (or a span/vector) and
asserting/REQUIRE that `name.size()` and `value.size()` fit their prefix limits
and that `cap` is sufficient before writing.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -110,8 +115,26 @@ class TestQPACKEventHandler : public Continuation
return this->_event;
}
+ // Delivered on an event thread, so wait for this to move rather than
sleeping
+ // a fixed time when the handler must outlive the events QPACK schedules.
+ int
+ events_seen()
+ {
+ return this->_events_seen.load();
+ }
Review Comment:
`_event` is now `std::atomic<int>`, but `last_event()` returns it without an
explicit `load()`. While this may compile via the implicit conversion operator,
it’s clearer and more consistent (and avoids toolchain/library discrepancies)
to call `this->_event.load()` explicitly. Also consider making `last_event()` /
`events_seen()` `const` since they conceptually only observe state.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -110,8 +115,26 @@ class TestQPACKEventHandler : public Continuation
return this->_event;
}
+ // Delivered on an event thread, so wait for this to move rather than
sleeping
+ // a fixed time when the handler must outlive the events QPACK schedules.
+ int
+ events_seen()
+ {
+ return this->_events_seen.load();
+ }
+
+ bool
+ wait_for_events(int expected)
+ {
+ for (int i = 0; i < 5000 && this->_events_seen.load() < expected; ++i) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
+ return this->_events_seen.load() >= expected;
+ }
Review Comment:
This is a polling loop that can add up to 5s latency per call; in the
`repeated decodes` section it can become very slow or time out badly if events
stop arriving (worst-case minutes). Prefer a synchronization primitive (e.g.,
`std::condition_variable` notified by `event_handler`), or at least use a
single deadline-based loop (via `steady_clock`) with a shorter configurable
timeout to reduce flakiness and runtime impact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
