brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r4387645483
##########
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:
Leaving it as is. The loop returns as soon as the count is reached, so a
passing run only waits for the event itself; the 5 s bound applies only when an
event never arrives. Every caller wraps it in `REQUIRE`, so the first timeout
ends the test case: in the repeated-decode section that is one 5 s wait, not
one per iteration.
##########
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:
Done in f658e595f0: `skip_first_block()` now allocates until a release is
seen to rewind instead of a fixed count, so it no longer depends on the block
size, and it fails the test if that never happens.
##########
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:
Done in f658e595f0: the helper appends to a `std::vector` and `REQUIRE`s
that the name and value fit their single-byte length prefixes.
##########
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:
Done in f658e595f0: `last_event()` uses `load()`, and both accessors are
`const`.
--
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]
